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

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 [1] can have max. 4 tags:
id string, ro section, rw section, end tag.

[0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/
[1] PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags

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

On 13.10.2021 20:37, Heiner Kallweit wrote:
> 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 [1] can have max. 4 tags:
> id string, ro section, rw section, end tag.
> 
> [0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/
> [1] PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags
> 
> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 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 a4fc4d069..921470611 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;
>  
>  	while (pci_read_vpd_any(dev, off, 1, header) == 1) {
>  		size = 0;
> @@ -63,6 +64,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_any(dev, off + 1, 2, &header[1]) != 2) {
> 

Can this one be picked up for next?

On Wed, Nov 17, 2021 at 10:31:51PM +0100, Heiner Kallweit wrote:
> On 13.10.2021 20:37, Heiner Kallweit wrote:
> > 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 [1] can have max. 4 tags:
> > id string, ro section, rw section, end tag.
> > 
> > [0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/
> > [1] PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags
> > 
> > Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> > 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 a4fc4d069..921470611 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;
> >  
> >  	while (pci_read_vpd_any(dev, off, 1, header) == 1) {
> >  		size = 0;
> > @@ -63,6 +64,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_any(dev, off + 1, 2, &header[1]) != 2) {
> > 
> 
> Can this one be picked up for next?

I'm hesitating because we (or maybe just "I" :)) worked so hard to
avoid interpreting the VPD data, and now we're back to that.

There's nothing of value in this particular device's VPD.  Is there
any reason we shouldn't just use quirk_blacklist_vpd() for it?

Bjorn

On 17.11.2021 23:19, Bjorn Helgaas wrote:
> On Wed, Nov 17, 2021 at 10:31:51PM +0100, Heiner Kallweit wrote:
>> On 13.10.2021 20:37, Heiner Kallweit wrote:
>>> 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 [1] can have max. 4 tags:
>>> id string, ro section, rw section, end tag.
>>>
>>> [0] https://lore.kernel.org/lkml/20210915223218.GA1542966@bjorn-Precision-5520/
>>> [1] PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags
>>>
>>> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
>>> 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 a4fc4d069..921470611 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;
>>>  
>>>  	while (pci_read_vpd_any(dev, off, 1, header) == 1) {
>>>  		size = 0;
>>> @@ -63,6 +64,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_any(dev, off + 1, 2, &header[1]) != 2) {
>>>
>>
>> Can this one be picked up for next?
> 
> I'm hesitating because we (or maybe just "I" :)) worked so hard to
> avoid interpreting the VPD data, and now we're back to that.
> 
> There's nothing of value in this particular device's VPD.  Is there
> any reason we shouldn't just use quirk_blacklist_vpd() for it?
> 
The bogus device we talk about has vendor/device id of an Intel card,
we could blacklist just based on subvendor/device id. Seems the
quirk mechanism doesn't support subvendor id level.

In general: Once we blacklist this device, tomorrow another similarly
broken one may come. Therefore I'd prefer the more general approach.
But I see your point. If (theoretically) the next PCI spec would introduce
a new VPD tag, then we most likely would get to know about this only
once somebody complains about reading VPD from his shiny new card fails.
So it's a tradeoff ..

> Bjorn
> 
Heiner