Re: [PATCH] PCI/sysfs: Fix read permissions for VPD attributes

[Leon Romanovsky <leon@kernel.org>]

On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> [+cc LKML etc, should PCI VPD be readable by non-root?]
> 
> On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > The Virtual Product Data (VPD) attribute is not readable by regular
> 
> ("Vital Product Data" is the term in the spec)

Ohh, sorry for the confusion. I relied to much on auto-completion.

> 
> > > user without root permissions. Such restriction is not really needed,
> > > as data presented in that VPD is not sensitive at all.
> > > 
> > > This change aligns the permissions of the VPD attribute to be accessible
> > > for read by all users, while write being restricted to root only.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Applied to pci/vpd for v6.13, thanks!
> 
> I think this deserves a little more consideration than I gave it
> initially.
> 
> Obviously somebody is interested in using this; can we include some
> examples so we know there's an actual user?

I'll provide it after the weekend.

> 
> Are we confident that VPD never contains anything sensitive?  It may
> contain arbitrary vendor-specific information, so we can't know what
> might be in that part.

It depends on the vendor, but I'm pretty confident that any sane vendor who
read the PCI spec will not put sensitive information in the VPD. The
spec is very clear that this open to everyone.

> 
> Reading VPD is fairly complicated and we've had problems in the past
> (we have quirk_blacklist_vpd() for devices that behave
> "unpredictably"), so it's worth considering whether allowing non-root
> to do this could be exploited or could allow DOS attacks.

It is not different from any other PCI field. If you are afraid of DOS,
you should limit to read all other fields too.

> 
> For reference, here are the fields defined in PCIe r6.0, sec 6.27.2
> (although VPD can contain anything a manufacturer wants to put there):
> 
>   PN Add-in Card Part Number
>   EC Engineering Change Level of the Add-in Card
>   FG Fabric Geography
>   LC Location
>   MN Manufacture ID
>   PG PCI Geography
>   SN Serial Number
>   TR Thermal Reporting
>   Vx Vendor Specific
>   CP Extended Capability
>   RV Checksum and Reserved
>   FF Form Factor
>   Yx System Specific
>   YA Asset Tag Identifier
>   RW Remaining Read/Write Area
> 
> The Conventional PCI spec, r3.0, sec 6.4, says:
> 
>   Vital Product Data (VPD) is the information that uniquely defines
>   items such as the hardware, software, and microcode elements of a
>   system. The VPD provides the system with information on various FRUs
>   (Field Replaceable Unit) including Part Number, Serial Number, and
>   other detailed information. VPD also provides a mechanism for
>   storing information such as performance and failure data on the
>   device being monitored. The objective, from a system point of view,
>   is to collect this information by reading it from the hardware,
>   software, and microcode components.
> 
> Some of that, e.g., performance and failure data, might be considered
> sensitive in some environments.

I'm enabling it for modern device which is compliant to PCI spec v6.0.
Do you want me to add quirk_allow_vpd() to allow only specific devices to
read that field?a It is doable but not scalable.

> 
> > > ---
> > > I added stable@ as it was discovered during our hardware ennoblement
> > > and it is important to be picked by distributions too.
> > > ---
> > >  drivers/pci/vpd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > > index e4300f5f304f..2537685cac90 100644
> > > --- a/drivers/pci/vpd.c
> > > +++ b/drivers/pci/vpd.c
> > > @@ -317,7 +317,7 @@ static ssize_t vpd_write(struct file *filp, struct kobject *kobj,
> > >  
> > >  	return ret;
> > >  }
> > > -static BIN_ATTR(vpd, 0600, vpd_read, vpd_write, 0);
> > > +static BIN_ATTR_RW(vpd, 0);
> > >  
> > >  static struct bin_attribute *vpd_attrs[] = {
> > >  	&bin_attr_vpd,
> > > -- 
> > > 2.46.2
> > >