Messages in this thread | | | From | "Templin, Fred L" <> | Date | Mon, 12 Oct 2009 16:58:14 -0700 | Subject | RE: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get_prl |
| |
Wolfgang,
Thanks for your comments. As I wrote to David and Greg, I tested the patch and it is indeed good. I'd really rather not get into changing other code that has already been tested at this time, however.
Regards - Fred fred.l.templin@boeing.com
> -----Original Message----- > From: Wolfgang Walter [mailto:wolfgang.walter@stwm.de] > Sent: Saturday, October 10, 2009 6:30 PM > To: David Miller > Cc: Templin, Fred L; gregkh@suse.de; linux-kernel@vger.kernel.org; stable@kernel.org; stable- > review@kernel.org; torvalds@linux-foundation.org; akpm@linux-foundation.org; > alan@lxorguk.ukuu.org.uk; contact@saschahlusiak.de; yoshfuji@linux-ipv6.org > Subject: Re: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get_prl > > 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/
| |