Messages in this thread | | | From | Wolfgang Walter <> | Subject | Re: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get_prl | Date | Sun, 11 Oct 2009 03:29:49 +0200 |
| |
On Saturday 10 October 2009, David Miller wrote: > From: "Templin, Fred L" <Fred.L.Templin@boeing.com> > Date: Fri, 9 Oct 2009 17:34:49 -0700 > > > Wait a moment - I remember now that this code came > > from Yoshifuji, and I believe there was a reason for > > the cmax+1. The application is expected to know this > > and to post a large enough buffer.
But wouldn't this corrupt kernel memory?
It seems:
kp is never larger then
sizeof(struct ip_tunnel_prl) * cmax
so kp[cmax] is of by one.
userspace can't allocate a large enough buffer as doesn't know the size of the prl.
If the comment is correct and non-root can call this function it could set a.addr = htonl(INADDR_ANY); a.datalen = sizeof(*kp);
=> cmax == 1 => kp[1] would be written if prl has 2 or more entries.
So I think the patch is correct.
Probably it would even be better to check for
c >= ca
as this even more obviously correct.
> > > > Can we put this on hold until I have had a chance to > > check my e-mail archives and my local iproute changes > > (will respond on monday)? > > Sure, we can keep it out of -stable for now. > > But it is in Linus's tree so if you find we shouldn't do this > you'll need to send me a revert for net-2.6 > > Otherwise if it's good, you'll have to remind me to resubmit > it to -stable. >
This function seems to be a bit strange:
1)
if (!kp) { /* We don't try hard to allocate much memory for * non-root users. * For root users, retry allocating enough memory for * the answer. */ kp = kcalloc(ca, sizeof(*kp), GFP_ATOMIC); if (!kp) { ret = -ENOMEM; goto out;
why ist ret set to -ENOMEM it will be set to 0 here:
out: read_unlock(&ipip6_lock);
len = sizeof(*kp) * c; ret = 0;
Because c == 0 len will be 0, so only put_user(len, &a->datalen)
will be executed I'm not sure what this means for put_user(len, &a->datalen) but probably it will succed. Therefor the return value is 0 in this case.
I think the
ret = 0
should be removed as ret is initialized to 0 so it will be zero if not modifiied.
And either
if (! ret) { len = sizeof(*kp) * c; if ((len && copy_to_user(a + 1, kp, len)) || put_user(len, &a->datalen)) ret = -EFAULT; }
or
len = sizeof(*kp) * c; if (len && (opy_to_user(a + 1, kp, len) || put_user(len, &a->datalen)) ret = -EFAULT;
2) Then
c = 0; for (prl = t->prl; prl; prl = prl->next) {
c is already 0 here, so this seems to be unecessary
3) If the caller should know that the list is to large for the buffer he supplied by returning -EFAULT then cmax should be increased by one:
cmax = kprl.datalen / sizeof(kprl) + 1;
But probably user space should initialise its buffer with 0 and assume that the list was to large if the last entry's addr != 0.
Regards, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |