Message ID | 1466827722-17287-1-git-send-email-hariprasad@chelsio.com |
---|---|
State | Changes Requested |
Headers | show |
[+cc Hannes] Hi Hariprasad, On Sat, Jun 25, 2016 at 09:38:42AM +0530, Hariprasad Shenai wrote: > 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. By "the new implementation of pci_read_vpd()", are you referring to 104daa71b396 ("PCI: Determine actual VPD size on first access")? Please be explicit about which change you mean because it helps people review the change and figure out whether it should be backported. I think the existing semantics are the same as for the read(2) syscall: we return the number of bytes read, which may be less than the size requested, and callers may use random garbage if they don't check for short reads. If we make pci_read_vpd() return error instead of a short read, how do callers figure out how much to request? In the current tree, I think the following callers don't handle short reads correctly: cxl_pci_read_adapter_vpd() (cxl, used via read_vpd()) eeprom_rd_phys() (cxgb4) t4_get_raw_vpd_params() (cxgb4) sky2_show_vpd() (sky2) efx_probe_vpd_strings() (efx) vfio_vpd_config_write() (vfio) That's not a very long list, so we could certainly fix them. > Signed-off-by: Casey Leedom <leedom@chelsio.com> > Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com> > --- > drivers/pci/access.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index d11cdbb8fba3..113637de79bf 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -405,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; > -- > 2.3.4 > > -- > 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 -- 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
| From: Bjorn Helgaas <helgaas@kernel.org> | Sent: Monday, August 8, 2016 2:08 PM | | I think the existing semantics are the same as for the read(2) | syscall: we return the number of bytes read, which may be less than | the size requested, and callers may use random garbage if they don't | check for short reads. read(2) is a generic I/O system call, which maintains a read pointer with lseek(2) semantics, etc. This seems like an Apples and Oranges comparison. | If we make pci_read_vpd() return error instead of a short read, how do | callers figure out how much to request? The same question could be asked of the pci_write_vpd() call which currently throws an error is any portion of the requested write is beyond the recorded VPD area. Also, isn't this what (struct pci_dev *)->vpd.len is for? Although I think we'd want an API to retrieve that rather than have callers simply access the potentially uninitialized field which is lazily initialized. On the other hand, we don't seem to have any current callers with this issue. | In the current tree, I think the following callers don't handle short | reads correctly: | | cxl_pci_read_adapter_vpd() (cxl, used via read_vpd()) | eeprom_rd_phys() (cxgb4) | t4_get_raw_vpd_params() (cxgb4) | sky2_show_vpd() (sky2) | efx_probe_vpd_strings() (efx) | vfio_vpd_config_write() (vfio) | | That's not a very long list, so we could certainly fix them. I agree. If we do decide not to address the weirdness of returning success for a partial VPD read, then at least the fixup of the callers isn't too bad. 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
Hi Casey, Sorry for the delay in responding. On Mon, Aug 08, 2016 at 09:20:22PM +0000, Casey Leedom wrote: > | From: Bjorn Helgaas <helgaas@kernel.org> > | Sent: Monday, August 8, 2016 2:08 PM > | > | I think the existing semantics are the same as for the read(2) > | syscall: we return the number of bytes read, which may be less than > | the size requested, and callers may use random garbage if they don't > | check for short reads. > > read(2) is a generic I/O system call, which maintains a read pointer with > lseek(2) semantics, etc. This seems like an Apples and Oranges > comparison. > > | If we make pci_read_vpd() return error instead of a short read, how do > | callers figure out how much to request? > > The same question could be asked of the pci_write_vpd() call which > currently throws an error is any portion of the requested write is beyond > the recorded VPD area. Also, isn't this what (struct pci_dev *)->vpd.len > is for? Although I think we'd want an API to retrieve that rather than have > callers simply access the potentially uninitialized field which is lazily > initialized. On the other hand, we don't seem to have any current callers > with this issue. Drivers that read VPD probably can figure out how much to read. The place I'm worried about is read_vpd_attr(), the hook for reading VPD via sysfs. If we do something like "cat /sys/.../vpd", I think cat will try to read a large chunk and it will currently get a short read. If we return an error instead of the short read, I think the sysfs file will be much harder to use. Bjorn -- 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 d11cdbb8fba3..113637de79bf 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -405,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;