Message ID | 20160412082304.GB2771@hari-Latitude-E5550 |
---|---|
State | Not Applicable |
Headers | show |
On 04/12/2016 10:23 AM, Hariprasad Shenai wrote: [ .. ] > > Hi, > > How about adding a PCI helper function to set the actual VPD_SIZE. > > Some thing like below. We have tested this and works. The bnx2x and the tg3 > drive may also need this, because I see them calling pci_read_vpd() > with non-zero offsets. The bnx2x in particular looks like it's doing something > similar to cxgb4 so it would also probably benefit from this change (once it's > fixed to call the new pci_set_size_vpd() API). > That indeed looks reasonable. Please find some comments inline. > Thanks, > Hari > > ==== > > --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c > +++ a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c > @@ -2557,6 +2557,7 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size) > } > > #define EEPROM_STAT_ADDR 0x7bfc > +#define VPD_SIZE 0x800 > #define VPD_BASE 0x400 > #define VPD_BASE_OLD 0 > #define VPD_LEN 1024 > @@ -2594,6 +2595,15 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) > if (!vpd) > return -ENOMEM; > > + /* We have two VPD data structures stored in the adapter VPD area. > + * By default, Linux calculates the size of the VPD area by traversing > + * the first VPD area at offset 0x0, so we need to tell the OS what > + * our real VPD size is. > + */ > + ret = pci_set_size_vpd(adapter->pdev, VPD_SIZE); > + if (ret < 0) > + goto out; > + > /* Card information normally starts at VPD_BASE but early cards had > * it at 0. > */ > --- a/drivers/pci/access.c > +++ a/drivers/pci/access.c > @@ -275,6 +275,19 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void > } > EXPORT_SYMBOL(pci_write_vpd); > > +/** > + * pci_set_size_vpd - Set size of Vital Product Data space > + * @dev: pci device struct > + * @len: size of vpd space > + */ > +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len) > +{ > + if (!dev->vpd || !dev->vpd->ops) > + return -ENODEV; > + return dev->vpd->ops->set_size(dev, len); > +} > +EXPORT_SYMBOL(pci_set_size_vpd); > + > #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1) > > /** > @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > if (vpd->len == 0) > return -EIO; > > - if (pos > vpd->len) > - return 0; > - > - if (end > vpd->len) { > - end = vpd->len; > - count = end - pos; > - } > + if (end > vpd->len) > + return -EINVAL; > > if (mutex_lock_killable(&vpd->lock)) > return -EINTR; Why do you need this change? We still would be needing to validate 'pos', don't we? I'd prefer not to have this bit in. > @@ -498,9 +506,23 @@ out: > return ret ? ret : count; > } > > +static ssize_t pci_vpd_set_size(struct pci_dev *dev, size_t len) > +{ > + struct pci_vpd *vpd = dev->vpd; > + > + if (len == 0 || len > PCI_VPD_MAX_SIZE) > + return -EIO; > + > + vpd->valid = 1; > + vpd->len = len; > + > + return 0; > +} > + > static const struct pci_vpd_ops pci_vpd_ops = { > .read = pci_vpd_read, > .write = pci_vpd_write, > + .set_size = pci_vpd_set_size, > }; > > static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count, > @@ -533,9 +555,24 @@ static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count, > return ret; > } > > +static ssize_t pci_vpd_f0_set_size(struct pci_dev *dev, size_t len) > +{ > + struct pci_dev *tdev = pci_get_slot(dev->bus, > + PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); > + ssize_t ret; > + > + if (!tdev) > + return -ENODEV; > + > + ret = pci_set_size_vpd(tdev, len); > + pci_dev_put(tdev); > + return ret; > +} > + > static const struct pci_vpd_ops pci_vpd_f0_ops = { > .read = pci_vpd_f0_read, > .write = pci_vpd_f0_write, > + .set_size = pci_vpd_f0_set_size, > }; > > int pci_vpd_init(struct pci_dev *dev) > --- a/drivers/pci/pci.h > +++ a/drivers/pci/pci.h > @@ -97,6 +97,7 @@ static inline bool pci_has_subordinate(struct pci_dev *pci_dev) > struct pci_vpd_ops { > ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf); > ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); > + ssize_t (*set_size)(struct pci_dev *dev, size_t len); > }; > > struct pci_vpd { > --- a/include/linux/pci.h > +++ a/include/linux/pci.h > @@ -1111,6 +1111,7 @@ void pci_unlock_rescan_remove(void); > /* Vital product data routines */ > ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf); > ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); > +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len); > > /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */ > resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx); > > Remaining bits look okay. Cheers, Hannes
| From: Hannes Reinecke <hare@suse.com> | Sent: Tuesday, April 12, 2016 1:46 AM | To: Hariprasad S | Cc: Bjorn Helgaas; SWise OGC; Casey Leedom; linux-pci@vger.kernel.org; bhelgaas@google.com | Subject: Re: 4.6-rc2 regression with commit 104daa71b396: check VPD access offset against length | | On 04/12/2016 10:23 AM, Hariprasad Shenai wrote: | [ .. ] | > | > Hi, | > | > How about adding a PCI helper function to set the actual VPD_SIZE. | > | > Some thing like below. We have tested this and works. The bnx2x and the tg3 | > drive may also need this, because I see them calling pci_read_vpd() | > with non-zero offsets. The bnx2x in particular looks like it's doing something | > similar to cxgb4 so it would also probably benefit from this change (once it's | > fixed to call the new pci_set_size_vpd() API). | | That indeed looks reasonable. | Please find some comments inline. | | ... | | > --- a/drivers/pci/access.c | > +++ a/drivers/pci/access.c | > @@ -275,6 +275,19 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void | > } | > EXPORT_SYMBOL(pci_write_vpd); | > | > +/** | > + * pci_set_size_vpd - Set size of Vital Product Data space | > + * @dev: pci device struct | > + * @len: size of vpd space | > + */ | > +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len) | > +{ | > + if (!dev->vpd || !dev->vpd->ops) | > + return -ENODEV; | > + return dev->vpd->ops->set_size(dev, len); | > +} | > +EXPORT_SYMBOL(pci_set_size_vpd); | > + | > #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1) | > | > /** | > @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, | > if (vpd->len == 0) | > return -EIO; | > | > - if (pos > vpd->len) | > - return 0; | > - | > - if (end > vpd->len) { | > - end = vpd->len; | > - count = end - pos; | > - } | > + if (end > vpd->len) | > + return -EINVAL; | > | > if (mutex_lock_killable(&vpd->lock)) | > return -EINTR; | | Why do you need this change? | We still would be needing to validate 'pos', don't we? | I'd prefer not to have this bit in. Two reasons: 1. It makes pci_vpd_read() with pci_vpd_write() which has exactly this logic. 2. More importantly, the new implementation of pci_read_vpd() silently fails to perform a VPD read and allows the caller to use random stack garbage in the read buffer without knowing that it's not really VPD contents. If any portion of the VPD read isn't going to be performed, we should signal that back to the caller. We could either return an error or we could return the number of bytes actually read. The problem with the latter is that it would require changing every single caller to check for Requested Read Length == Actual Read Length. Returning an error is the more conservative fix and allows for rapid diagnosis of problems. And that last point is important because I spent quite a bit of time digging around trying to figure out why cxgb4 suddenly wasn't working. Casey -- 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
By the way, I should note that I don't think that cxgb4 is the only regression associated with kernel.org commit 104daa71b396 which added a check for accesses beyond the computed end of the VPD data structures starting at offset 0x0 in the VPD Space. Looking at other callers to pci_read_vpd() I see at least one other driver which pass in a non-zero offset which seems to be the same kind of thing: drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:bnx2x_read_fwinfo(): cnt = pci_read_vpd(bp->pdev, BNX2X_VPD_LEN, block_end - BNX2X_VPD_LEN, vpd_extended_data + BNX2X_VPD_LEN); Looking more in depth, if you really want to keep the new pci_read_vpd() logic where it can return partial reads, then we need to go through all of the callers and look for cases like cxgb4 where the only check is for return values less than zero and enhance them to deal with return values less than the requested VPD read request. Casey
On 4/12/2016 12:35 PM, Casey Leedom wrote: > | From: Hannes Reinecke <hare@suse.com> > | Sent: Tuesday, April 12, 2016 1:46 AM > | To: Hariprasad S > | Cc: Bjorn Helgaas; SWise OGC; Casey Leedom; linux-pci@vger.kernel.org; bhelgaas@google.com > | Subject: Re: 4.6-rc2 regression with commit 104daa71b396: check VPD access offset against length > | > | On 04/12/2016 10:23 AM, Hariprasad Shenai wrote: > | [ .. ] > | > > | > Hi, > | > > | > How about adding a PCI helper function to set the actual VPD_SIZE. > | > > | > Some thing like below. We have tested this and works. The bnx2x and the tg3 > | > drive may also need this, because I see them calling pci_read_vpd() > | > with non-zero offsets. The bnx2x in particular looks like it's doing something > | > similar to cxgb4 so it would also probably benefit from this change (once it's > | > fixed to call the new pci_set_size_vpd() API). > | > | That indeed looks reasonable. > | Please find some comments inline. > | > | ... > | > | > --- a/drivers/pci/access.c > | > +++ a/drivers/pci/access.c > | > @@ -275,6 +275,19 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void > | > } > | > EXPORT_SYMBOL(pci_write_vpd); > | > > | > +/** > | > + * pci_set_size_vpd - Set size of Vital Product Data space > | > + * @dev: pci device struct > | > + * @len: size of vpd space > | > + */ > | > +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len) > | > +{ > | > + if (!dev->vpd || !dev->vpd->ops) > | > + return -ENODEV; > | > + return dev->vpd->ops->set_size(dev, len); > | > +} > | > +EXPORT_SYMBOL(pci_set_size_vpd); > | > + > | > #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1) > | > > | > /** > | > @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > | > if (vpd->len == 0) > | > return -EIO; > | > > | > - if (pos > vpd->len) > | > - return 0; > | > - > | > - if (end > vpd->len) { > | > - end = vpd->len; > | > - count = end - pos; > | > - } > | > + if (end > vpd->len) > | > + return -EINVAL; > | > > | > if (mutex_lock_killable(&vpd->lock)) > | > return -EINTR; > | > | Why do you need this change? > | We still would be needing to validate 'pos', don't we? > | I'd prefer not to have this bit in. > > Two reasons: > > 1. It makes pci_vpd_read() with pci_vpd_write() which has exactly this > logic. > > 2. More importantly, the new implementation of pci_read_vpd() silently > fails to perform a VPD read and allows the caller to use random stack > garbage in the read buffer without knowing that it's not really VPD > contents. If any portion of the VPD read isn't going to be performed, > we should signal that back to the caller. We could either return an > error or we could return the number of bytes actually read. The problem > with the latter is that it would require changing every single caller to > check for Requested Read Length == Actual Read Length. Returning an > error is the more conservative fix and allows for rapid diagnosis of > problems. > > And that last point is important because I spent quite a bit of time > digging around trying to figure out why cxgb4 suddenly wasn't working. > > I agree with Casey. As I said before, pci_vpd_read() returning 0 and not reading anything has to be a bug, right? IMO it needs to return a negative errno if the region defined by the offset/length being read exceeds the entire vpd region length. I don't see the use of a partial read anyway. Is there utility in that? Also, I suggest the patch Hari sent out be split into two patches: 1) add the pci_set_size_vpd() API and usage by cxgb4 - this is critical for 4.6-rc 2) fixing pci_vpd_read() - perhaps less critical, but if we converge on a consensus, it can hit 4.6-rc or 4.7 Thoughts? -- 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 04/12/2016 07:35 PM, Casey Leedom wrote: > | From: Hannes Reinecke <hare@suse.com> > | Sent: Tuesday, April 12, 2016 1:46 AM > | To: Hariprasad S > | Cc: Bjorn Helgaas; SWise OGC; Casey Leedom; linux-pci@vger.kernel.org; bhelgaas@google.com > | Subject: Re: 4.6-rc2 regression with commit 104daa71b396: check VPD access offset against length > | > | On 04/12/2016 10:23 AM, Hariprasad Shenai wrote: > | [ .. ] > | > > | > Hi, > | > > | > How about adding a PCI helper function to set the actual VPD_SIZE. > | > > | > Some thing like below. We have tested this and works. The bnx2x and the tg3 > | > drive may also need this, because I see them calling pci_read_vpd() > | > with non-zero offsets. The bnx2x in particular looks like it's doing something > | > similar to cxgb4 so it would also probably benefit from this change (once it's > | > fixed to call the new pci_set_size_vpd() API). > | > | That indeed looks reasonable. > | Please find some comments inline. > | > | ... > | > | > --- a/drivers/pci/access.c > | > +++ a/drivers/pci/access.c > | > @@ -275,6 +275,19 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void > | > } > | > EXPORT_SYMBOL(pci_write_vpd); > | > > | > +/** > | > + * pci_set_size_vpd - Set size of Vital Product Data space > | > + * @dev: pci device struct > | > + * @len: size of vpd space > | > + */ > | > +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len) > | > +{ > | > + if (!dev->vpd || !dev->vpd->ops) > | > + return -ENODEV; > | > + return dev->vpd->ops->set_size(dev, len); > | > +} > | > +EXPORT_SYMBOL(pci_set_size_vpd); > | > + > | > #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1) > | > > | > /** > | > @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > | > if (vpd->len == 0) > | > return -EIO; > | > > | > - if (pos > vpd->len) > | > - return 0; > | > - > | > - if (end > vpd->len) { > | > - end = vpd->len; > | > - count = end - pos; > | > - } > | > + if (end > vpd->len) > | > + return -EINVAL; > | > > | > if (mutex_lock_killable(&vpd->lock)) > | > return -EINTR; > | > | Why do you need this change? > | We still would be needing to validate 'pos', don't we? > | I'd prefer not to have this bit in. > > Two reasons: > > 1. It makes pci_vpd_read() with pci_vpd_write() which has exactly this > logic. > > 2. More importantly, the new implementation of pci_read_vpd() silently > fails to perform a VPD read and allows the caller to use random stack > garbage in the read buffer without knowing that it's not really VPD > contents. If any portion of the VPD read isn't going to be performed, > we should signal that back to the caller. We could either return an > error or we could return the number of bytes actually read. The problem > with the latter is that it would require changing every single caller to > check for Requested Read Length == Actual Read Length. Returning an > error is the more conservative fix and allows for rapid diagnosis of > problems. > > And that last point is important because I spent quite a bit of time > digging around trying to figure out why cxgb4 suddenly wasn't working. > Okay. But wouldn't we need to check for 'pos' exceeding 'vpd->len', too? Cheers, Hannes
| From: linux-pci-owner@vger.kernel.org <linux-pci-owner@vger.kernel.org> on behalf of Hannes Reinecke <hare@suse.com> | Sent: Tuesday, April 12, 2016 11:00 PM | | On 04/12/2016 07:35 PM, Casey Leedom wrote: | > | From: Hannes Reinecke <hare@suse.com> | > | Sent: Tuesday, April 12, 2016 1:46 AM | > | ... | > | > --- a/drivers/pci/access.c | > | > +++ a/drivers/pci/access.c | > | > ... | > | > @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, | > | > if (vpd->len == 0) | > | > return -EIO; | > | > | > | > - if (pos > vpd->len) | > | > - return 0; | > | > - | > | > - if (end > vpd->len) { | > | > - end = vpd->len; | > | > - count = end - pos; | > | > - } | > | > + if (end > vpd->len) | > | > + return -EINVAL; | > | > | > | > if (mutex_lock_killable(&vpd->lock)) | > | > return -EINTR; | > | | > | Why do you need this change? | > | We still would be needing to validate 'pos', don't we? | > | I'd prefer not to have this bit in. | > | > Two reasons: | > | > 1. It makes pci_vpd_read() with pci_vpd_write() which has exactly this | > logic. | > | > 2. More importantly, the new implementation of pci_read_vpd() silently | > fails to perform a VPD read and allows the caller to use random stack | > garbage in the read buffer without knowing that it's not really VPD | > contents. If any portion of the VPD read isn't going to be performed, | > we should signal that back to the caller. We could either return an | > error or we could return the number of bytes actually read. The problem | > with the latter is that it would require changing every single caller to | > check for Requested Read Length == Actual Read Length. Returning an | > error is the more conservative fix and allows for rapid diagnosis of | > problems. | > | > And that last point is important because I spent quite a bit of time | > digging around trying to figure out why cxgb4 suddenly wasn't working. | | Okay. But wouldn't we need to check for 'pos' exceeding 'vpd->len', too? Nope. "pos", "count", and "end" are unsigned: loff_t end = pos + count; If "pos" is greater than "vpd->len", "end" will be as well. This is actually exactly the same code that's already in pci_vpd_write(). Casey -- 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
==== --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c +++ a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c @@ -2557,6 +2557,7 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size) } #define EEPROM_STAT_ADDR 0x7bfc +#define VPD_SIZE 0x800 #define VPD_BASE 0x400 #define VPD_BASE_OLD 0 #define VPD_LEN 1024 @@ -2594,6 +2595,15 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) if (!vpd) return -ENOMEM; + /* We have two VPD data structures stored in the adapter VPD area. + * By default, Linux calculates the size of the VPD area by traversing + * the first VPD area at offset 0x0, so we need to tell the OS what + * our real VPD size is. + */ + ret = pci_set_size_vpd(adapter->pdev, VPD_SIZE); + if (ret < 0) + goto out; + /* Card information normally starts at VPD_BASE but early cards had * it at 0. */ --- a/drivers/pci/access.c +++ a/drivers/pci/access.c @@ -275,6 +275,19 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void } EXPORT_SYMBOL(pci_write_vpd); +/** + * pci_set_size_vpd - Set size of Vital Product Data space + * @dev: pci device struct + * @len: size of vpd space + */ +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len) +{ + if (!dev->vpd || !dev->vpd->ops) + return -ENODEV; + return dev->vpd->ops->set_size(dev, len); +} +EXPORT_SYMBOL(pci_set_size_vpd); + #define PCI_VPD_MAX_SIZE (PCI_VPD_ADDR_MASK + 1) /** @@ -392,13 +405,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, if (vpd->len == 0) return -EIO; - if (pos > vpd->len) - return 0; - - if (end > vpd->len) { - end = vpd->len; - count = end - pos; - } + if (end > vpd->len) + return -EINVAL; if (mutex_lock_killable(&vpd->lock)) return -EINTR; @@ -498,9 +506,23 @@ out: return ret ? ret : count; } +static ssize_t pci_vpd_set_size(struct pci_dev *dev, size_t len) +{ + struct pci_vpd *vpd = dev->vpd; + + if (len == 0 || len > PCI_VPD_MAX_SIZE) + return -EIO; + + vpd->valid = 1; + vpd->len = len; + + return 0; +} + static const struct pci_vpd_ops pci_vpd_ops = { .read = pci_vpd_read, .write = pci_vpd_write, + .set_size = pci_vpd_set_size, }; static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count, @@ -533,9 +555,24 @@ static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count, return ret; } +static ssize_t pci_vpd_f0_set_size(struct pci_dev *dev, size_t len) +{ + struct pci_dev *tdev = pci_get_slot(dev->bus, + PCI_DEVFN(PCI_SLOT(dev->devfn), 0)); + ssize_t ret; + + if (!tdev) + return -ENODEV; + + ret = pci_set_size_vpd(tdev, len); + pci_dev_put(tdev); + return ret; +} + static const struct pci_vpd_ops pci_vpd_f0_ops = { .read = pci_vpd_f0_read, .write = pci_vpd_f0_write, + .set_size = pci_vpd_f0_set_size, }; int pci_vpd_init(struct pci_dev *dev) --- a/drivers/pci/pci.h +++ a/drivers/pci/pci.h @@ -97,6 +97,7 @@ static inline bool pci_has_subordinate(struct pci_dev *pci_dev) struct pci_vpd_ops { ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf); ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); + ssize_t (*set_size)(struct pci_dev *dev, size_t len); }; struct pci_vpd { --- a/include/linux/pci.h +++ a/include/linux/pci.h @@ -1111,6 +1111,7 @@ void pci_unlock_rescan_remove(void); /* Vital product data routines */ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf); ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf); +ssize_t pci_set_size_vpd(struct pci_dev *dev, size_t len); /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */ resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);