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

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

Hi all,

After merging the char-misc tree, today's linux-next build (x86_64
allmodconfig) failed like this:

In file included from include/linux/string.h:392,
                 from include/linux/bitmap.h:13,
                 from include/linux/cpumask.h:12,
                 from arch/x86/include/asm/paravirt.h:21,
                 from arch/x86/include/asm/irqflags.h:102,
                 from include/linux/irqflags.h:18,
                 from include/linux/spinlock.h:59,
                 from include/linux/wait.h:9,
                 from include/linux/wait_bit.h:8,
                 from include/linux/fs.h:7,
                 from include/linux/highmem.h:5,
                 from drivers/misc/vmw_vmci/vmci_context.c:10:
In function 'fortify_memset_chk',
    inlined from 'ctx_fire_notification.isra' at drivers/misc/vmw_vmci/vmci_context.c:254:3:
include/linux/fortify-string.h:480:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
  480 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Caused by commit

  bfb4cf9fb97e ("vmci: Prevent the dispatching of uninitialized payloads")

I have used the char-misc tree from next-20250702 for today.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Greg KH]

On Thu, Jul 03, 2025 at 05:10:21PM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the char-misc tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
> 
> In file included from include/linux/string.h:392,
>                  from include/linux/bitmap.h:13,
>                  from include/linux/cpumask.h:12,
>                  from arch/x86/include/asm/paravirt.h:21,
>                  from arch/x86/include/asm/irqflags.h:102,
>                  from include/linux/irqflags.h:18,
>                  from include/linux/spinlock.h:59,
>                  from include/linux/wait.h:9,
>                  from include/linux/wait_bit.h:8,
>                  from include/linux/fs.h:7,
>                  from include/linux/highmem.h:5,
>                  from drivers/misc/vmw_vmci/vmci_context.c:10:
> In function 'fortify_memset_chk',
>     inlined from 'ctx_fire_notification.isra' at drivers/misc/vmw_vmci/vmci_context.c:254:3:
> include/linux/fortify-string.h:480:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>   480 |                         __write_overflow_field(p_size_field, size);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Caused by commit
> 
>   bfb4cf9fb97e ("vmci: Prevent the dispatching of uninitialized payloads")

Ah, nice catch!  Sorry this didn't trigger in my testing.

Yes, it looks like both the code is correct, AND the warning is correct
as the compiler has no idea that you can just scribble off the end of
the structure like this.  In fact I would say the code is wrong as there
could be padding between the two fields that the change doesn't handle
properly (which in reality probably isn't happening).

Lizhi, I'm going to revert this change now, please fix it up "properly"
either by correctly changing the structure definition to show that the
payload is hanging out after the header (and also you can use the
__counted_by mark), or just properly zapping out the payload length in
the proper way instead of doing "fun" pointer math like your change did.

thanks,

greg k-h

回复: linux-next: build failure after merge of the char-misc tree

[Xu, Lizhi]

Greg, 

In ctx_fire_notification(), the following code can fully prove that the header is followed by the payload.
ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);

I sent a second version of the patch, did you notice it? It can solve the problem reported by Stephen Rothwell.

V2 Patch: https://lore.kernel.org/all/20250703075334.856445-1-lizhi.xu@windriver.com

BR,
Lizhi
________________________________________
发件人: Greg KH <greg@kroah.com>
发送时间: 2025年7月3日 16:30
收件人: Stephen Rothwell; Xu, Lizhi
抄送: Arnd Bergmann; Linux Kernel Mailing List; Linux Next Mailing List
主题: Re: linux-next: build failure after merge of the char-misc tree

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Thu, Jul 03, 2025 at 05:10:21PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the char-misc tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> In file included from include/linux/string.h:392,
>                  from include/linux/bitmap.h:13,
>                  from include/linux/cpumask.h:12,
>                  from arch/x86/include/asm/paravirt.h:21,
>                  from arch/x86/include/asm/irqflags.h:102,
>                  from include/linux/irqflags.h:18,
>                  from include/linux/spinlock.h:59,
>                  from include/linux/wait.h:9,
>                  from include/linux/wait_bit.h:8,
>                  from include/linux/fs.h:7,
>                  from include/linux/highmem.h:5,
>                  from drivers/misc/vmw_vmci/vmci_context.c:10:
> In function 'fortify_memset_chk',
>     inlined from 'ctx_fire_notification.isra' at drivers/misc/vmw_vmci/vmci_context.c:254:3:
> include/linux/fortify-string.h:480:25: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>   480 |                         __write_overflow_field(p_size_field, size);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Caused by commit
>
>   bfb4cf9fb97e ("vmci: Prevent the dispatching of uninitialized payloads")

Ah, nice catch!  Sorry this didn't trigger in my testing.

Yes, it looks like both the code is correct, AND the warning is correct
as the compiler has no idea that you can just scribble off the end of
the structure like this.  In fact I would say the code is wrong as there
could be padding between the two fields that the change doesn't handle
properly (which in reality probably isn't happening).

Lizhi, I'm going to revert this change now, please fix it up "properly"
either by correctly changing the structure definition to show that the
payload is hanging out after the header (and also you can use the
__counted_by mark), or just properly zapping out the payload length in
the proper way instead of doing "fun" pointer math like your change did.

thanks,

greg k-h

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

[Greg KH]

On Thu, Jul 03, 2025 at 08:55:31AM +0000, Xu, Lizhi wrote:
> Greg, 
> 
> In ctx_fire_notification(), the following code can fully prove that the header is followed by the payload.
> ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);
> 
> I sent a second version of the patch, did you notice it? It can solve the problem reported by Stephen Rothwell.
> 
> V2 Patch: https://lore.kernel.org/all/20250703075334.856445-1-lizhi.xu@windriver.com

I see that now, thank you.

But, if I had not reverted your previous one, that patch would not have
applied :(

Also, how can you "guarantee" that there is no padding between those
structure fields so that your "pointer math" is correct here?  Why not
fix this up properly to use the correct way to define that you have a
"payload" at the end of a structure, AND properly define how large that
payload is with the specific variable that describes that?  By doing
that, then the compiler can check when things violate those rules going
forward.

thanks,

greg k-h

回复: 回复: linux-next: build failure after merge of the char-misc tree

[Xu, Lizhi]

Perhaps you can focus on "struct vmci_event_ctx", whose members have already clearly defined which are the payloads.
On the other hand, the purpose of the patch is to prevent the data in "struct vmci_event_ctx" from being initialized before the datagram is sent, thus preventing the uninitialized data from leaking to user space.

________________________________________
发件人: Greg KH <greg@kroah.com>
发送时间: 2025年7月3日 17:28
收件人: Xu, Lizhi
抄送: Stephen Rothwell; Arnd Bergmann; Linux Kernel Mailing List; Linux Next Mailing List
主题: Re: 回复: linux-next: build failure after merge of the char-misc tree

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Thu, Jul 03, 2025 at 08:55:31AM +0000, Xu, Lizhi wrote:
> Greg,
>
> In ctx_fire_notification(), the following code can fully prove that the header is followed by the payload.
> ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);
>
> I sent a second version of the patch, did you notice it? It can solve the problem reported by Stephen Rothwell.
>
> V2 Patch: https://lore.kernel.org/all/20250703075334.856445-1-lizhi.xu@windriver.com

I see that now, thank you.

But, if I had not reverted your previous one, that patch would not have
applied :(

Also, how can you "guarantee" that there is no padding between those
structure fields so that your "pointer math" is correct here?  Why not
fix this up properly to use the correct way to define that you have a
"payload" at the end of a structure, AND properly define how large that
payload is with the specific variable that describes that?  By doing
that, then the compiler can check when things violate those rules going
forward.

thanks,

greg k-h

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

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Thu, Jul 03, 2025 at 09:44:13AM +0000, Xu, Lizhi wrote:
> 
> Perhaps you can focus on "struct vmci_event_ctx", whose members have already clearly defined which are the payloads.

I do not understand this statement at all, sorry.

> On the other hand, the purpose of the patch is to prevent the data in "struct vmci_event_ctx" from being initialized before the datagram is sent, thus preventing the uninitialized data from leaking to user space.

Great, then do this properly, again, you are just "guessing" that there
is not going to be any padding between the structures.  Are you sure
there isn't?  How?  Where is that enforced in your patch?

thanks,

greg k-h

回复: 回复: 回复: linux-next: build failure after merge of the char-misc tree

[Xu, Lizhi]

Please read the context carefully, and you will understand that this is
where everything starts. In the code before memset, the ev variable only
initializes the members of its member 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.

context:  drivers/misc/vmw_vmci/vmci_context.c
   2         for (i = 0; i < array_size; i++) {
   1                 int result;
248                  struct vmci_event_ctx ev;
   1
   2                 ev.msg.hdr.dst = vmci_handle_arr_get_entry(subscriber_array, i);
   3                 ev.msg.hdr.src = vmci_make_handle(VMCI_HYPERVISOR_CONTEXT_ID,
   4                                                   VMCI_CONTEXT_RESOURCE_ID);
   5                 ev.msg.hdr.payload_size = sizeof(ev) - sizeof(ev.msg.hdr);
   +                 memset((char*)&ev + sizeof(ev.msg.hdr), 0, ev.msg.hdr.payload_size);
   6                 ev.msg.event_data.event = VMCI_EVENT_CTX_REMOVED;
   7                 ev.payload.context_id = context_id;
   8
   9                 result = vmci_datagram_dispatch(VMCI_HYPERVISOR_CONTEXT_ID,
  10                                                 &ev.msg.hdr, false);
  11                 if (result < VMCI_SUCCESS) {
  12                         pr_devel("Failed to enqueue event datagram (type=%d) for context (ID=0x%x)\n",
  13                                  ev.msg.event_data.event,
  14                                  ev.msg.hdr.dst.context);
  15                         /* We continue to enqueue on next subscriber. */
  16                 }
  17         }

________________________________________
发件人: Greg KH <greg@kroah.com>
发送时间: 2025年7月3日 18:03
收件人: Xu, Lizhi
抄送: Stephen Rothwell; Arnd Bergmann; Linux Kernel Mailing List; Linux Next Mailing List
主题: Re: 回复: 回复: linux-next: build failure after merge of the char-misc tree

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Thu, Jul 03, 2025 at 09:44:13AM +0000, Xu, Lizhi wrote:
>
> Perhaps you can focus on "struct vmci_event_ctx", whose members have already clearly defined which are the payloads.

I do not understand this statement at all, sorry.

> On the other hand, the purpose of the patch is to prevent the data in "struct vmci_event_ctx" from being initialized before the datagram is sent, thus preventing the uninitialized data from leaking to user space.

Great, then do this properly, again, you are just "guessing" that there
is not going to be any padding between the structures.  Are you sure
there isn't?  How?  Where is that enforced in your patch?

thanks,

greg k-h

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

[Greg KH]

On Thu, Jul 03, 2025 at 11:09:10AM +0000, Xu, Lizhi 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?

> 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;

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...)

Please fix this properly.

greg k-h

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

[Lizhi Xu]

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