[PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()

[Daniel P. Berrangé]

On Tue, Dec 08, 2020 at 08:37:43PM -0500, Laine Stump wrote:
> Previously there was no way to differentiate between this function 1)
> encountering an error while reading the pci config, and 2) determining
> that the device in question is a conventional PCI device, and so has
> no Express Capabilities.
> 
> The difference between these two conditions is important, because an
> unprivileged libvirtd will be unable to read all of the pci config (it
> can only read the first 64 bytes, and will get ENOENT when it tries to
> seek past that limit) even though the device is in fact a PCIe device.
> 
> This patch just changes virPCIDeviceFindCapabilityOffset() to put the
> determined offset into an argument of the function (rather than
> sending it back as the return value), and to return the standard "0 on
> success, -1 on failure". Failure is determined by checking the value
> of errno after each attemptd read of the config file (which can only
> work reliably if errno is reset to 0 before each read, and after
> virPCIDeviceFindCapabilityOffset() has finished examining it).
> 
> (NB: if the config file is read successfully, but no Express
> Capabilities are found, then the function returns success, but the
> returned offset will be 0 (which is an impossible offset for Express
> Capabilities, and so easily recognizeable).
> 
> An upcoming patch will take advantage of the change made here.
> 
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
>  src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 9109fb4f3a..2f99bb93bd 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
>                   unsigned int buflen)
>  {
>      memset(buf, 0, buflen);
> +    errno = 0;
>  
>      if (lseek(cfgfd, pos, SEEK_SET) != pos ||
>          saferead(cfgfd, buf, buflen) != buflen) {
> @@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
>      return ret;
>  }
>


I think there should be some API docs added here to describe the
semantics this method is trying to achieve wrt unprivileged
usage and pci vs pcie.

> -static uint8_t
> +static int
>  virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
>                                   int cfgfd,
> -                                 unsigned int capability)
> +                                 unsigned int capability,
> +                                 unsigned int *offset)
>  {
>      uint16_t status;
>      uint8_t pos;
>  
> +    *offset = 0; /* assume failure (*nothing* can be at offset 0) */
> +
>      status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
> -    if (!(status & PCI_STATUS_CAP_LIST))
> -        return 0;
> +    if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
> +        goto error;
>  
>      pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
> +    if (errno != 0)
> +        goto error;
>  
>      /* Zero indicates last capability, capabilities can't
>       * be in the config space header and 0xff is returned
> @@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
>       */
>      while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
>          uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
> +        if (errno != 0)
> +            goto error;
> +
>          if (capid == capability) {
>              VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
>                        dev->id, dev->name, capability, pos);
> -            return pos;
> +            *offset = pos;
> +            return 0;
>          }
>  
>          pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
> +        if (errno != 0)
> +            goto error;
>      }
>  
> -    VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name, capability);
> + error:
> +    VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
> +              dev->id, dev->name, capability, g_strerror(errno));
>  
> -    return 0;
> +    /* reset errno in case the failure was due to insufficient
> +     * privileges to read the entire PCI config file
> +     */
> +    errno = 0;
> +
> +    return -1;
>  }
>  
>  static unsigned int
> @@ -553,7 +572,7 @@ static bool
>  virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
>  {
>      uint32_t caps;
> -    uint8_t pos;
> +    unsigned int pos;
>      g_autofree char *path = NULL;
>      int found;
>  
> @@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
>       * the same thing, except for conventional PCI
>       * devices. This is not common yet.
>       */
> -    pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
> +
>      if (pos) {
>          caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
>          if (caps & PCI_AF_CAP_FLR) {
> @@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
>  static int
>  virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
>  {
> -    dev->pcie_cap_pos   = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
> -    dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP, &dev->pcie_cap_pos);
> +    virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM, &dev->pci_pm_cap_pos);
>      dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
>      dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
>  
> -- 
> 2.28.0
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|