Message-ID: <20241101164737.GA1308861@bhelgaas>
Date: Fri, 1 Nov 2024 11:47:37 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Leon Romanovsky <leon@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Wilczyński <kw@...ux.com>,
	linux-pci@...r.kernel.org, Ariel Almog <ariela@...dia.com>,
	Aditya Prabhune <aprabhune@...dia.com>,
	Hannes Reinecke <hare@...e.de>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Arun Easi <aeasi@...vell.com>, Jonathan Chocron <jonnyc@...zon.com>,
	Bert Kenward <bkenward@...arflare.com>,
	Matt Carlson <mcarlson@...adcom.com>,
	Kai-Heng Feng <kai.heng.feng@...onical.com>,
	Jean Delvare <jdelvare@...e.de>,
	Alex Williamson <alex.williamson@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI/sysfs: Fix read permissions for VPD attributes

On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > 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@...dia.com>
> > > > 
> > > > The Virtual Product Data (VPD) attribute is not readable by regular
> > > > 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@...r.kernel.org
> > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > Signed-off-by: Leon Romanovsky <leonro@...dia.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.

I don't think the spec really defines "everyone" in this context, does
it?  The concept of privileged vs unprivileged users is an OS
construct, not really something the PCIe spec covers.

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

Reading VPD is much different than reading things from config space.

To read VPD, software needs to:

  - Mutex with any other read/write path

  - Write the VPD address to read to the VPD Address register, with F
    bit clear

  - Wait (with timeout) for hardware to set the F bit of VPD Address
    register

  - Read VPD information from the VPD Data register

  - Repeat as necessary

The address is 15 bits wide, so there may be up to 32KB of VPD data.
The only way to determine the actual length is to read the data and
parse the data items, which is vulnerable to corrupted EEPROMs and
hardware issues if we read beyond the implemented size.

The PCI core currently doesn't touch VPD until a driver or userspace
(via sysfs) reads or writes it, so this path is not tested on most
devices.

> > 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? It is doable but not scalable.

None of these questions really has to do with old vs new devices.  An
"allow-list" quirk is possible, but I agree it would be a maintenance
headache.  To me it feels like VPD is kind of in the same category as
dmesg logs.  We try to avoid putting secret stuff in dmesg, but
generally distros still don't make it completely public.

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

Powered by blists - more mailing lists