| 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
| ||
|
Date: Mon, 14 Aug 2023 20:09:49 +0000
From: "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To: Saurabh Singh Sengar <ssengar@...rosoft.com>,
Saurabh Sengar <ssengar@...ux.microsoft.com>,
KY Srinivasan <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with
flexible-array member
From: Saurabh Singh Sengar <ssengar@...rosoft.com> Sent: Tuesday, August 8, 2023 2:49 AM
>
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> > Sent: Monday, August 7, 2023 10:26 PM
> > To: Saurabh Sengar <ssengar@...ux.microsoft.com>; KY Srinivasan
> > <kys@...rosoft.com>; Haiyang Zhang <haiyangz@...rosoft.com>;
> > wei.liu@...nel.org; Dexuan Cui <decui@...rosoft.com>
> > Cc: linux-hyperv@...r.kernel.org; linux-kernel@...r.kernel.org; Saurabh
> > Singh Sengar <ssengar@...rosoft.com>
> > Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> > array member
> >
> > From: Saurabh Sengar <ssengar@...ux.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@...ux.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
Powered by blists - more mailing lists