RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-array member

[Michael Kelley (LINUX)]

From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx> Sent: Tuesday, August 8, 2023 2:49 AM
> 
> > -----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.
> 

Got it.  I have now carefully looked at the netvsc_receive() code myself,
and I agree with you.  With the 1 element array, the validation in
netvsc_receive() could have generated a false positive if the range_cnt
is zero.  But I don't think a zero range_cnt ever happens, so the
false positive never happens.  With the change to use a flexible array,
the validation is now correct even with a range_cnt of zero.

Please add a note to the commit message for this patch:  The validation
code in the netvsc driver is affected by changing the struct size, but the
effects have been examined and have been determined to be appropriate.

Michael