| From: linux-pci-owner@xxxxxxxxxxxxxxx <linux-pci-owner@xxxxxxxxxxxxxxx> on behalf of Hannes Reinecke <hare@xxxxxxxx>
| Sent: Tuesday, April 12, 2016 11:00 PM
|
| On 04/12/2016 07:35 PM, Casey Leedom wrote:
| > | From: Hannes Reinecke <hare@xxxxxxxx>
| > | 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
