Message-ID: <20250225165746.GH53094@unreal>
Date: Tue, 25 Feb 2025 18:57:46 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: 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, netdev@...r.kernel.org,
	Jakub Kicinski <kuba@...nel.org>,
	Thomas Weißschuh <linux@...ssschuh.net>,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

On Tue, Feb 25, 2025 at 10:05:42AM -0600, Bjorn Helgaas wrote:
> On Sun, Jan 19, 2025 at 09:27:54AM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@...dia.com>
> > 
> > The Vital Product Data (VPD) attribute is not readable by regular
> > user without root permissions. Such restriction is not needed at
> > all for Mellanox devices, as data presented in that VPD is not
> > sensitive and access to the HW is safe and well tested.
> > 
> > This change changes the permissions of the VPD attribute to be accessible
> > for read by all users for Mellanox devices, while write continue to be
> > restricted to root only.
> > 
> > The main use case is to remove need to have root/setuid permissions
> > while using monitoring library [1].
> 
> As far as I can tell, this is basically a device identification
> problem, which would be better handled by the Vendor, Device, and
> Revision IDs.  If that would solve the problem, it would also make
> standard unprivileged lspci output more specific.

Yes, unfortunately these devices have same IDs as "regular" NICs and the
difference in some FW configuration.

> 
> VPD has never been user readable, so I assume you have some existing
> method for device identification?

We always read VPD by using "sudo ..." command, until one of our customers
requested to provide a way to run monitoring library without any root access.
It runs on hypervisor and being non-root there is super important for them.

> 
> Other concerns raised in previous threads include:
> 
>   - Potential for sensitive information in VPD, similar to dmesg and
>     dmidecode
> 
>   - Kernel complexity of reading VPD (mutex, address/data registers)
> 
>   - Performance and potential denial of service as a consequence of
>     mutex and hardware interaction
> 
>   - Missing EEPROMs or defective or incompletely-installed firmware
>     breaking VPD read
> 
>   - Broken devices that crash when VPD is read

This patch allows non-root read for Mellanox (NICs) devices only and
such access is going to be used only once during library initiation
flow. So nothing from above is applicable in our case.

In general case, all devices in the world were accessed at least once
with "sudo lspci ....", during their bringup, installation, daily use
e.t.c. Broken devices are filtered by kernel and have limited access
to VPD.

So if it is broken, it will be broken with sudo too.

> 
>   - Potential for issues with future Mellanox devices, even though all
>     current ones work fine

It is not different from any other feature. MLNX devices exist for more
than 25 years already and we never exposed anything sensitive through VPD.

I'm confident that we have no plans to change this policy in the future
either.

> 
> This is basically similar to mmapping a device BAR, for which we also
> require root.

It is kernel controlled exposure, through well defined sysfs file and
in-kernel API for very specific PCI section. Device BAR is much more
than that.

Thanks

> 
> > [leonro@vm ~]$ lspci |grep nox
> > 00:09.0 Ethernet controller: Mellanox Technologies MT2910 Family [ConnectX-7]
> > 
> > Before:
> > [leonro@vm ~]$ ls -al /sys/bus/pci/devices/0000:00:09.0/vpd
> > -rw------- 1 root root 0 Nov 13 12:30 /sys/bus/pci/devices/0000:00:09.0/vpd
> > After:
> > [leonro@vm ~]$ ls -al /sys/bus/pci/devices/0000:00:09.0/vpd
> > -rw-r--r-- 1 root root 0 Nov 13 12:30 /sys/bus/pci/devices/0000:00:09.0/vpd
> > 
> > [1] https://developer.nvidia.com/management-library-nvml
> > Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> > ---
> > Changelog:
> > v4:
> >  * Change comment to the variant suggested by Stephen
> > v3: https://lore.kernel.org/all/18f36b3cbe2b7e67eed876337f8ba85afbc12e73.1733227737.git.leon@kernel.org
> >  * Used | to change file attributes
> >  * Remove WARN_ON
> > v2: https://lore.kernel.org/all/61a0fa74461c15edfae76222522fa445c28bec34.1731502431.git.leon@kernel.org
> >  * Another implementation to make sure that user is presented with
> >    correct permissions without need for driver intervention.
> > v1: https://lore.kernel.org/all/cover.1731005223.git.leonro@nvidia.com
> >  * Changed implementation from open-read-to-everyone to be opt-in
> >  * Removed stable and Fixes tags, as it seems like feature now.
> > v0: https://lore.kernel.org/all/65791906154e3e5ea12ea49127cf7c707325ca56.1730102428.git.leonro@nvidia.com/
> > ---
> >  drivers/pci/vpd.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index a469bcbc0da7..c873ab47526b 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -332,6 +332,13 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
> >  	if (!pdev->vpd.cap)
> >  		return 0;
> >  
> > +	/*
> > +	 * On Mellanox devices reading VPD is safe for unprivileged users,
> > +	 * so just add needed bits to allow read.
> > +	 */
> > +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> > +		return a->attr.mode | 0044;
> > +
> >  	return a->attr.mode;
> >  }
> >  
> > -- 
> > 2.47.1
> > 

Powered by blists - more mailing lists