linux-kernel - Re: linux-next: build failure after merge of the char-misc tree

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] [day] [month] [year] [list]

Message-ID: <20250704013236.703903-1-lizhi.xu@windriver.com>
Date: Fri, 4 Jul 2025 09:32:36 +0800
From: Lizhi Xu <lizhi.xu@...driver.com>
To: <greg@...ah.com>
CC: <Lizhi.Xu@...driver.com>, <arnd@...db.de>, <linux-kernel@...r.kernel.org>,
        <linux-next@...r.kernel.org>, <sfr@...b.auug.org.au>
Subject: Re: linux-next: build failure after merge of the char-misc tree

On Thu, 3 Jul 2025 13:20:47 +0200, Greg KH wrote:
> > Please read the context carefully, and you will understand that this is
> > where everything starts.
> 
> I'm sorry, but I do not understand your quoting style.  Didn't the links
> I provided earlier explain this?
> 
> > In the code before memset, the ev variable only
> > initializes the members of its member hdr.
> 
> What code does that?
    static int ctx_fire_notification(u32 context_id, u32 priv_flags)
...	
   5                 struct vmci_event_ctx ev;
   4
   3                 ev.msg.hdr.dst = vmci_handle_arr_get_entry(subscriber_array, i);
   2                 ev.msg.hdr.src = vmci_make_handle(VMCI_HYPERVISOR_CONTEXT_ID,
   1                                                   VMCI_CONTEXT_RESOURCE_ID);
253                  ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);
> 
> > Originally, "struct vmci_event_ctx ev = {0};" could be used to solve this
> > problem. After careful analysis, I can clearly see that the data after the
> > ev member hdr is not fully initialized, so memset() is used to set the
> > uninitialized data after the hdr member in ev to 0.
> 
> Again, you have a structure that has 2 structures in it, but no
> guarantees that there will not be any padding between those structures:
> 
> struct vmci_event_ctx {
>         struct vmci_event_msg msg;
>         struct vmci_event_payld_ctx payload;
> };
> 
> Nor do you have any guarantee that those structures don't also have
> holes in them.  How does any of this work?  Is it just luck?  I walked
> things backwards and find it impossible to guess as to any of the fields
> here actually being properly aligned or even using the correct data
> types to cross the user/kernel boundary.
> 
> And then you throw the whole thing on the stack:
> 
> > 248                  struct vmci_event_ctx ev;
1.
struct vmci_event_msg and struct vmci_event_payld_ctx, those struct are
used to contain data for events.
Size of this struct is a multiple of 8 bytes.
So no hole.

2. 
ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);
It means:
2.1 ev.msg.hdr.payload_size is sizeof(struct vmci_event_ctx) - sizeof(struct vmci_datagram)
2.2 The size of payload_size is fixed.

3.
Yes, they are on the stack now.
ctx_fire_notification()->
  vmci_datagram_dispatch()->
    dg_dispatch_as_host()->

Before the datagram is queued in dg_dispatch_as_host(), a copy of the
datagram in the stack is made using kmemdup() and then queued.

4.
Before they all enter the queue, they are in the data preparation stage,
the actual event datagram is not really saved, and the size of payload_size
is fixed, so it is clear that except for hdr, setting the payload content
to 0 before initializing other data will not introduce any unknown behavior.
> 
> And attempt to initialize the fields manually.  What could go wrong?
> (hint, syzbot showed what went wrong, and the compiler is now telling
> you how your proposed fix is not correct in the long-run...)

BR,
Lizhi