| Message ID | 20180427154502.GA22544@la.guarana.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] vhost: Use kzalloc() to allocate vhost_msg_node | expand |
On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Does it help if a patch naming the padding is applied, and then we init just the relevant field? Just curious. > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq; > -- > 2.8.1
On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> so it should be allocated with kzalloc() to ensure all structure padding >> is zeroed. >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Does it help if a patch naming the padding is applied, > and then we init just the relevant field? > Just curious. Yes, it would help. >> --- >> drivers/vhost/vhost.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f3bd8e9..1b84dcff 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> /* Create a new message. */ >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> { >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> if (!node) >> return NULL; >> node->vq = vq; >> -- >> 2.8.1 > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout.
On Fri, Apr 27, 2018 at 06:11:31PM +0200, Dmitry Vyukov wrote: > On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, > >> so it should be allocated with kzalloc() to ensure all structure padding > >> is zeroed. > >> > >> Signed-off-by: Kevin Easton <kevin@guarana.org> > >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > Yes, it would help. I think it's slightly better that way then. node has a lot of internal stuff we don't care to init. Would you mind taking my patch and building on top of that then? > >> --- > >> drivers/vhost/vhost.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >> index f3bd8e9..1b84dcff 100644 > >> --- a/drivers/vhost/vhost.c > >> +++ b/drivers/vhost/vhost.c > >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > >> /* Create a new message. */ > >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > >> { > >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > >> if (!node) > >> return NULL; > >> node->vq = vq; > >> -- > >> 2.8.1 > > > > -- > > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > > For more options, visit https://groups.google.com/d/optout.
On Fri, Apr 27, 2018 at 6:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> >> so it should be allocated with kzalloc() to ensure all structure padding >> >> is zeroed. >> >> >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> > >> > Does it help if a patch naming the padding is applied, >> > and then we init just the relevant field? >> > Just curious. >> >> Yes, it would help. > > I think it's slightly better that way then. node has a lot of internal > stuff we don't care to init. Would you mind taking my patch and building > on top of that then? But it's asking for more information leaks in future. This looks like work for compiler. >> >> --- >> >> drivers/vhost/vhost.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> >> index f3bd8e9..1b84dcff 100644 >> >> --- a/drivers/vhost/vhost.c >> >> +++ b/drivers/vhost/vhost.c >> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> >> /* Create a new message. */ >> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> >> { >> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> >> if (!node) >> >> return NULL; >> >> node->vq = vq; >> >> -- >> >> 2.8.1
On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov <dvyukov@google.com> wrote: >>> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>> >> so it should be allocated with kzalloc() to ensure all structure padding >>> >> is zeroed. >>> >> >>> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >>> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >>> > >>> > Does it help if a patch naming the padding is applied, >>> > and then we init just the relevant field? >>> > Just curious. >>> >>> Yes, it would help. >> >> I think it's slightly better that way then. node has a lot of internal >> stuff we don't care to init. Would you mind taking my patch and building >> on top of that then? > > > But it's asking for more information leaks in future. This looks like > work for compiler. Modern compilers are perfectly capable of doing this: #include <memory.h> #include <unistd.h> int main() { int x[10]; memset(&x, 0, sizeof(x)); x[0] = 0; x[2] = 2; x[3] = 3; x[4] = 4; x[5] = 5; x[6] = 6; x[7] = 7; x[8] = 8; x[9] = 9; write(0, x, sizeof(x)); return 0; } gcc 7.2 -O3 0000000000000540 <main>: 540: sub $0x38,%rsp 544: mov $0x28,%edx 549: xor %edi,%edi 54b: movdqa 0x1cd(%rip),%xmm0 # 720 <_IO_stdin_used+0x10> 553: mov %rsp,%rsi 556: movq $0x0,(%rsp) 55e: movups %xmm0,0x8(%rsp) 563: movdqa 0x1c5(%rip),%xmm0 # 730 <_IO_stdin_used+0x20> 56b: movups %xmm0,0x18(%rsp) 570: callq 520 <write@plt> 575: xor %eax,%eax 577: add $0x38,%rsp 57b: retq 57c: nopl 0x0(%rax) But they will not put a security hole next time fields are shuffled. >>> >> --- >>> >> drivers/vhost/vhost.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>> >> index f3bd8e9..1b84dcff 100644 >>> >> --- a/drivers/vhost/vhost.c >>> >> +++ b/drivers/vhost/vhost.c >>> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >>> >> /* Create a new message. */ >>> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >>> >> { >>> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >>> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >>> >> if (!node) >>> >> return NULL; >>> >> node->vq = vq; >>> >> -- >>> >> 2.8.1
On Fri, Apr 27, 2018 at 06:11:31PM +0200, Dmitry Vyukov wrote: > On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, > >> so it should be allocated with kzalloc() to ensure all structure padding > >> is zeroed. > >> > >> Signed-off-by: Kevin Easton <kevin@guarana.org> > >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > Yes, it would help. How about a Tested-by tag then? > >> --- > >> drivers/vhost/vhost.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >> index f3bd8e9..1b84dcff 100644 > >> --- a/drivers/vhost/vhost.c > >> +++ b/drivers/vhost/vhost.c > >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > >> /* Create a new message. */ > >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > >> { > >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > >> if (!node) > >> return NULL; > >> node->vq = vq; > >> -- > >> 2.8.1 > > > > -- > > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. > > For more options, visit https://groups.google.com/d/optout.
On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Does it help if a patch naming the padding is applied, > and then we init just the relevant field? > Just curious. No, I don't believe that is sufficient to fix the problem. The structure is allocated by kmalloc(), then individual fields are initialised. The named adding would be forced to be initialised if it were initialised with a struct initialiser, but that's not the case. The compiler is free to leave padding0 with whatever junk kmalloc() left there. Having said that, naming the padding *does* help - technically, the compiler is allowed to put whatever it likes in the padding every time you modify the struct. It really needs both. I didn't name the padding in my original patch because I wasn't sure if the padding actually exists on 32 bit architectures? - Kevin
On Fri, Apr 27, 2018 at 09:07:56PM -0400, Kevin Easton wrote: > On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: > > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > > so it should be allocated with kzalloc() to ensure all structure padding > > > is zeroed. > > > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > > > Does it help if a patch naming the padding is applied, > > and then we init just the relevant field? > > Just curious. > > No, I don't believe that is sufficient to fix the problem. Scratch that, somehow I missed the "..and then we init just the relevant field" part, sorry. There's still the padding after the vhost_iotlb_msg to consider. It's named in the union but I don't think that's guaranteed to be initialised when the iotlb member of the union is used to initialise things. > I didn't name the padding in my original patch because I wasn't sure > if the padding actually exists on 32 bit architectures? This might still be a concern, too? At the end of the day, zeroing 96 bytes (the full size of vhost_msg_node) is pretty quick. - Kevin
On 2018年04月28日 09:51, Kevin Easton wrote: > On Fri, Apr 27, 2018 at 09:07:56PM -0400, Kevin Easton wrote: >> On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote: >>> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >>>> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>>> so it should be allocated with kzalloc() to ensure all structure padding >>>> is zeroed. >>>> >>>> Signed-off-by: Kevin Easton <kevin@guarana.org> >>>> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >>> Does it help if a patch naming the padding is applied, >>> and then we init just the relevant field? >>> Just curious. >> No, I don't believe that is sufficient to fix the problem. > Scratch that, somehow I missed the "..and then we init just the > relevant field" part, sorry. > > There's still the padding after the vhost_iotlb_msg to consider. It's > named in the union but I don't think that's guaranteed to be initialised > when the iotlb member of the union is used to initialise things. > >> I didn't name the padding in my original patch because I wasn't sure >> if the padding actually exists on 32 bit architectures? > This might still be a conce Yes. print &((struct vhost_msg *)0)->iotlb $3 = (struct vhost_iotlb_msg *) 0x4 > > At the end of the day, zeroing 96 bytes (the full size of vhost_msg_node) > is pretty quick. > > - Kevin Right, and even if it may be used heavily in the data-path, zeroing is not the main delay in that path. Thanks
On Fri, Apr 27, 2018 at 9:36 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> >> so it should be allocated with kzalloc() to ensure all structure padding >> >> is zeroed. >> >> >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> > >> > Does it help if a patch naming the padding is applied, >> > and then we init just the relevant field? >> > Just curious. >> >> Yes, it would help. > > How about a Tested-by tag then? I didn't test either patch. >> >> --- >> >> drivers/vhost/vhost.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> >> index f3bd8e9..1b84dcff 100644 >> >> --- a/drivers/vhost/vhost.c >> >> +++ b/drivers/vhost/vhost.c >> >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> >> /* Create a new message. */ >> >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> >> { >> >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> >> if (!node) >> >> return NULL; >> >> node->vq = vq; >> >> -- >> >> 2.8.1 >> > >> > -- >> > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. >> > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. >> > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180427185501-mutt-send-email-mst%40kernel.org. >> > For more options, visit https://groups.google.com/d/optout.
On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq; Let's just init the msg though. OK it seems this is the best we can do for now, we need a new feature bit to fix it for 32 bit userspace on 64 bit kernels. Does the following help? Signed-off-by: Michael S. Tsirkin <mst@redhat.com> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e9..58d9aec 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node;
On Mon, May 7, 2018 at 3:03 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> so it should be allocated with kzalloc() to ensure all structure padding >> is zeroed. >> >> Signed-off-by: Kevin Easton <kevin@guarana.org> >> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> --- >> drivers/vhost/vhost.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index f3bd8e9..1b84dcff 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> /* Create a new message. */ >> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> { >> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> if (!node) >> return NULL; >> node->vq = vq; > > > Let's just init the msg though. > > OK it seems this is the best we can do for now, > we need a new feature bit to fix it for 32 bit > userspace on 64 bit kernels. > > Does the following help? Hi Michael, You can ask reporter (syzbot) to test: https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches https://github.com/google/syzkaller/blob/master/docs/syzbot.md#kmsan-bugs > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..58d9aec 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180507155534-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout.
On Mon, May 07, 2018 at 04:03:25PM +0300, Michael S. Tsirkin wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > --- > > drivers/vhost/vhost.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index f3bd8e9..1b84dcff 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > > /* Create a new message. */ > > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > { > > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > > if (!node) > > return NULL; > > node->vq = vq; > > > Let's just init the msg though. > > OK it seems this is the best we can do for now, > we need a new feature bit to fix it for 32 bit > userspace on 64 bit kernels. > > Does the following help? Yes, the reproducer doesn't trigger the error with that patch applied. - Kevin
On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > so it should be allocated with kzalloc() to ensure all structure padding > is zeroed. > > Signed-off-by: Kevin Easton <kevin@guarana.org> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com Is this patch going anywhere ? The patch fixes CVE-2018-1118. It would be useful to understand if and when this problem is going to be fixed. Thanks, Guenter > --- > drivers/vhost/vhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e9..1b84dcff 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > /* Create a new message. */ > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > { > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > node->vq = vq;
On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: > > The struct vhost_msg within struct vhost_msg_node is copied to userspace, > > so it should be allocated with kzalloc() to ensure all structure padding > > is zeroed. > > > > Signed-off-by: Kevin Easton <kevin@guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com > > Is this patch going anywhere ? > > The patch fixes CVE-2018-1118. It would be useful to understand if and when > this problem is going to be fixed. > > Thanks, > Guenter > > --- > > drivers/vhost/vhost.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index f3bd8e9..1b84dcff 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); > > /* Create a new message. */ > > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > > { > > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); > > if (!node) > > return NULL; > > node->vq = vq; As I pointed out, we don't need to init the whole structure. The proper fix is thus (I think) below. Could you use your testing infrastructure to confirm this fixes the issue? Thanks! Signed-off-by: Michael S. Tsirkin <mst@redhat.com> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e941224..58d9aec90afb 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; + + /* Make sure all padding within the structure is initialized. */ + memset(&node->msg, 0, sizeof node->msg); node->vq = vq; node->msg.type = type; return node;
On 05/29/2018 08:01 PM, Michael S. Tsirkin wrote: > On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: >> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >>> The struct vhost_msg within struct vhost_msg_node is copied to userspace, >>> so it should be allocated with kzalloc() to ensure all structure padding >>> is zeroed. >>> >>> Signed-off-by: Kevin Easton <kevin@guarana.org> >>> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> >> Is this patch going anywhere ? >> >> The patch fixes CVE-2018-1118. It would be useful to understand if and when >> this problem is going to be fixed. >> >> Thanks, >> Guenter >>> --- >>> drivers/vhost/vhost.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>> index f3bd8e9..1b84dcff 100644 >>> --- a/drivers/vhost/vhost.c >>> +++ b/drivers/vhost/vhost.c >>> @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >>> /* Create a new message. */ >>> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >>> { >>> - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >>> + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >>> if (!node) >>> return NULL; >>> node->vq = vq; > > As I pointed out, we don't need to init the whole structure. The proper > fix is thus (I think) below. > > Could you use your testing infrastructure to confirm this fixes the issue? > Sorry, I don't have the means to test the fix. Guenter > Thanks! > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e941224..58d9aec90afb 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; >
On Wed, May 30, 2018 at 5:01 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, May 29, 2018 at 03:19:08PM -0700, Guenter Roeck wrote: >> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote: >> > The struct vhost_msg within struct vhost_msg_node is copied to userspace, >> > so it should be allocated with kzalloc() to ensure all structure padding >> > is zeroed. >> > >> > Signed-off-by: Kevin Easton <kevin@guarana.org> >> > Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com >> >> Is this patch going anywhere ? >> >> The patch fixes CVE-2018-1118. It would be useful to understand if and when >> this problem is going to be fixed. >> >> Thanks, >> Guenter >> > --- >> > drivers/vhost/vhost.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> > index f3bd8e9..1b84dcff 100644 >> > --- a/drivers/vhost/vhost.c >> > +++ b/drivers/vhost/vhost.c >> > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); >> > /* Create a new message. */ >> > struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) >> > { >> > - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); >> > + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); >> > if (!node) >> > return NULL; >> > node->vq = vq; > > As I pointed out, we don't need to init the whole structure. The proper > fix is thus (I think) below. > > Could you use your testing infrastructure to confirm this fixes the issue? Hi Michael, syzbot is self-service, see: https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches > Thanks! > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index f3bd8e941224..58d9aec90afb 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -2342,6 +2342,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) > struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); > if (!node) > return NULL; > + > + /* Make sure all padding within the structure is initialized. */ > + memset(&node->msg, 0, sizeof node->msg); > node->vq = vq; > node->msg.type = type; > return node; > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180530055704-mutt-send-email-mst%40kernel.org. > For more options, visit https://groups.google.com/d/optout.
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index f3bd8e9..1b84dcff 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify); /* Create a new message. */ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type) { - struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL); + struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL); if (!node) return NULL; node->vq = vq;
The struct vhost_msg within struct vhost_msg_node is copied to userspace, so it should be allocated with kzalloc() to ensure all structure padding is zeroed. Signed-off-by: Kevin Easton <kevin@guarana.org> Reported-by: syzbot+87cfa083e727a224754b@syzkaller.appspotmail.com --- drivers/vhost/vhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)