| Message ID | 1450262992-77276-1-git-send-email-hare@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@suse.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@gmail.com> > Cc: Michal Kubecek <mkubecek@suse.com> > Signed-off-by: Hannes Reinecke <hare@suse.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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, December 16, 2015 08:52:10 AM Alexander Duyck wrote: > On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@suse.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@gmail.com> > > Cc: Michal Kubecek <mkubecek@suse.com> > > Signed-off-by: Hannes Reinecke <hare@suse.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. > Okay, can do. > > + 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? > Yep, true. > > + 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. > Which I'm not sure about. We have three cases to worry about: a) return after the 'end' tag b) return after failing to read the 'end' tag c) return after reading an invalid tag For a) we obviously have to return the size. But for b) and c)? Just returning the maximal size (= old_size) would be exposing invalid data to userland, with the possibility of hanging the system by just reading from the attribute. So to avoid that I've been returning the size of valid data. But I'm open to suggestions if you think that's wrong. > > + 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. > Hmm Yeah, can do. > > + } > > + 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. > Weelll; as discussed above: We _might_ encounter an error after we've read a certain amount of (valid) VPD data. We surely should expose the valid data, no? Or should we invalidate the entire VPD data (and return 0) in these cases? > > 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. Okay. Cheers, Hannes
On Wed, Dec 16, 2015 at 9:01 AM, Hannes Reinecke <hare@suse.de> wrote: > On Wednesday, December 16, 2015 08:52:10 AM Alexander Duyck wrote: >> On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@suse.de> wrote: >> > + 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. >> > Which I'm not sure about. > We have three cases to worry about: > a) return after the 'end' tag > b) return after failing to read the 'end' tag > c) return after reading an invalid tag > > For a) we obviously have to return the size. > But for b) and c)? > Just returning the maximal size (= old_size) would be exposing > invalid data to userland, with the possibility of hanging the system > by just reading from the attribute. > So to avoid that I've been returning the size of valid data. > > But I'm open to suggestions if you think that's wrong. If you didn't encounter an end tag how can you be sure you have valid data? Maybe the random data managed to work out for the first couple of reads and then suddenly failed. You might have a block of data that is valid for half of something like the read-only area and is going to return garbage data starting part way through. I'd say you should handle this with an all-or-nothing type approach in order to err on the side of caution. We could then see about white listing in those rare cases where a tag is missing using something like PCI quirk since we likely cannot use a parsing based approach if we cannot find the end tag. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, December 16, 2015 09:13:35 AM Alexander Duyck wrote: > On Wed, Dec 16, 2015 at 9:01 AM, Hannes Reinecke <hare@suse.de> wrote: > > On Wednesday, December 16, 2015 08:52:10 AM Alexander Duyck wrote: > >> On Wed, Dec 16, 2015 at 2:49 AM, Hannes Reinecke <hare@suse.de> wrote: > >> > + 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. > > > > Which I'm not sure about. > > We have three cases to worry about: > > a) return after the 'end' tag > > b) return after failing to read the 'end' tag > > c) return after reading an invalid tag > > > > For a) we obviously have to return the size. > > But for b) and c)? > > Just returning the maximal size (= old_size) would be exposing > > invalid data to userland, with the possibility of hanging the system > > by just reading from the attribute. > > So to avoid that I've been returning the size of valid data. > > > > But I'm open to suggestions if you think that's wrong. > > If you didn't encounter an end tag how can you be sure you have valid > data? Maybe the random data managed to work out for the first couple > of reads and then suddenly failed. You might have a block of data > that is valid for half of something like the read-only area and is > going to return garbage data starting part way through. I'd say you > should handle this with an all-or-nothing type approach in order to > err on the side of caution. We could then see about white listing in > those rare cases where a tag is missing using something like PCI quirk > since we likely cannot use a parsing based approach if we cannot find > the end tag. Fair enough. The only 'error' cases I've encountered so far is a read of all zeroes (and a halting the machine once you've read beyond a certain point) or a read of 0xff throughout the entire area. So that approach would work for both of them. I'll be updating the patch. Cheers, Hannes
> The only 'error' cases I've encountered so far is a read of all zeroes (and a > halting the machine once you've read beyond a certain point) or a read of > 0xff throughout the entire area. So that approach would work for both of them. I should add that I'd tested the previous patch and this patch. I'll retest once v3 is available. I have a card that repeats the vpd data every 4k for all 32k. The patch as it stands truncates that to just the valid data and lspci -vvvv shows the vpd data correctly. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) +{ + 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; + + 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; + 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; + } + } + return off; +} + 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; }
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@gmail.com> Cc: Michal Kubecek <mkubecek@suse.com> Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/pci/access.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)