LinuxLists.cc - KMSAN: uninit-value in _copy_to

[permalink] [raw]

Subject: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
Reported-by: [email protected]
---
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

2018-04-27 16:07:31

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

2018-04-27 16:13:25

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Fri, Apr 27, 2018 at 6:05 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> Reported-by: [email protected]
>
> 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 [email protected].
> 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.

2018-04-27 16:17:16

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]> 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 <[email protected]>
> >> Reported-by: [email protected]
> >
> > 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 [email protected].
> > 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.

2018-04-27 16:26:56

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Fri, Apr 27, 2018 at 6:15 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> >> Reported-by: [email protected]
>> >
>> > 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

2018-04-27 16:31:44

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov <[email protected]> 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 <[email protected]>
>>> >> Reported-by: [email protected]
>>> >
>>> > 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

2018-04-27 19:38:16

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]> 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 <[email protected]>
> >> Reported-by: [email protected]
> >
> > 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 [email protected].
> > 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.

2018-04-28 01:09:19

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
> > Reported-by: [email protected]
>
> 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

2018-04-28 01:52:37

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
> > > Reported-by: [email protected]
> >
> > 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

2018-04-28 02:24:58

by Jason Wang

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
>>>> Reported-by: [email protected]
>>> 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

2018-04-29 08:13:06

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Fri, Apr 27, 2018 at 9:36 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> >> Reported-by: [email protected]
>> >
>> > 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 [email protected].
>> > 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.

2018-05-07 13:07:08

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
> Reported-by: [email protected]
> ---
> 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 <[email protected]>

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;

2018-05-07 13:13:26

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Mon, May 7, 2018 at 3:03 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> Reported-by: [email protected]
>> ---
>> 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 <[email protected]>
>
> 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 [email protected].
> 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.

2018-05-08 08:28:16

[permalink] [raw]

Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

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 <[email protected]>
> > Reported-by: [email protected]
> > ---
> > 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

2018-05-29 22:20:43

by Guenter Roeck

[permalink] [raw]

Subject: Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node

2018-05-30 03:02:16

[permalink] [raw]

Subject: Re: [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 <[email protected]>
> > Reported-by: [email protected]
>
> 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 <[email protected]>

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;

2018-05-30 03:42:57

by Guenter Roeck

[permalink] [raw]

Subject: Re: [net] vhost: Use kzalloc() to allocate vhost_msg_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 <[email protected]>
>>> Reported-by: [email protected]
>>
>> 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 <[email protected]>
>
> 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;
>

2018-06-04 12:36:02

[permalink] [raw]

Subject: Re: [net] vhost: Use kzalloc() to allocate vhost_msg_node

On Wed, May 30, 2018 at 5:01 AM, Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>> > Reported-by: [email protected]
>>
>> 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 <[email protected]>
>
> 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 [email protected].
> 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.

2018-06-07 15:40:56

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

#syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617

Subject: vhost: fix info leak

Fixes: CVE-2018-1118
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f0be5f35ab28..9beefa6ed1ce 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2345,6 +2345,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;

2018-06-07 18:54:36

by syzbot

[permalink] [raw]

Subject: Re: Re: KMSAN: uninit-value in _copy_to_iter (2)

> #syz test: https://github.com/google/kmsan.git/master
> d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617

"https://github.com/google/kmsan.git/master" does not look like a valid git
repo address.

> Subject: vhost: fix info leak

> Fixes: CVE-2018-1118
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f0be5f35ab28..9beefa6ed1ce 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2345,6 +2345,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;

2018-06-07 18:59:06

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

On Thu, Jun 7, 2018 at 5:38 PM, Michael S. Tsirkin <[email protected]> wrote:
> #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617

Hi Michael,

We need:

#syz test: https://github.com/google/kmsan.git master

here. Please see
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
for more info.

Please also add the Reported-by tag when mailing the patch for review.

Thanks

> Subject: vhost: fix info leak
>
> Fixes: CVE-2018-1118
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f0be5f35ab28..9beefa6ed1ce 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2345,6 +2345,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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180607183627-mutt-send-email-mst%40kernel.org.
> For more options, visit https://groups.google.com/d/optout.

Attachments:

patch (518.00 B)

2018-06-07 19:03:48

by syzbot

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
[email protected]

Tested on:

commit: c6a6aed994b6 kmsan: remove dead code to trigger syzbot build
git tree: https://github.com/google/kmsan.git/master
kernel config: https://syzkaller.appspot.com/x/.config?x=848e40757852af3e
compiler: clang version 7.0.0 (trunk 334104)
patch: https://syzkaller.appspot.com/x/patch.diff?x=17dc3a8f800000

Note: testing is done by a robot and is best-effort only.

2018-06-07 19:04:36

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

#syz test: https://github.com/google/kmsan.git master

Subject: vhost: fix info leak

Fixes: CVE-2018-1118
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f0be5f35ab28..9beefa6ed1ce 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2345,6 +2345,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;

2018-06-07 19:06:32

by Al Viro

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote:
> #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617
>
> Subject: vhost: fix info leak
>
> Fixes: CVE-2018-1118
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f0be5f35ab28..9beefa6ed1ce 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2345,6 +2345,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);

Umm... Maybe kzalloc(), then? You have

struct vhost_msg_node {
struct vhost_msg msg;
struct vhost_virtqueue *vq;
struct list_head node;
};

and that's what, 68 bytes in msg, then either 4 bytes pointer or
4 bytes padding + 8 bytes pointer, then two pointers? How much
does explicit partial memset() save you here?

> node->vq = vq;
> node->msg.type = type;
> return node;

2018-06-07 19:07:05

by syzbot

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger
crash:

Reported-and-tested-by:
[email protected]

Tested on:

commit: c6a6aed994b6 kmsan: remove dead code to trigger syzbot build
git tree: https://github.com/google/kmsan.git/master
kernel config: https://syzkaller.appspot.com/x/.config?x=848e40757852af3e
compiler: clang version 7.0.0 (trunk 334104)
patch: https://syzkaller.appspot.com/x/patch.diff?x=1119eddf800000

Note: testing is done by a robot and is best-effort only.

2018-06-07 19:08:24

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote:
> On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote:
> > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617
> >
> > Subject: vhost: fix info leak
> >
> > Fixes: CVE-2018-1118
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index f0be5f35ab28..9beefa6ed1ce 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2345,6 +2345,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);
>
> Umm... Maybe kzalloc(), then? You have
>
> struct vhost_msg_node {
> struct vhost_msg msg;
> struct vhost_virtqueue *vq;
> struct list_head node;
> };
>
> and that's what, 68 bytes in msg, then either 4 bytes pointer or
> 4 bytes padding + 8 bytes pointer, then two pointers? How much
> does explicit partial memset() save you here?

Yes but 0 isn't a nop here so if this struct is used without
a sensible initialization, it will crash elsewhere.
I prefer KASAN to catch such uses.

> > node->vq = vq;
> > node->msg.type = type;
> > return node;

2018-06-07 19:08:29

by Al Viro

[permalink] [raw]

Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

On Thu, Jun 07, 2018 at 08:59:06PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote:
> > On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote:
> > > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617
> > >
> > > Subject: vhost: fix info leak
> > >
> > > Fixes: CVE-2018-1118
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index f0be5f35ab28..9beefa6ed1ce 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2345,6 +2345,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);
> >
> > Umm... Maybe kzalloc(), then? You have
> >
> > struct vhost_msg_node {
> > struct vhost_msg msg;
> > struct vhost_virtqueue *vq;
> > struct list_head node;
> > };
> >
> > and that's what, 68 bytes in msg, then either 4 bytes pointer or
> > 4 bytes padding + 8 bytes pointer, then two pointers? How much
> > does explicit partial memset() save you here?
>
> Yes but 0 isn't a nop here so if this struct is used without
> a sensible initialization, it will crash elsewhere.
> I prefer KASAN to catch such uses.
>
>
> > > node->vq = vq;
> > > node->msg.type = type;

IDGI - what would your variant catch that kzalloc + 2 assignments won't?
Accesses to uninitialized ->node? Because that's the only difference in
what is and is not initialized between those variants...

2018-06-07 19:31:01