linux-kernel - Re: [PATCHv2] pci: Update VPD size with correct length

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]
Message-ID: <CAKgT0Ucjv0GK02T5jeE79MnR1g57XOr0ZWhGe4ETA15Y3Cnh-g@mail.gmail.com>
Date:	Wed, 16 Dec 2015 08:52:10 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Hannes Reinecke <hare@...e.de>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Michal Kubecek <mkubecek@...e.com>
Subject: Re: [PATCHv2] pci: Update VPD size with correct length

On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@...e.de> wrote:
> PCI-2.2 VPD entries have a maximum size of 32k, but might actually
> be smaller than that. To figure out the actual size one has to read
> the VPD area until the 'end marker' is reached.
> Trying to read VPD data beyond that marker results in 'interesting'
> effects, from simple read errors to crashing the card. And to make
> matters worse not every PCI card implements this properly, leaving
> us with no 'end' marker or even completely invalid data.
> This path modifies the size of the VPD attribute to the available
> size, and disables the VPD attribute altogether if no valid data
> could be read.
>
> Cc: Alexander Duyck <alexander.duyck@...il.com>
> Cc: Michal Kubecek <mkubecek@...e.com>
> Signed-off-by: Hannes Reinecke <hare@...e.de>
> ---
>  drivers/pci/access.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 59ac36f..ab571a5 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -475,6 +475,48 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
>         .release = pci_vpd_pci22_release,
>  };
>
> +/**
> + * pci_vpd_size - determine actual size of Vital Product Data
> + * @dev:       pci device struct
> + * @old_size:  current assumed size, also maximum allowed size
> + *
> + */
> +static size_t
> +pci_vpd_pci22_size(struct pci_dev *dev, size_t old_size)
> +{

Instead of passing old_size you could look at just using
PCI_VPD_PCI22_SIZE since currently your only caller will always be
passing that value anyway.

> +       size_t off = 0;
> +       unsigned char header[1+2];      /* 1 byte tag, 2 bytes length */
> +
> +       while (off < old_size && pci_read_vpd(dev, off, 1, header)) {
> +               unsigned char tag;
> +

I'm not sure the use of pci_read_vpd here is correct.  Doesn't it
return a non-zero value on error?  If so you should probably be
checking for this being greater than 0 instead of just non-zero
shouldn't you?

> +               if (header[0] == 0xff) {
> +                       /* Invalid data from VPD read */
> +                       tag = header[0];
> +               } else if (header[0] & 0x80) {
> +                       /* Large Resource Data Type Tag */
> +                       if (pci_read_vpd(dev, off+1, 2, &header[1]) != 2)
> +                               return off + 1;
> +                       off += 3 + ((header[2] << 8) | header[1]);
> +                       tag = (header[0] & 0x7f);
> +               } else {
> +                       /* Short Resource Data Type Tag */
> +                       off += 1 + (header[0] & 0x07);
> +                       tag = (header[0] & 0x78) >> 3;
> +               }
> +               if (tag == 0x0f)        /* End tag descriptor */
> +                       break;

It might make sense to just use the "return off" here since this is
the only spot that should be returning the offset.

> +               if ((tag != 0x02) && (tag != 0x10) && (tag != 0x11)) {
> +                       dev_dbg(&dev->dev,
> +                               "invalid %s vpd tag %02x at offset %zu.",
> +                               header[0] & 0x80 ? "large" : "short",
> +                               tag, off);
> +                       break;
> +               }

It might be worth while to convert some of the values used in your
conditional checks into a human readable format by converting some of
the magic numbers into defines or placing them in an enum.  Then it
makes it a bit more obvious that if the tag is not a VPD identifier
string descriptor, VPD-R descriptor, or VPD-W descriptor you should be
reporting an error.

> +       }
> +       return off;
> +}
> +

You could probably convert this to "return 0" since there is only one
spot above where you would be returning the offset from.

>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>         struct pci_vpd_pci22 *vpd;
> @@ -497,6 +539,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>         vpd->cap = cap;
>         vpd->busy = false;
>         dev->vpd = &vpd->base;
> +       vpd->base.len = pci_vpd_pci22_size(dev, vpd->base.len);
> +       if (vpd->base.len == 0) {
> +               dev_dbg(&dev->dev, "Disabling VPD access.");
> +               dev->vpd = NULL;
> +               kfree(vpd);
> +               return -ENXIO;
> +       }
>         return 0;
>  }
>

You might want to incorporate the PCI_DEV_FLAGS_VPD_REF_F0 bits that
were added a while ago in order to avoid having to reset the length
each time.  You should only need to call this on function 0 for such a
device so you may want to consider adding a check for that into your
length function.  If that flag is set and we are not testing function
zero you could just default to 0 for the base length.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/