[PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[Heiner Kallweit]

We have a problem with a device where each VPD read returns 0x33 [0].
This results in a valid VPD structure (except the tag id) and
therefore pci_vpd_size() scans the full VPD address range.
On an affected system this took ca. 80s.

That's not acceptable, on the other hand we may not want to re-add
the old tag checks. In addition these tag check still wouldn't be able
to avoid the described scenario 100%.
Instead let's add a simple sanity check on the number of found tags.
A VPD image conforming to the PCI spec can have max. 4 tags:
id string, ro section, rw section, end tag.

[0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 4be248901..75e48df2e 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -56,6 +56,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 {
 	size_t off = 0, size;
 	unsigned char tag, header[1+2];	/* 1 byte tag, 2 bytes length */
+	int num_tags = 0;
 
 	/* Otherwise the following reads would fail. */
 	dev->vpd.len = PCI_VPD_MAX_SIZE;
@@ -66,6 +67,10 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
 			goto error;
 
+		/* We can have max 4 tags: STRING_ID, RO, RW, END */
+		if (++num_tags > 4)
+			goto error;
+
 		if (header[0] & PCI_VPD_LRDT) {
 			/* Large Resource Data Type Tag */
 			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
-- 
2.33.0

Re: [PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[Krzysztof Wilczyński]

Hi Heiner,

[...]
> Instead let's add a simple sanity check on the number of found tags.
> A VPD image conforming to the PCI spec can have max. 4 tags:
> id string, ro section, rw section, end tag.

It's always nice to check if something is compliant with the specification.

Would you be able to either cite this part of the official specification or
mention where to find it?  Like we do in other such changes related to some
official standards, mainly for posterity to benefit others that might look
at this commit in the future.

[...]
> +		/* We can have max 4 tags: STRING_ID, RO, RW, END */
> +		if (++num_tags > 4)
> +			goto error;

Do we want to let someone know that their device (or a device they might
have in the system) has non-compliant and/or malformed VPD which is why we
decided to return an error?  I wonder if this would help with
troubleshooting or just simply had some informative value.  So perhaps
a warning or debug level message?  What do you think?

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

	Krzysztof

Re: [PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[Heiner Kallweit]

On 17.09.2021 15:53, Krzysztof Wilczyński wrote:
> Hi Heiner,
> 
> [...]
>> Instead let's add a simple sanity check on the number of found tags.
>> A VPD image conforming to the PCI spec can have max. 4 tags:
>> id string, ro section, rw section, end tag.
> 
> It's always nice to check if something is compliant with the specification.
> 
> Would you be able to either cite this part of the official specification or
> mention where to find it?  Like we do in other such changes related to some
> official standards, mainly for posterity to benefit others that might look
> at this commit in the future.
> 
Right, I should have mentioned that:
PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags

> [...]
>> +		/* We can have max 4 tags: STRING_ID, RO, RW, END */
>> +		if (++num_tags > 4)
>> +			goto error;
> 
> Do we want to let someone know that their device (or a device they might
> have in the system) has non-compliant and/or malformed VPD which is why we
> decided to return an error?  I wonder if this would help with
> troubleshooting or just simply had some informative value.  So perhaps
> a warning or debug level message?  What do you think?
> 
A message is printed, see code after error label.  We differentiate
between "hard" and "soft" error. Soft error here means that the VPD EEPROM
is optional, in such a case it's not an actual error that the VPD reads
return non-VPD data.

> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> 	Krzysztof
> 

Re: [PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[Krzysztof Wilczyński]

Hi Heiner,

> > [...]
> >> Instead let's add a simple sanity check on the number of found tags.
> >> A VPD image conforming to the PCI spec can have max. 4 tags:
> >> id string, ro section, rw section, end tag.
> > 
> > It's always nice to check if something is compliant with the specification.
> > 
> > Would you be able to either cite this part of the official specification or
> > mention where to find it?  Like we do in other such changes related to some
> > official standards, mainly for posterity to benefit others that might look
> > at this commit in the future.
> > 
> Right, I should have mentioned that:
> PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags

Very nice!  Do you have plans to send v2 that include this information or
you reckon this is something Bjorn could add when merging if he has the
time, of course.

> > [...]
> >> +		/* We can have max 4 tags: STRING_ID, RO, RW, END */
> >> +		if (++num_tags > 4)
> >> +			goto error;
> > 
> > Do we want to let someone know that their device (or a device they might
> > have in the system) has non-compliant and/or malformed VPD which is why we
> > decided to return an error?  I wonder if this would help with
> > troubleshooting or just simply had some informative value.  So perhaps
> > a warning or debug level message?  What do you think?
> > 
> A message is printed, see code after error label.  We differentiate
> between "hard" and "soft" error. Soft error here means that the VPD EEPROM
> is optional, in such a case it's not an actual error that the VPD reads
> return non-VPD data.

Got it.  Thank you!

I had a look and, does the following:

	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
		 header[0], size, off, off == 0 ?
		 "; assume missing optional EEPROM" : "");

Still apply to having too many tags?  Would the error make sense?  Forgive
me for asking about this, especially as I am not a VPD expert, and was
simply wondering.

Also, does pci_info() there makes sense?  Not pci_warn() or pci_err(), just
so this message has more appropriate weight and logging level.  What do you
think?

> > Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

	Krzysztof

Re: [PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

[Heiner Kallweit]

On 28.09.2021 00:29, Krzysztof Wilczyński wrote:
> Hi Heiner,
> 
>>> [...]
>>>> Instead let's add a simple sanity check on the number of found tags.
>>>> A VPD image conforming to the PCI spec can have max. 4 tags:
>>>> id string, ro section, rw section, end tag.
>>>
>>> It's always nice to check if something is compliant with the specification.
>>>
>>> Would you be able to either cite this part of the official specification or
>>> mention where to find it?  Like we do in other such changes related to some
>>> official standards, mainly for posterity to benefit others that might look
>>> at this commit in the future.
>>>
>> Right, I should have mentioned that:
>> PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags
> 
> Very nice!  Do you have plans to send v2 that include this information or
> you reckon this is something Bjorn could add when merging if he has the
> time, of course.
> 
Back from vacation .. I'll send a v2.

>>> [...]
>>>> +		/* We can have max 4 tags: STRING_ID, RO, RW, END */
>>>> +		if (++num_tags > 4)
>>>> +			goto error;
>>>
>>> Do we want to let someone know that their device (or a device they might
>>> have in the system) has non-compliant and/or malformed VPD which is why we
>>> decided to return an error?  I wonder if this would help with
>>> troubleshooting or just simply had some informative value.  So perhaps
>>> a warning or debug level message?  What do you think?
>>>
>> A message is printed, see code after error label.  We differentiate
>> between "hard" and "soft" error. Soft error here means that the VPD EEPROM
>> is optional, in such a case it's not an actual error that the VPD reads
>> return non-VPD data.
> 
> Got it.  Thank you!
> 
> I had a look and, does the following:
> 
> 	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
> 		 header[0], size, off, off == 0 ?
> 		 "; assume missing optional EEPROM" : "");
> 
> Still apply to having too many tags?  Would the error make sense?  Forgive
> me for asking about this, especially as I am not a VPD expert, and was
> simply wondering.
> 
The message still is applicable, just that the tag now is invalid in a
different sense.

> Also, does pci_info() there makes sense?  Not pci_warn() or pci_err(), just
> so this message has more appropriate weight and logging level.  What do you
> think?
> 
Only impact typically is that the vpd sysfs attribute isn't available.
Userspace applications like lspci can deal with this and simply report
"can't read vpd". I doubt that it's worth it to add more complexity here.

>>> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> 	Krzysztof
> 

Heiner