Message-ID: <20241105001027.GA1447341@bhelgaas>
Date: Mon, 4 Nov 2024 18:10:27 -0600
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 Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> > 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.
> 
> As it is seen through lspci, nothing criminal here.
> 08:00.0 Ethernet controller: Mellanox Technologies MT2910 Family [ConnectX-7]
> ...
>         Capabilities: [48] Vital Product Data
>                 Product Name: NVIDIA ConnectX-7 HHHL adapter Card, 200GbE / NDR200 IB, Dual-port QSFP112, PCIe 5.0 x16 with x16 PCIe extension option, Crypto, Secure Boot Capable
>                 Read-only fields:
>                         [PN] Part number: MCX713106AEHEA_QP1
>                         [EC] Engineering changes: A5
>                         [V2] Vendor specific: MCX713106AEHEA_QP1
>                         [SN] Serial number: MT2314XZ0JUZ
>                         [V3] Vendor specific: 0a5efb8958deed118000946dae7db798
>                         [VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A
>                         [V0] Vendor specific: PCIeGen5 x16
>                         [VU] Vendor specific: MT2314XZ0JUZMLNXS0D0F0
>                         [RV] Reserved: checksum good, 1 byte(s) reserved
>                 End
> 
> > > > 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.
> 
> Agree that it OS specific, but for me, the fields like manufacturer
> ID, serial number e.t.c shows that the VPD doesn't contain sensitive
> information.

I don't follow the reasoning that because these fields don't seem
sensitive, other fields won't be :)

> > > > 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.
> 
> The patch yes, but the flow is tested very well. It is hard to imagine
> situation where "lspci -vv" or corresponding library, never used to read
> data from device. Maybe it is not used daily on all computers, but all
> devices at least once in their lifetime were accessed.

Well, true, but I think "lspci -vv" requires root privilege to read
the VPD data, doesn't it?

> > > 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.
> 
> They hide it as dmesg already exposes a lot of sensitive data. For
> example, the kernel panic reveals a lot of such data. It is
> definitely not the case for VPD, and VPD vs. dmesg comparison is not
> correct one.

dmidecode is another similar case, which is also not public.

What's the use case?  How does an unprivileged user use the VPD
information?

I can certainly imagine using VPD for bug reporting, but that would
typically involve dmesg, dmidecode, lspci -vv, etc, all of which
already require privilege, so it's not clear to me how public VPD info
would help in that scenario.

Bjorn

Powered by blists - more mailing lists