On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
> 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 :)
It was not my best argument :)
>
> > > > > 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?
The point was different, I argued that VPD flow is tested thoroughly.
BTW, from my experience with testing in HW companies, they run all tests
as root.
>
> > > > 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?
We have to add new field keyword=value in VA section of VPD, which will
indicate very specific sub-model for devices used as a bridge.
>
> 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.
I'm targeting other scenario - monitoring tool, which doesn't need root
permissions for reading data. It needs to distinguish between NIC sub-models.
Thanks
>
> Bjorn
