[PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.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].

[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@nvidia.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

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Bjorn Helgaas]

On Sun, Jan 19, 2025 at 09:27:54AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.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.

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

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

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

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

> [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@nvidia.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
> 

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

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@nvidia.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@nvidia.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
> > 

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Andrew Lunn]

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

You can chmod files in sys. So the administrator can change the
permissions, and then non-root users can access it.

This seems a more scalable solution that adding a special case in the
kernel.

	Andrew

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

On Tue, Feb 25, 2025, at 19:30, Andrew Lunn wrote:
>> 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.
>
> You can chmod files in sys. So the administrator can change the
> permissions, and then non-root users can access it.
>
> This seems a more scalable solution that adding a special case in the
> kernel.

Special case is an outcome of discussion in previous versions. My initial patch which I believe is the right approach is to allow non-root read access to VPD for everyone.

Chmod solution is something that I thought, but for now I'm looking for the out of the box solution. Chmod still require from administrator to run   scripts with root permissions.

Thanks 

>
> 	Andrew

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Andrew Lunn]

> Chmod solution is something that I thought, but for now I'm looking
> for the out of the box solution. Chmod still require from
> administrator to run scripts with root permissions.

It is more likely to be a udev rule. systemd already has lots of
examples:

/lib/udev/rules.d/50-udev-default.rules:KERNEL=="rfkill", MODE="0664"

	Andrew

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

On Tue, Feb 25, 2025, at 20:59, Andrew Lunn wrote:
>> Chmod solution is something that I thought, but for now I'm looking
>> for the out of the box solution. Chmod still require from
>> administrator to run scripts with root permissions.
>
> It is more likely to be a udev rule. 

Udev rule is one of the ways to run such script.

systemd already has lots of
> examples:
>
> /lib/udev/rules.d/50-udev-default.rules:KERNEL=="rfkill", MODE="0664"
>
> 	Andrew

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Bjorn Helgaas]

On Tue, Feb 25, 2025 at 10:05:49PM +0200, Leon Romanovsky wrote:
> On Tue, Feb 25, 2025, at 20:59, Andrew Lunn wrote:
> >> Chmod solution is something that I thought, but for now I'm looking
> >> for the out of the box solution. Chmod still require from
> >> administrator to run scripts with root permissions.
> >
> > It is more likely to be a udev rule. 
> 
> Udev rule is one of the ways to run such script.
> 
> > systemd already has lots of examples:
> >
> > /lib/udev/rules.d/50-udev-default.rules:KERNEL=="rfkill", MODE="0664"

Where are we at with this?  Is a udev rule a feasible solution?

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

On Mon, Mar 03, 2025 at 03:17:55PM -0600, Bjorn Helgaas wrote:
> On Tue, Feb 25, 2025 at 10:05:49PM +0200, Leon Romanovsky wrote:
> > On Tue, Feb 25, 2025, at 20:59, Andrew Lunn wrote:
> > >> Chmod solution is something that I thought, but for now I'm looking
> > >> for the out of the box solution. Chmod still require from
> > >> administrator to run scripts with root permissions.
> > >
> > > It is more likely to be a udev rule. 
> > 
> > Udev rule is one of the ways to run such script.
> > 
> > > systemd already has lots of examples:
> > >
> > > /lib/udev/rules.d/50-udev-default.rules:KERNEL=="rfkill", MODE="0664"
> 
> Where are we at with this?  Is a udev rule a feasible solution?

We asked customer if this can work for him and still didn't get answer.
I don't know if they have systemd/udev in their hypervisors.

Thanks

Re: [PATCH v4] PCI/sysfs: Change read permissions for VPD attributes

[Leon Romanovsky]

On Tue, Mar 04, 2025 at 09:45:12AM +0200, Leon Romanovsky wrote:
> On Mon, Mar 03, 2025 at 03:17:55PM -0600, Bjorn Helgaas wrote:
> > On Tue, Feb 25, 2025 at 10:05:49PM +0200, Leon Romanovsky wrote:
> > > On Tue, Feb 25, 2025, at 20:59, Andrew Lunn wrote:
> > > >> Chmod solution is something that I thought, but for now I'm looking
> > > >> for the out of the box solution. Chmod still require from
> > > >> administrator to run scripts with root permissions.
> > > >
> > > > It is more likely to be a udev rule. 
> > > 
> > > Udev rule is one of the ways to run such script.
> > > 
> > > > systemd already has lots of examples:
> > > >
> > > > /lib/udev/rules.d/50-udev-default.rules:KERNEL=="rfkill", MODE="0664"
> > 
> > Where are we at with this?  Is a udev rule a feasible solution?
> 
> We asked customer if this can work for him and still didn't get answer.
> I don't know if they have systemd/udev in their hypervisors.

We still didn't get any update, so let's drop this patch for now.

Thanks

> 
> Thanks
>