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

[Bjorn Helgaas <helgaas@kernel.org>]

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