| Message ID | 20101028060529.GX6062@bicker |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
You've got a leak if copy_user fails. While testing this, I realized that printk() won't print more than 1k in a single call, anyways, so I've sent along a patch that just copies up to 1k onto the stack, which should prevent the overflow without changing behavior or needing a heap allocation. - Nelson On Thu, Oct 28, 2010 at 08:05:29AM +0200, Dan Carpenter wrote: > Nelson Elhage says he was able to oops both amd64 and i386 test > machines with 8k writes to the pktgen file. Let's just allocate the > buffer on the heap instead of on the stack. > > This can only be triggered by root so there are no security issues here. > > Reported-by: Nelson Elhage <nelhage@ksplice.com> > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > v3: just use kmalloc() > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 2c0df0f..c8d3620 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file, > i += len; > > if (debug) { > - char tb[count + 1]; > + char *tb; > + > + tb = kmalloc(count + 1, GFP_KERNEL); > + if (!tb) > + return -ENOMEM; > if (copy_from_user(tb, user_buffer, count)) > return -EFAULT; > tb[count] = 0; > printk(KERN_DEBUG "pktgen: %s,%lu buffer -:%s:-\n", name, > (unsigned long)count, tb); > + kfree(tb); > } > > if (!strcmp(name, "min_pkt_size")) { -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 28, 2010 at 11:22:22AM -0400, Nelson Elhage wrote: > You've got a leak if copy_user fails. > My QC scripts should have caught that, but they didn't... I'll figure it out. It shouldn't happen again. > While testing this, I realized that printk() won't print more than 1k in a > single call, anyways, so I've sent along a patch that just copies up to 1k onto > the stack, which should prevent the overflow without changing behavior or > needing a heap allocation. > Ok. Good to hear. Sorry I wasted people's time. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 28, 2010 at 06:28:25PM +0200, Dan Carpenter wrote: > On Thu, Oct 28, 2010 at 11:22:22AM -0400, Nelson Elhage wrote: > > You've got a leak if copy_user fails. > > > > My QC scripts should have caught that, but they didn't... I'll figure > it out. It shouldn't happen again. > > > While testing this, I realized that printk() won't print more than 1k in a > > single call, anyways, so I've sent along a patch that just copies up to 1k onto > > the stack, which should prevent the overflow without changing behavior or > > needing a heap allocation. > > > > Ok. Good to hear. Sorry I wasted people's time. No worries. I appreciate you jumping in to help, even if it looks like the approach wasn't needed in the end. - Nelson > > regards, > dan carpenter > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dan Carpenter <error27@gmail.com> writes: > Reported-by: Nelson Elhage <nelhage@ksplice.com> > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > v3: just use kmalloc() > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 2c0df0f..c8d3620 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file, > i += len; > > if (debug) { > - char tb[count + 1]; > + char *tb; > + > + tb = kmalloc(count + 1, GFP_KERNEL); This is still trivially exploitable (for root) -- think what happens when count is near ULONG_MAX -Andi
> > @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file, > > i += len; > > > > if (debug) { > > - char tb[count + 1]; > > + char *tb; > > + > > + tb = kmalloc(count + 1, GFP_KERNEL); > > > This is still trivially exploitable (for root) -- think what happens > when count is near ULONG_MAX > In vfs_write() we call rw_verify_area() which caps count at INT_MAX or LONG_MAX. if (unlikely((ssize_t) count < 0)) return retval; So I get lucky this time... ;) regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/pktgen.c b/net/core/pktgen.c index 2c0df0f..c8d3620 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -887,12 +887,17 @@ static ssize_t pktgen_if_write(struct file *file, i += len; if (debug) { - char tb[count + 1]; + char *tb; + + tb = kmalloc(count + 1, GFP_KERNEL); + if (!tb) + return -ENOMEM; if (copy_from_user(tb, user_buffer, count)) return -EFAULT; tb[count] = 0; printk(KERN_DEBUG "pktgen: %s,%lu buffer -:%s:-\n", name, (unsigned long)count, tb); + kfree(tb); } if (!strcmp(name, "min_pkt_size")) {
Nelson Elhage says he was able to oops both amd64 and i386 test machines with 8k writes to the pktgen file. Let's just allocate the buffer on the heap instead of on the stack. This can only be triggered by root so there are no security issues here. Reported-by: Nelson Elhage <nelhage@ksplice.com> Signed-off-by: Dan Carpenter <error27@gmail.com> --- v3: just use kmalloc() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html