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

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

[Saurabh Sengar]

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@linux.microsoft.com>
---
 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;
 
 struct vmgpadl_packet_header {
-- 
2.34.1

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

[Michael Kelley (LINUX)]

From: Saurabh Sengar <ssengar@linux.microsoft.com> 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@linux.microsoft.com>
> ---
>  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.

Michael

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

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Monday, August 7, 2023 10:26 PM
> To: Saurabh Sengar <ssengar@linux.microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> wei.liu@kernel.org; Dexuan Cui <decui@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Saurabh
> Singh Sengar <ssengar@microsoft.com>
> Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> array member
> 
> From: Saurabh Sengar <ssengar@linux.microsoft.com> 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@linux.microsoft.com>
> > ---
> >  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

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

[Michael Kelley (LINUX)]

From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday, August 8, 2023 2:49 AM
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Sent: Monday, August 7, 2023 10:26 PM
> > To: Saurabh Sengar <ssengar@linux.microsoft.com>; KY Srinivasan
> > <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> > wei.liu@kernel.org; Dexuan Cui <decui@microsoft.com>
> > Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Saurabh
> > Singh Sengar <ssengar@microsoft.com>
> > Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> > array member
> >
> > From: Saurabh Sengar <ssengar@linux.microsoft.com> 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@linux.microsoft.com>
> > > ---
> > >  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

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

[Michael Kelley (LINUX)]

From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Monday, August 14, 2023 1:10 PM
> From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday, August 8, 2023 2:49 AM
> >

[snip]

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

One other thought:  Could this change affect user space DPDK code that
is processing netvsc packets?

Michael

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

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Tuesday, August 15, 2023 1:46 AM
> To: Michael Kelley (LINUX) <mikelley@microsoft.com>; Saurabh Singh Sengar
> <ssengar@microsoft.com>; Saurabh Sengar <ssengar@linux.microsoft.com>;
> KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> array member
> 
> From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Monday,
> August 14, 2023 1:10 PM
> > From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday,
> > August 8, 2023 2:49 AM
> > >
> 
> [snip]
> 
> > >
> > > 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.
> >
> 
> One other thought:  Could this change affect user space DPDK code that is
> processing netvsc packets?

+ Long Li

I am aware that DPDK code uses uio_hv_generic driver to have its own
implementation of userspace netvsc and the changes here are only confined
to kernel's netvsc driver. Thus, I believe this code shouldn't affect anything
in userspace netvsc implementation.

I also browsed the DPDK code and found that DPDK has this struct implemented as
struct vmbus_chanpkt_rxbuf and that already has flexible array member.

https://github.com/DPDK/dpdk/blob/v23.07/drivers/bus/vmbus/rte_vmbus_reg.h#L182

- Saurabh

> 
> Michael

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

[Michael Kelley (LINUX)]

From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday, August 15, 2023 7:59 AM
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> > Sent: Tuesday, August 15, 2023 1:46 AM
> > To: Michael Kelley (LINUX) <mikelley@microsoft.com>; Saurabh Singh Sengar
> > <ssengar@microsoft.com>; Saurabh Sengar <ssengar@linux.microsoft.com>;
> > KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> > <decui@microsoft.com>
> > Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> > array member
> >
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Monday,
> > August 14, 2023 1:10 PM
> > > From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday,
> > > August 8, 2023 2:49 AM
> > > >
> >
> > [snip]
> >
> > > >
> > > > 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.
> > >
> >
> > One other thought:  Could this change affect user space DPDK code that is
> > processing netvsc packets?
> 
> + Long Li
> 
> I am aware that DPDK code uses uio_hv_generic driver to have its own
> implementation of userspace netvsc and the changes here are only confined
> to kernel's netvsc driver. Thus, I believe this code shouldn't affect anything
> in userspace netvsc implementation.
> 
> I also browsed the DPDK code and found that DPDK has this struct implemented as
> struct vmbus_chanpkt_rxbuf and that already has flexible array member.
> 
> https://github.com/DPDK/dpdk/blob/v23.07/drivers/bus/vmbus/rte_vmbus_reg.h#L182
> 

Sounds good to me.  Thanks for checking.

Michael