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@xxxxxxxxxx>
> > > > > >
> > > > > > 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@xxxxxxxxxxxxxxx
> > > > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > > > >
> > > > > 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
