netdev - Re: [PATCH V3 1/6] vDPA/ifcvf: get_config_size should return a value no greater than dev implementation

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Jul 2022 01:44:37 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        netdev <netdev@...r.kernel.org>, Parav Pandit <parav@...dia.com>,
        Yongji Xie <xieyongji@...edance.com>,
        "Dawar, Gautam" <gautam.dawar@....com>
Subject: Re: [PATCH V3 1/6] vDPA/ifcvf: get_config_size should return a value
 no greater than dev implementation

On Fri, Jul 08, 2022 at 02:44:08PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 7/4/2022 12:39 PM, Jason Wang wrote:
> > On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan <lingshan.zhu@...el.com> wrote:
> > > ifcvf_get_config_size() should return a virtio device type specific value,
> > > however the ret_value should not be greater than the onboard size of
> > > the device implementation. E.g., for virtio_net, config_size should be
> > > the minimum value of sizeof(struct virtio_net_config) and the onboard
> > > cap size.
> > Rethink of this, I wonder what's the value of exposing device
> > implementation details to users? Anyhow the parent is in charge of
> > "emulating" config space accessing.
> This will not be exposed to the users, it is a ifcvf internal helper,
> to get the actual device config space size.
> 
> For example, if ifcvf drives an Intel virtio-net device,
> if the device config space size is greater than sizeof(struct
> virtio_net_cfg),
> this means the device has something more than the spec, some private fields,
> we don't want to expose these extra private fields to the users, so in this
> case,
> we only return what the spec defines.

This is kind of already the case.

> If the device config space size is less than sizeof(struct virtio_net_cfg),
> means the device didn't implement all fields the spec defined, like no RSS.
> In such cases, we only return what the device implemented.
> So these are defensive programming.

I think the issue you are describing is simply this.


Driver must not access BAR outside capability length. Current code
does not verify that it does not. Not the case for the current
devices but it's best to be safe against the case where
device does not implement some of the capability.


>From that POV I think the patch is good, just fix the log.



> > 
> > If we do this, it's probably a blocker for cross vendor stuff.
> > 
> > Thanks
> > 
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
> > > ---
> > >   drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++--
> > >   drivers/vdpa/ifcvf/ifcvf_base.h |  2 ++
> > >   2 files changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > index 48c4dadb0c7c..fb957b57941e 100644
> > > --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> > > @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
> > >                          break;
> > >                  case VIRTIO_PCI_CAP_DEVICE_CFG:
> > >                          hw->dev_cfg = get_cap_addr(hw, &cap);
> > > +                       hw->cap_dev_config_size = le32_to_cpu(cap.length);
> > >                          IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
> > >                          break;
> > >                  }
> > > @@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
> > >   u32 ifcvf_get_config_size(struct ifcvf_hw *hw)
> > >   {
> > >          struct ifcvf_adapter *adapter;
> > > +       u32 net_config_size = sizeof(struct virtio_net_config);
> > > +       u32 blk_config_size = sizeof(struct virtio_blk_config);
> > > +       u32 cap_size = hw->cap_dev_config_size;
> > >          u32 config_size;
> > > 
> > >          adapter = vf_to_adapter(hw);
> > > +       /* If the onboard device config space size is greater than
> > > +        * the size of struct virtio_net/blk_config, only the spec
> > > +        * implementing contents size is returned, this is very
> > > +        * unlikely, defensive programming.
> > > +        */
> > >          switch (hw->dev_type) {
> > >          case VIRTIO_ID_NET:
> > > -               config_size = sizeof(struct virtio_net_config);
> > > +               config_size = cap_size >= net_config_size ? net_config_size : cap_size;
> > >                  break;
> > >          case VIRTIO_ID_BLOCK:
> > > -               config_size = sizeof(struct virtio_blk_config);
> > > +               config_size = cap_size >= blk_config_size ? blk_config_size : cap_size;
> > >                  break;
> > >          default:
> > >                  config_size = 0;
> > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> > > index 115b61f4924b..f5563f665cc6 100644
> > > --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> > > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> > > @@ -87,6 +87,8 @@ struct ifcvf_hw {
> > >          int config_irq;
> > >          int vqs_reused_irq;
> > >          u16 nr_vring;
> > > +       /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> > > +       u32 cap_dev_config_size;
> > >   };
> > > 
> > >   struct ifcvf_adapter {
> > > --
> > > 2.31.1
> > >