[PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[Hariprasad Shenai]

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.

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

Re: [PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[Bjorn Helgaas]

[+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

Re: [PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[Casey Leedom]

| 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

Re: [PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[Bjorn Helgaas]

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

Re: [PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

[Casey Leedom]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

  And now sorry for my long-delayed response — I was one vacation in Europe for four weeks hut hiking around the Monte Rosa Mastiff, etc.  Loads of fun and exhausting.


  I see your point regarding the sysfs interface.  So I think that we should make sure that all callers correctly handle short reads.  I don't think there are that many, so it should be an easy task.


Casey

________________________________
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Thursday, August 18, 2016 1:55:25 PM
To: Casey Leedom
Cc: Hariprasad S; bhelgaas@google.com; linux-pci@vger.kernel.org; SWise OGC; Hannes Reinecke
Subject: Re: [PATCH] pci: Use same logic in pci_vpd_read as that of pci_vpd_write

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

[-- Attachment #2: Type: text/html, Size: 3615 bytes --]