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