RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-array member
From: Saurabh Singh Sengar
Date: Tue Aug 08 2023 - 12:08:56 EST
> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Sent: Monday, August 7, 2023 10:26 PM
> To: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; Dexuan Cui <decui@xxxxxxxxxxxxx>
> Cc: linux-hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Saurabh
> Singh Sengar <ssengar@xxxxxxxxxxxxx>
> Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> array member
>
> From: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Monday, August
> 7, 2023 2:51 AM
> >
> > One-element and zero-length arrays are deprecated. Replace one-element
> > array in struct vmtransfer_page_packet_header with flexible-array
> > member. This change fixes below warning:
> >
> > [ 2.593788]
> >
> ================================================================
> ================
> > [ 2.593908] UBSAN: array-index-out-of-bounds in
> drivers/net/hyperv/netvsc.c:1445:41
> > [ 2.593989] index 1 is out of range for type 'vmtransfer_page_range [1]'
> > [ 2.594049] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.5.0-rc4-next-
> 20230803+ #1
> > [ 2.594114] Hardware name: Microsoft Corporation Virtual
> Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 04/20/2023
> > [ 2.594121] Call Trace:
> > [ 2.594126] <IRQ>
> > [ 2.594133] dump_stack_lvl+0x4c/0x70
> > [ 2.594154] dump_stack+0x14/0x20
> > [ 2.594162] __ubsan_handle_out_of_bounds+0xa6/0xf0
> > [ 2.594224] netvsc_poll+0xc01/0xc90 [hv_netvsc]
> > [ 2.594258] __napi_poll+0x30/0x1e0
> > [ 2.594320] net_rx_action+0x194/0x2f0
> > [ 2.594333] __do_softirq+0xde/0x31e
> > [ 2.594345] __irq_exit_rcu+0x6b/0x90
> > [ 2.594357] irq_exit_rcu+0x12/0x20
> > [ 2.594366] sysvec_hyperv_callback+0x84/0x90
> > [ 2.594376] </IRQ>
> > [ 2.594379] <TASK>
> > [ 2.594383] asm_sysvec_hyperv_callback+0x1f/0x30
> > [ 2.594394] RIP: 0010:pv_native_safe_halt+0xf/0x20
> > [ 2.594452] Code: 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90
> 90 90
> > 90 90 90 90 90 90 f3 0f 1e fa 66 90 0f 00 2d 05 35 3f 00 fb f4 <c3> cc
> > cc cc cc 66 2e 0f 1f 84 00 00 00 00 00 66 90 90 90 90 90 90
> > [ 2.594459] RSP: 0018:ffffb841c00d3e88 EFLAGS: 00000256
> > [ 2.594469] RAX: ffff9d18c326f4a0 RBX: ffff9d18c031df40 RCX:
> 4000000000000000
> > [ 2.594475] RDX: 0000000000000001 RSI: 0000000000000082 RDI:
> 00000000000268dc
> > [ 2.594481] RBP: ffffb841c00d3e90 R08: 00000066a171109b R09:
> 00000000d33d2600
> > [ 2.594486] R10: 000000009a41bf00 R11: 0000000000000000 R12:
> 0000000000000001
> > [ 2.594491] R13: 0000000000000000 R14: 0000000000000000 R15:
> 0000000000000000
> > [ 2.594501] ? ct_kernel_exit.constprop.0+0x7d/0x90
> > [ 2.594513] ? default_idle+0xd/0x20
> > [ 2.594523] arch_cpu_idle+0xd/0x20
> > [ 2.594532] default_idle_call+0x30/0xe0
> > [ 2.594542] do_idle+0x200/0x240
> > [ 2.594553] ? complete+0x71/0x80
> > [ 2.594613] cpu_startup_entry+0x24/0x30
> > [ 2.594624] start_secondary+0x12d/0x160
> > [ 2.594634] secondary_startup_64_no_verify+0x17e/0x18b
> > [ 2.594649] </TASK>
> > [ 2.594656]
> >
> ================================================================
> ======
> > ==========
> >
> > With this change the structure size is reduced by 8 bytes, below is
> > the pahole output.
> >
> > struct vmtransfer_page_packet_header {
> > struct vmpacket_descriptor d; /* 0 16 */
> > u16 xfer_pageset_id; /* 16 2 */
> > u8 sender_owns_set; /* 18 1 */
> > u8 reserved; /* 19 1 */
> > u32 range_cnt; /* 20 4 */
> > struct vmtransfer_page_range ranges[]; /* 24 0 */
> >
> > /* size: 24, cachelines: 1, members: 6 */
> > /* last cacheline: 24 bytes */
> > };
> >
> > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> > ---
> > include/linux/hyperv.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index
> > bfbc37ce223b..c529f407bfb8 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -348,7 +348,7 @@ struct vmtransfer_page_packet_header {
> > u8 sender_owns_set;
> > u8 reserved;
> > u32 range_cnt;
> > - struct vmtransfer_page_range ranges[1];
> > + struct vmtransfer_page_range ranges[];
> > } __packed;
> >
>
> As you noted, switching to a flexible array member changes the
> size of struct vmtransfer_page_packet_header. In netvsc_receive()
> the size of this structure is used in validation of the VMbus packet received
> from Hyper-V. Changing the size of the structure changes
> the validation. The validation code probably needs to be adjusted
> to account for the new structure size (or the original validation code was
> wrong).
>
> There might be other places in the code that are affected by a change in the
> size of this structure. I haven't fully investigated.
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make this change.
Through my analysis, I arrived at the conclusion that the initial validation code
wasn't entirely accurate. And with the proposed changes it gets more accurate.
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size validation"
is only to make sure that header is correct. So, that we fetch the range_cnt and
xfer_pageset_id correctly from it. For this to be done I don't find any reason
to include the size of ranges in this check. With inclusion of ranges we are
checking the first 'struct vmtransfer_page_range' size as well which is not required
for fetching above two values.
Once we have the count of ranges we will anyway check the sanity of ranges with
NETVSC_XFER_HEADER_SIZE. This will check "count * (struct vmtransfer_page_range)"
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between both the checks as
of today.
Please let me know you opinion if you don't find my explanation reasonable.
I don't see any other place this structure's size change will affect.
- Saurabh
>
> Michael