[Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found)

[Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found)

[Josselin Mouette]

We’ve been observing a subtle kernel bug on a few servers after kernel
upgrades (starting from 5.15 and persisting in 6.8-rc1). The bug arises
only on machines with Mellanox Connect-X 3 cards and the symptom is
RabbitMQ disconnections caused by packet loss on the system Ethernet
card (Intel I350). Replacing the I350 by a 82580 produced the exact
same symptoms.

A bisect led to this change:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=5fe204eab174fd474227f23fd47faee4e7a6c000

Reverting the patch and adding more warnings (patch follows) allowed us
to identify that the VPD data in the Connect-X 3 firmware is missing
VPD_STIN_END, which makes it return at a 32k offset. But I presume the
VPD data is incorrect far before that 32k limit.
[   43.854869] mlx4_core 0000:16:00.0: missing VPD_STIN_END at offset 32769

Bjorn advised (thanks!) to look for what process is reading that VPD
data. In our case it is libvirtd, and enabling debugging in libvirtd
turned out a very interesting exercise, since it starts spewing
gabajillions of VPD errors, especially in the Intel 82580 data.

That igb data does not look corrupt when we revert the change mentioned
earlier, and we don’t see the packet loss either.

I’m not proficient in Kernel nor PCI internals, but a plausible
explanation is that incorrect handling of the returned data causes out-
of-bounds memory write, so this would mean a bug somewhere else, still
to be found. 

If this hypothesis is correct, there are security implications, since a
specifically crafted PCI firmware could elevate privileges to kernel
level. In all cases, it does not look sensible to return data that is
known to be incorrect.

-- 
Josselin MOUETTE
Infrastructure & Security architect
EXAION

[PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"

[Josselin Mouette]

When a device returns invalid VPD data, it can be misused by other
code paths in kernel space or user space, and there are instances
in which this seems to cause memory corruption.

There is no sensible reason why the kernel would provide userspace
or drivers with invalid and potentially dangerous data.

This reverts commit 5fe204eab174fd474227f23fd47faee4e7a6c000.
---
 drivers/pci/vpd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 485a642b9304..daaa208c9d9c 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -68,7 +68,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
                        if (pci_read_vpd_any(dev, off + 1, 2,
&header[1]) != 2) {
                                pci_warn(dev, "failed VPD read at
offset %zu\n",
                                         off + 1);
-                               return off ?: PCI_VPD_SZ_INVALID;
+                               return PCI_VPD_SZ_INVALID;
                        }
                        size = pci_vpd_lrdt_size(header);
                        if (off + size > PCI_VPD_MAX_SIZE)
@@ -87,13 +87,13 @@ static size_t pci_vpd_size(struct pci_dev *dev)
                                return off;
                }
        }
-       return off;
+       return PCI_VPD_SZ_INVALID;
 
 error:
        pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset
%zu%s\n",
                 header[0], size, off, off == 0 ?
                 "; assume missing optional EEPROM" : "");
-       return off ?: PCI_VPD_SZ_INVALID;
+       return PCI_VPD_SZ_INVALID;
 }
 
 static bool pci_vpd_available(struct pci_dev *dev, bool check_size)
-- 
2.39.2

[PATCH 2/2] Add better warnings about invalid VPD data

[Josselin Mouette]

Some Mellanox Connect-X 3 cards have firmware bugs which return
unfinished VPD data. This change helps to diagnose such issues
with clear warning messages.
---
 drivers/pci/vpd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index daaa208c9d9c..fc38a611dd3e 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -87,10 +87,11 @@ static size_t pci_vpd_size(struct pci_dev *dev)
                                return off;
                }
        }
+       pci_warn(dev, "missing VPD_STIN_END at offset %zu\n", off + 1);
        return PCI_VPD_SZ_INVALID;
 
 error:
-       pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset
%zu%s\n",
+       pci_warn(dev, "invalid VPD tag %#04x (size %zu) at offset
%zu%s\n",
                 header[0], size, off, off == 0 ?
                 "; assume missing optional EEPROM" : "");
        return PCI_VPD_SZ_INVALID;
-- 
2.39.2

[Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found)

[Josselin Mouette]

(resending since apparently UTF8 encoding didn't make it through VGER)

We've been observing a subtle kernel bug on a few servers after kernel
upgrades (starting from 5.15 and persisting in 6.8-rc1). The bug arises
only on machines with Mellanox Connect-X 3 cards and the symptom is
RabbitMQ disconnections caused by packet loss on the system Ethernet
card (Intel I350). Replacing the I350 by a 82580 produced the exact
same symptoms.

A bisect led to this change:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit
/?id=5fe204eab174fd474227f23fd47faee4e7a6c000

Reverting the patch and adding more warnings (patch follows) allowed us
to identify that the VPD data in the Connect-X 3 firmware is missing
VPD_STIN_END, which makes it return at a 32k offset. But I presume the
VPD data is incorrect far before that 32k limit.
[   43.854869] mlx4_core 0000:16:00.0: missing VPD_STIN_END at offset
32769

Bjorn advised (thanks!) to look for what process is reading that VPD
data. In our case it is libvirtd, and enabling debugging in libvirtd
turned out a very interesting exercise, since it starts spewing
gabajillions of VPD errors, especially in the Intel 82580 data.

That igb data does not look corrupt when we revert the change mentioned
earlier, and we don't see the packet loss either.

I'm not proficient in Kernel nor PCI internals, but a plausible
explanation is that incorrect handling of the returned data causes out-
of-bounds memory write, so this would mean a bug somewhere else, still
to be found.

If this hypothesis is correct, there are security implications, since a
specifically crafted PCI firmware could elevate privileges to kernel
level. In all cases, it does not look sensible to return data that is
known to be incorrect.

-- 
Josselin MOUETTE
Infrastructure & Security architect
EXAION

Re: [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"

[Bjorn Helgaas]

[+cc Hannes, who did a lot of related VPD work and reviewed the
original posting at
https://lore.kernel.org/r/20210715215959.2014576-6-helgaas@kernel.org/]

On Thu, Mar 07, 2024 at 05:09:27PM +0100, Josselin Mouette wrote:
> When a device returns invalid VPD data, it can be misused by other
> code paths in kernel space or user space, and there are instances
> in which this seems to cause memory corruption.

More of the background from Josselin at
https://lore.kernel.org/r/aaea0b30c35bb73b947727e4b3ec354d6b5c399c.camel@exaion.com

This is a regression, and obviously needs to be fixed somehow, but I'm
a bit hesitant to revert this until we understand the problem better.
If there's a memory corruption lurking and a revert hides the
corruption so we never fix it, I'm not sure that's an improvement
overall.

> There is no sensible reason why the kernel would provide userspace
> or drivers with invalid and potentially dangerous data.
> 
> This reverts commit 5fe204eab174fd474227f23fd47faee4e7a6c000.
> ---
>  drivers/pci/vpd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 485a642b9304..daaa208c9d9c 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -68,7 +68,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
>                         if (pci_read_vpd_any(dev, off + 1, 2,
> &header[1]) != 2) {
>                                 pci_warn(dev, "failed VPD read at
> offset %zu\n",
>                                          off + 1);
> -                               return off ?: PCI_VPD_SZ_INVALID;
> +                               return PCI_VPD_SZ_INVALID;
>                         }
>                         size = pci_vpd_lrdt_size(header);
>                         if (off + size > PCI_VPD_MAX_SIZE)
> @@ -87,13 +87,13 @@ static size_t pci_vpd_size(struct pci_dev *dev)
>                                 return off;
>                 }
>         }
> -       return off;
> +       return PCI_VPD_SZ_INVALID;
>  
>  error:
>         pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset
> %zu%s\n",
>                  header[0], size, off, off == 0 ?
>                  "; assume missing optional EEPROM" : "");
> -       return off ?: PCI_VPD_SZ_INVALID;
> +       return PCI_VPD_SZ_INVALID;
>  }
>  
>  static bool pci_vpd_available(struct pci_dev *dev, bool check_size)
> -- 
> 2.39.2
> 

Re: [Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found)

[Bjorn Helgaas]

[+cc Hannes]

[BTW, the patches are whitespace damaged, so they don't apply.  Looks
like tabs got converted to spaces]

On Thu, Mar 07, 2024 at 05:07:50PM +0100, Josselin Mouette wrote:
> We’ve been observing a subtle kernel bug on a few servers after kernel
> upgrades (starting from 5.15 and persisting in 6.8-rc1). The bug arises
> only on machines with Mellanox Connect-X 3 cards and the symptom is
> RabbitMQ disconnections caused by packet loss on the system Ethernet
> card (Intel I350). Replacing the I350 by a 82580 produced the exact
> same symptoms.

It looks like both I350 and 82580 use the igb driver?

> A bisect led to this change:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=5fe204eab174fd474227f23fd47faee4e7a6c000
> 
> Reverting the patch and adding more warnings (patch follows) allowed us
> to identify that the VPD data in the Connect-X 3 firmware is missing
> VPD_STIN_END, which makes it return at a 32k offset. But I presume the
> VPD data is incorrect far before that 32k limit.
> [   43.854869] mlx4_core 0000:16:00.0: missing VPD_STIN_END at offset 32769
> 
> Bjorn advised (thanks!) to look for what process is reading that VPD
> data. In our case it is libvirtd, and enabling debugging in libvirtd
> turned out a very interesting exercise, since it starts spewing
> gabajillions of VPD errors, especially in the Intel 82580 data.

Can we dig into these errors a bit?  I assume most of these come from
libvirtd (not the kernel)?

The VPD for different devices should be independent, so maybe an mlx4
VPD buffer overflow corrupted an igb VPD buffer, probably more likely
in libvirtd than in the kernel.

> That igb data does not look corrupt when we revert the change mentioned
> earlier, and we don’t see the packet loss either.

When you revert 5fe204eab174 ("PCI/VPD: Allow access to valid
parts of VPD if some is invalid"), you see no VPD errors either from
the kernel or from libvirtd except this one?

  mlx4_core 0000:16:00.0: missing VPD_STIN_END at offset 32769

> I’m not proficient in Kernel nor PCI internals, but a plausible
> explanation is that incorrect handling of the returned data causes out-
> of-bounds memory write, so this would mean a bug somewhere else, still
> to be found. 
> 
> If this hypothesis is correct, there are security implications, since a
> specifically crafted PCI firmware could elevate privileges to kernel
> level. In all cases, it does not look sensible to return data that is
> known to be incorrect.
> 
> -- 
> Josselin MOUETTE
> Infrastructure & Security architect
> EXAION
> 

Re: [Regression] [PCI/VPD] Possible memory corruption caused by invalid VPD data (commit found)

[Josselin Mouette]

Le jeudi 07 mars 2024 à 17:11 -0600, Bjorn Helgaas a écrit :
> [+cc Hannes]
> 
> [BTW, the patches are whitespace damaged, so they don't apply.  Looks
> like tabs got converted to spaces]

I’ll be damned for trying to interact here with corporate email, won’t
I? :) Sorry about that.

> On Thu, Mar 07, 2024 at 05:07:50PM +0100, Josselin Mouette wrote:
> > We’ve been observing a subtle kernel bug on a few servers after
> > kernel
> > upgrades (starting from 5.15 and persisting in 6.8-rc1). The bug
> > arises
> > only on machines with Mellanox Connect-X 3 cards and the symptom is
> > RabbitMQ disconnections caused by packet loss on the system
> > Ethernet
> > card (Intel I350). Replacing the I350 by a 82580 produced the exact
> > same symptoms.
> 
> It looks like both I350 and 82580 use the igb driver?

Yes indeed. I wanted to try hardware from another vendor entirely, but
we don’t have that in stock unfortunately.

> > Bjorn advised (thanks!) to look for what process is reading that
> > VPD
> > data. In our case it is libvirtd, and enabling debugging in
> > libvirtd
> > turned out a very interesting exercise, since it starts spewing
> > gabajillions of VPD errors, especially in the Intel 82580 data.
> 
> Can we dig into these errors a bit?  I assume most of these come from
> libvirtd (not the kernel)?

Yes, it’s libvirtd that does the parsing. 

virPCIDeviceNew:1496 : 15b3 1003 0000:16:00.0: initialized
 → That’s the Connect-X 3
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x4
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x4
 (several thousands of these)
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0
 (tens of thousands of those)
debug : virPCIVPDParse:748 : Encountered an invalid VPD: does not have a VPD-R record
virPCIDeviceFree:1526 : 15b3 1003 0000:16:00.0: freeing

Then it tries reading again the Connect-X 3 VPD a couple times, before
giving up.

Then we reach the 82350:
virPCIDeviceNew:1496 : 8086 150e 0000:86:00.1: initialized
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x5
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x8
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0xa
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x5
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x8
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0xc
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0xe
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0xc
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x4
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x4
virPCIVPDParse:734 : Encountered an unexpected VPD resource tag: 0x7
virPCIVPDResourceGetKeywordPrefix:70 : internal error: The keyword is not comprised only of uppercase ASCII letters or digits
virPCIVPDParseVPDLargeResourceFields:517 : Could not determine a field value format for keyword: ^KN
virPCIVPDResourceGetKeywordPrefix:70 : internal error: The keyword is not comprised only of uppercase ASCII letters or digits
virPCIVPDParseVPDLargeResourceFields:517 : Could not determine a field value format for keyword: 0^E
virPCIVPDParseVPDLargeResourceFields:529 : internal error: A field data length violates the resource length boundary.
virPCIVPDParse:740 : Encountered an invalid VPD
virPCIDeviceFree:1526 : 8086 150e 0000:86:00.1: freeing

(Multiply by the number of 82350 ports and the number of attempts.)

> The VPD for different devices should be independent, so maybe an mlx4
> VPD buffer overflow corrupted an igb VPD buffer, probably more likely
> in libvirtd than in the kernel.

… or maybe not, see later.

> > That igb data does not look corrupt when we revert the change
> > mentioned
> > earlier, and we don’t see the packet loss either.
> 
> When you revert 5fe204eab174 ("PCI/VPD: Allow access to valid
> parts of VPD if some is invalid"), you see no VPD errors either from
> the kernel or from libvirtd except this one?
> 
>   mlx4_core 0000:16:00.0: missing VPD_STIN_END at offset 32769

We do have these as well, nothing new:
pci 0000:1b:00.0: [Firmware Bug]: disabling VPD access (can't determine size of non-standard VPD format)
(This is a LSI Megaraid controller, which thankfully doesn’t seem to
cause any havoc.)

But actually with patch 0002 we also get these:
igb 0000:86:00.0: invalid VPD tag 0xff (size 65535) at offset 132

So we would not have one, but TWO buggy pieces of firmware here. Which
provides a perfect explanation for why the igb warnings disappear as
well. 

This, plus the insane size of the libvirtd logs (we’re talking millions
of lines when it loops over invalid VPD data, over and over again),
leads me to another hypothesis: libvirtd could spend so much time
parsing VPD data it will actually fail to handle network packets in
time before a timeout; timeouts being low in the AMQP protocol.

Thanks again for pushing me in the right direction. I hope we’re onto
something.

-- 
Josselin MOUETTE
Infrastructure & security architect
EXAION

[PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"

[Josselin Mouette]

(Sorry about the resend, this time with correct patch formatting.)

When a device returns invalid VPD data, it can be misused by other
code paths in kernel space or user space, and there are instances
in which this seems to cause memory corruption.

There is no sensible reason why the kernel would provide userspace
or drivers with invalid and potentially dangerous data.

This reverts commit 5fe204eab174fd474227f23fd47faee4e7a6c000.
---
 drivers/pci/vpd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 485a642b9304..daaa208c9d9c 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -68,7 +68,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 			if (pci_read_vpd_any(dev, off + 1, 2, &header[1]) != 2) {
 				pci_warn(dev, "failed VPD read at offset %zu\n",
 					 off + 1);
-				return off ?: PCI_VPD_SZ_INVALID;
+				return PCI_VPD_SZ_INVALID;
 			}
 			size = pci_vpd_lrdt_size(header);
 			if (off + size > PCI_VPD_MAX_SIZE)
@@ -87,13 +87,13 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 				return off;
 		}
 	}
-	return off;
+	return PCI_VPD_SZ_INVALID;
 
 error:
 	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
 		 header[0], size, off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
-	return off ?: PCI_VPD_SZ_INVALID;
+	return PCI_VPD_SZ_INVALID;
 }
 
 static bool pci_vpd_available(struct pci_dev *dev, bool check_size)
-- 
2.39.2

[PATCH 2/2] Add better warnings about invalid VPD data

[Josselin Mouette]

Some Mellanox Connect-X 3 cards have firmware bugs which return
unfinished VPD data. This change helps to diagnose such issues
with clear warning messages.
---
 drivers/pci/vpd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index daaa208c9d9c..fc38a611dd3e 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -87,10 +87,11 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 				return off;
 		}
 	}
+	pci_warn(dev, "missing VPD_STIN_END at offset %zu\n", off + 1);
 	return PCI_VPD_SZ_INVALID;
 
 error:
-	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
+	pci_warn(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
 		 header[0], size, off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
 	return PCI_VPD_SZ_INVALID;
-- 
2.39.2

Re: [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"

[Bjorn Helgaas]

On Thu, Mar 07, 2024 at 04:36:06PM -0600, Bjorn Helgaas wrote:
> [+cc Hannes, who did a lot of related VPD work and reviewed the
> original posting at
> https://lore.kernel.org/r/20210715215959.2014576-6-helgaas@kernel.org/]
> 
> On Thu, Mar 07, 2024 at 05:09:27PM +0100, Josselin Mouette wrote:
> > When a device returns invalid VPD data, it can be misused by other
> > code paths in kernel space or user space, and there are instances
> > in which this seems to cause memory corruption.
> 
> More of the background from Josselin at
> https://lore.kernel.org/r/aaea0b30c35bb73b947727e4b3ec354d6b5c399c.camel@exaion.com
> 
> This is a regression, and obviously needs to be fixed somehow, but I'm
> a bit hesitant to revert this until we understand the problem better.
> If there's a memory corruption lurking and a revert hides the
> corruption so we never fix it, I'm not sure that's an improvement
> overall.

I don't want to drop this, but we're kind of stuck here:

  - I'd still like to understand the problem better.

  - Trivially, I can't apply patches lacking the appropriate
    signed-off-by; see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.6#n396

> > There is no sensible reason why the kernel would provide userspace
> > or drivers with invalid and potentially dangerous data.
> > 
> > This reverts commit 5fe204eab174fd474227f23fd47faee4e7a6c000.
> > ---
> >  drivers/pci/vpd.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index 485a642b9304..daaa208c9d9c 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -68,7 +68,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
> >                         if (pci_read_vpd_any(dev, off + 1, 2,
> > &header[1]) != 2) {
> >                                 pci_warn(dev, "failed VPD read at
> > offset %zu\n",
> >                                          off + 1);
> > -                               return off ?: PCI_VPD_SZ_INVALID;
> > +                               return PCI_VPD_SZ_INVALID;
> >                         }
> >                         size = pci_vpd_lrdt_size(header);
> >                         if (off + size > PCI_VPD_MAX_SIZE)
> > @@ -87,13 +87,13 @@ static size_t pci_vpd_size(struct pci_dev *dev)
> >                                 return off;
> >                 }
> >         }
> > -       return off;
> > +       return PCI_VPD_SZ_INVALID;
> >  
> >  error:
> >         pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset
> > %zu%s\n",
> >                  header[0], size, off, off == 0 ?
> >                  "; assume missing optional EEPROM" : "");
> > -       return off ?: PCI_VPD_SZ_INVALID;
> > +       return PCI_VPD_SZ_INVALID;
> >  }
> >  
> >  static bool pci_vpd_available(struct pci_dev *dev, bool check_size)
> > -- 
> > 2.39.2
> > 

Re: [PATCH 1/2] Revert "PCI/VPD: Allow access to valid parts of VPD if some is invalid"

[Hannes Reinecke]

On 5/3/24 00:23, Bjorn Helgaas wrote:
> On Thu, Mar 07, 2024 at 04:36:06PM -0600, Bjorn Helgaas wrote:
>> [+cc Hannes, who did a lot of related VPD work and reviewed the
>> original posting at
>> https://lore.kernel.org/r/20210715215959.2014576-6-helgaas@kernel.org/]
>>
>> On Thu, Mar 07, 2024 at 05:09:27PM +0100, Josselin Mouette wrote:
>>> When a device returns invalid VPD data, it can be misused by other
>>> code paths in kernel space or user space, and there are instances
>>> in which this seems to cause memory corruption.
>>
>> More of the background from Josselin at
>> https://lore.kernel.org/r/aaea0b30c35bb73b947727e4b3ec354d6b5c399c.camel@exaion.com
>>
>> This is a regression, and obviously needs to be fixed somehow, but I'm
>> a bit hesitant to revert this until we understand the problem better.
>> If there's a memory corruption lurking and a revert hides the
>> corruption so we never fix it, I'm not sure that's an improvement
>> overall.
> 
> I don't want to drop this, but we're kind of stuck here:
> 
>    - I'd still like to understand the problem better.
> 
>    - Trivially, I can't apply patches lacking the appropriate
>      signed-off-by; see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.6#n396
> 
Right. Again.

So: Problem is that VPD data is a self-describing stream of serialized 
TLV records. The records themselves have no internal consistency check, 
so any data read is assumed to be correct. If there is an error during 
decoding we really should not continue, as everything beyond that error 
cannot be relied on.

However, the assumption was that the read data is correct, so in getting
an error the very assumption leading us to interpret the data as VPD 
records is invalid, too.

But question remains: was the data correct until the error?
Or was the error earlier, and we just mis-interpreted invalid data?

In my original patch I used the second attempt, as this will pretty
much guarantee that all VPD data will be correct. It has the drawback,
though, that for some cards we won't be able to read the VPD data, even
if they would provide valid data until the error.

As there is no good way out of this I'd rather see a blacklist of cards
which _can_ be read despite the error, and don't return VPD data otherwise.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich