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 at guarana.org> > Reported-by: syzbot+87cfa083e727a224754b at syzkaller.appspotmail.comIs 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;
Michael S. Tsirkin
2018-May-30 03:01 UTC
[net] vhost: Use kzalloc() to allocate vhost_msg_node
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 at guarana.org> > > Reported-by: syzbot+87cfa083e727a224754b at 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 at 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 at guarana.org> >>> Reported-by: syzbot+87cfa083e727a224754b at 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 at 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 at 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 at guarana.org> >> > Reported-by: syzbot+87cfa083e727a224754b at 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 at 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 at 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.
Possibly Parallel Threads
- [net] vhost: Use kzalloc() to allocate vhost_msg_node
- [net] vhost: Use kzalloc() to allocate vhost_msg_node
- [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node
- [PATCH] vhost: use kzalloc() instead of kmalloc() followed by memset()
- [PATCH] vhost: make msg padding explicit