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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241031232252.GA1268406@bhelgaas>
Date: Thu, 31 Oct 2024 18:22:52 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Leon Romanovsky <leon@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Leon Romanovsky <leonro@...dia.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

[+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@...dia.com>
> > 
> > The Virtual Product Data (VPD) attribute is not readable by regular

("Vital Product Data" is the term in the spec)

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

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.

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.

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