On Fri, Jul 08, 2022 at 02:44:08PM +0800, Zhu, Lingshan wrote:

On 7/4/2022 12:39 PM, Jason Wang wrote:

quoted

On Fri, Jul 1, 2022 at 9:36 PM Zhu Lingshan [off-list ref] wrote:

quoted

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.

quoted

If we do this, it's probably a blocker for cross vendor stuff.

Thanks

quoted

Signed-off-by: Zhu Lingshan <redacted>
---
  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