Virtualization - Jul 2022 - [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 at
intel.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 at intel.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
> > >