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

2018-05-07 Thread Dmitry Vyukov via Virtualization 
On Mon, May 7, 2018 at 3:03 PM, 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 
>> Reported-by: syzbot+87cfa083e727a2247...@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 
>
> 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(>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+unsubscr...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-05-07 Thread Michael S. Tsirkin 
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 
> Reported-by: syzbot+87cfa083e727a2247...@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 

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(>msg, 0, sizeof node->msg);
node->vq = vq;
node->msg.type = type;
return node;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-29 Thread Dmitry Vyukov via Virtualization 
On Fri, Apr 27, 2018 at 9:36 PM, Michael S. Tsirkin  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 
>> >> Reported-by: syzbot+87cfa083e727a2247...@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+unsubscr...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Jason Wang 



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 
Reported-by: syzbot+87cfa083e727a2247...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Michael S. Tsirkin 
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  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 
> >> Reported-by: syzbot+87cfa083e727a2247...@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+unsubscr...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Dmitry Vyukov via Virtualization 
On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov  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 
>>> >> Reported-by: syzbot+87cfa083e727a2247...@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 
#include 
int main()
{
int x[10];
memset(, 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

0540 :
 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 
 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Dmitry Vyukov via Virtualization 
On Fri, Apr 27, 2018 at 6:15 PM, Michael S. Tsirkin  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 
>> >> Reported-by: syzbot+87cfa083e727a2247...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Michael S. Tsirkin 
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  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 
> >> Reported-by: syzbot+87cfa083e727a2247...@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+unsubscr...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Dmitry Vyukov via Virtualization 
On Fri, Apr 27, 2018 at 6:05 PM, 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 
>> Reported-by: syzbot+87cfa083e727a2247...@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+unsubscr...@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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2018-04-27 Thread Michael S. Tsirkin 
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 
> Reported-by: syzbot+87cfa083e727a2247...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization