netdev - Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+ZUoAB8ZORGnR+xhcg46XqP+HsfbeuQ+HNcRVE51xdH6A@mail.gmail.com>
Date:   Fri, 27 Apr 2018 18:29:20 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Kevin Easton <kevin@...rana.org>, Jason Wang <jasowang@...hat.com>,
        KVM list <kvm@...r.kernel.org>,
        virtualization@...ts.linux-foundation.org,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

On Fri, Apr 27, 2018 at 6:25 PM, Dmitry Vyukov <dvyukov@...gle.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@...rana.org>
>>> >> Reported-by: syzbot+87cfa083e727a224754b@...kaller.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