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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BYAPR21MB1688F40FA3C2CB6AEC80AC76D714A@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Tue, 15 Aug 2023 16:12:34 +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>,
        Long Li <longli@...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 15, 2023 7:59 AM
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> > Sent: Tuesday, August 15, 2023 1:46 AM
> > To: Michael Kelley (LINUX) <mikelley@...rosoft.com>; 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; Dexuan Cui
> > <decui@...rosoft.com>
> > Cc: linux-hyperv@...r.kernel.org; linux-kernel@...r.kernel.org
> > Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
> > array member
> >
> > From: Michael Kelley (LINUX) <mikelley@...rosoft.com> Sent: Monday,
> > August 14, 2023 1:10 PM
> > > From: Saurabh Singh Sengar <ssengar@...rosoft.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