| Message ID | 20101027224302.GQ6062@bicker |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
You want to add a trailing NUL, or else printk will read off the end of the buffer. Also, by memdup()ing count + 1 bytes, you're technically reading one more byte than userspace asked for, which could in principle lead to a spurious EFAULT. - Nelson On Thu, Oct 28, 2010 at 12:43:02AM +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> > --- > I saw this on twitter. Hi Nelson, could you test this? > > V2: strndup_user() => memdup_user() > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index 2c0df0f..b5d3c70 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -887,12 +887,14 @@ static ssize_t pktgen_if_write(struct file *file, > i += len; > > if (debug) { > - char tb[count + 1]; > - if (copy_from_user(tb, user_buffer, count)) > - return -EFAULT; > - tb[count] = 0; > + char *tb; > + > + tb = memdup_user(user_buffer, count + 1); > + if (IS_ERR(tb)) > + return PTR_ERR(tb); > 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 Wed, Oct 27, 2010 at 07:06:57PM -0400, Nelson Elhage wrote: > You want to add a trailing NUL, or else printk will read off the end of the > buffer. > > Also, by memdup()ing count + 1 bytes, you're technically reading one more byte > than userspace asked for, which could in principle lead to a spurious EFAULT. > That's a lot of bugs per line. :( I'm eating humble pie today... 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..b5d3c70 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -887,12 +887,14 @@ static ssize_t pktgen_if_write(struct file *file, i += len; if (debug) { - char tb[count + 1]; - if (copy_from_user(tb, user_buffer, count)) - return -EFAULT; - tb[count] = 0; + char *tb; + + tb = memdup_user(user_buffer, count + 1); + if (IS_ERR(tb)) + return PTR_ERR(tb); 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> --- I saw this on twitter. Hi Nelson, could you test this? V2: strndup_user() => memdup_user() -- 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