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

Commit Message

Leon Romanovsky Nov. 13, 2024, 12:59 p.m. UTC

From: Leon Romanovsky <[e-mail]>

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 <[e-mail]>
---
Changelog:
v2:
 * Another implementation to make sure that user is presented with
   correct permissions without need for driver intervention.
v1: https://lore.kernel.org/all/[e-mail]
 * 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/[e-mail]/
---
 drivers/pci/vpd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Jean Delvare Nov. 21, 2024, 12:01 p.m. UTC | #1

Hi Leon,

On Wed, 13 Nov 2024 14:59:58 +0200, Leon Romanovsky wrote:
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -332,6 +332,14 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
>  	if (!pdev->vpd.cap)
>  		return 0;
>  
> +	/*
> +	 * Mellanox devices have implementation that allows VPD read by
> +	 * unprivileged users, so just add needed bits to allow read.
> +	 */
> +	WARN_ON_ONCE(a->attr.mode != 0600);
> +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> +		return a->attr.mode + 0044;

When manipulating bitfields, | is preferred. This would make the
operation safe regardless of the initial value, so you can even get rid
of the WARN_ON_ONCE() above.

> +
>  	return a->attr.mode;
>  }
>

Leon Romanovsky Nov. 21, 2024, 12:13 p.m. UTC | #2

On Thu, Nov 21, 2024 at 01:01:27PM +0100, Jean Delvare wrote:
> Hi Leon,
> 
> On Wed, 13 Nov 2024 14:59:58 +0200, Leon Romanovsky wrote:
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -332,6 +332,14 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
> >  	if (!pdev->vpd.cap)
> >  		return 0;
> >  
> > +	/*
> > +	 * Mellanox devices have implementation that allows VPD read by
> > +	 * unprivileged users, so just add needed bits to allow read.
> > +	 */
> > +	WARN_ON_ONCE(a->attr.mode != 0600);
> > +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> > +		return a->attr.mode + 0044;
> 
> When manipulating bitfields, | is preferred. This would make the
> operation safe regardless of the initial value, so you can even get rid
> of the WARN_ON_ONCE() above.

The WARN_ON_ONCE() is intended to catch future changes in VPD sysfs
attributes. My intention is that once that WARN will trigger, the
author will be forced to reevaluate the latter if ( ... PCI_VENDOR_ID_MELLANOX)
condition and maybe we won't need it anymore. Without WARN_ON_ONCE, it
is easy to miss that code.

I still didn't lost hope that at some point VPD will be open for read to
all kernel devices.

Bjorn, are you ok with this patch? If yes, I'll resend the patch with
the suggested change after the merge window.

Thanks

> 
> > +
> >  	return a->attr.mode;
> >  }
> >  
> 
> -- 
> Jean Delvare
> SUSE L3 Support
>

Jean Delvare Nov. 21, 2024, 2:11 p.m. UTC | #3

On Thu, 21 Nov 2024 14:13:01 +0200, Leon Romanovsky wrote:
> On Thu, Nov 21, 2024 at 01:01:27PM +0100, Jean Delvare wrote:
> > On Wed, 13 Nov 2024 14:59:58 +0200, Leon Romanovsky wrote:  
> > > --- a/drivers/pci/vpd.c
> > > +++ b/drivers/pci/vpd.c
> > > @@ -332,6 +332,14 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
> > >  	if (!pdev->vpd.cap)
> > >  		return 0;
> > >  
> > > +	/*
> > > +	 * Mellanox devices have implementation that allows VPD read by
> > > +	 * unprivileged users, so just add needed bits to allow read.
> > > +	 */
> > > +	WARN_ON_ONCE(a->attr.mode != 0600);
> > > +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> > > +		return a->attr.mode + 0044;  
> > 
> > When manipulating bitfields, | is preferred. This would make the
> > operation safe regardless of the initial value, so you can even get rid
> > of the WARN_ON_ONCE() above.  
> 
> The WARN_ON_ONCE() is intended to catch future changes in VPD sysfs
> attributes. My intention is that once that WARN will trigger, the
> author will be forced to reevaluate the latter if ( ... PCI_VENDOR_ID_MELLANOX)
> condition and maybe we won't need it anymore. Without WARN_ON_ONCE, it
> is easy to miss that code.

The default permissions are 10 lines above in the same file. Doesn't
seem that easy to miss to me.

In my opinion, WARN_ON should be limited to cases where something really
bad has happened. It's not supposed to be a reminder for developers to
perform some code clean-up. Remember that WARN_ON has a run-time cost
and it could be evaluated for a possibly large number of PCI devices
(although admittedly VPD support seems to be present only in a limited
number of PCI device).

Assuming you properly use | instead of +, then nothing bad will happen
if the default permissions change, the code will simply become a no-op,
until someone notices and deletes it. No harm done.

I'm not maintaining this part of the kernel so I can't speak or decide
on behalf of the maintainers, but in my opinion, if you really want to
leave a note for future developers, then a comment in the source code
is a better way, as it has no run-time cost, and will also be found
earlier by the developers (no need for run-time testing).

Thanks,

Leon Romanovsky Nov. 21, 2024, 5 p.m. UTC | #4

On Thu, Nov 21, 2024 at 03:11:16PM +0100, Jean Delvare wrote:
> On Thu, 21 Nov 2024 14:13:01 +0200, Leon Romanovsky wrote:
> > On Thu, Nov 21, 2024 at 01:01:27PM +0100, Jean Delvare wrote:
> > > On Wed, 13 Nov 2024 14:59:58 +0200, Leon Romanovsky wrote:  
> > > > --- a/drivers/pci/vpd.c
> > > > +++ b/drivers/pci/vpd.c
> > > > @@ -332,6 +332,14 @@ static umode_t vpd_attr_is_visible(struct kobject *kobj,
> > > >  	if (!pdev->vpd.cap)
> > > >  		return 0;
> > > >  
> > > > +	/*
> > > > +	 * Mellanox devices have implementation that allows VPD read by
> > > > +	 * unprivileged users, so just add needed bits to allow read.
> > > > +	 */
> > > > +	WARN_ON_ONCE(a->attr.mode != 0600);
> > > > +	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
> > > > +		return a->attr.mode + 0044;  
> > > 
> > > When manipulating bitfields, | is preferred. This would make the
> > > operation safe regardless of the initial value, so you can even get rid
> > > of the WARN_ON_ONCE() above.  
> > 
> > The WARN_ON_ONCE() is intended to catch future changes in VPD sysfs
> > attributes. My intention is that once that WARN will trigger, the
> > author will be forced to reevaluate the latter if ( ... PCI_VENDOR_ID_MELLANOX)
> > condition and maybe we won't need it anymore. Without WARN_ON_ONCE, it
> > is easy to miss that code.
> 
> The default permissions are 10 lines above in the same file. Doesn't
> seem that easy to miss to me.
> 
> In my opinion, WARN_ON should be limited to cases where something really
> bad has happened. It's not supposed to be a reminder for developers to
> perform some code clean-up. Remember that WARN_ON has a run-time cost
> and it could be evaluated for a possibly large number of PCI devices
> (although admittedly VPD support seems to be present only in a limited
> number of PCI device).

Sorry about which run-time cost are you referring? This is slow path and
extra if() inside WARN_ON which has unlikely keyword, makes no difference
when accessing HW.

In addition, this check is for devices which already known to have VPD
(see pdev->vpd.cap check above).

> 
> Assuming you properly use | instead of +, then nothing bad will happen
> if the default permissions change, the code will simply become a no-op,
> until someone notices and deletes it. No harm done.
> 
> I'm not maintaining this part of the kernel so I can't speak or decide
> on behalf of the maintainers, but in my opinion, if you really want to
> leave a note for future developers, then a comment in the source code
> is a better way, as it has no run-time cost, and will also be found
> earlier by the developers (no need for run-time testing).

I don't have any strong feelings about this WARN_ON_ONCE, will remove.

Thanks

> 
> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

Patch

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e4300f5f304f..9d5a35737abf 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -332,6 +332,14 @@  static umode_t vpd_attr_is_visible(struct kobject *kobj,
 	if (!pdev->vpd.cap)
 		return 0;
 
+	/*
+	 * Mellanox devices have implementation that allows VPD read by
+	 * unprivileged users, so just add needed bits to allow read.
+	 */
+	WARN_ON_ONCE(a->attr.mode != 0600);
+	if (unlikely(pdev->vendor == PCI_VENDOR_ID_MELLANOX))
+		return a->attr.mode + 0044;
+
 	return a->attr.mode;
 }