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

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

[Bjorn Helgaas]

[+cc Linux hardening folks for any security/reliability concerns]

On Tue, Dec 03, 2024 at 07:40:27PM +0200, Leon Romanovsky wrote:
> On Tue, Dec 03, 2024 at 09:24:56AM -0800, Stephen Hemminger wrote:
> > On Tue,  3 Dec 2024 14:15:28 +0200
> > Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > > 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:
> > > v3:
> > >  * 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..a7aa54203321 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;
> > >  
> > > +	/*
> > > +	 * Mellanox devices have implementation that allows VPD read by
> > > +	 * 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;
> > >  }
> > 
> > Could this be with other vendor specific quirks instead?
> 
> In previous versions, I asked Bjorn about using quirks and the answer
> was that quirks are mainly to fix HW defects fixes and this change doesn't
> belong to that category.
> 
> https://lore.kernel.org/linux-pci/20241111214804.GA1820183@bhelgaas/

That previous proposal was driver-based, so VPD would only be readable
by unprivileged users after mlx5 was loaded.  VPD would be readable at
any time with either a quirk or the current patch.  The quirk would
require a new bit in pci_dev but has the advantage of getting the
Mellanox grunge out of the generic code.

My biggest concerns are that this exposes VPD data of unknown
sensitivity and exercises the sometimes-problematic device VPD
protocol for very little user benefit.  IIUC, the monitoring library
only wants this to identify the specific device variant in the user
interface; it doesn't need it to actually *use* the device.

We think these concerns are minimal for these devices (and I guess for
*all* present and future Mellanox devices), but I don't think it's a
great precedent.

Bjorn

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

[Leon Romanovsky]

On Tue, Dec 03, 2024 at 02:36:25PM -0600, Bjorn Helgaas wrote:
> [+cc Linux hardening folks for any security/reliability concerns]
> 
> On Tue, Dec 03, 2024 at 07:40:27PM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 03, 2024 at 09:24:56AM -0800, Stephen Hemminger wrote:
> > > On Tue,  3 Dec 2024 14:15:28 +0200
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > > 
> > > > 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:
> > > > v3:
> > > >  * 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..a7aa54203321 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;
> > > >  
> > > > +	/*
> > > > +	 * Mellanox devices have implementation that allows VPD read by
> > > > +	 * 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;
> > > >  }
> > > 
> > > Could this be with other vendor specific quirks instead?
> > 
> > In previous versions, I asked Bjorn about using quirks and the answer
> > was that quirks are mainly to fix HW defects fixes and this change doesn't
> > belong to that category.
> > 
> > https://lore.kernel.org/linux-pci/20241111214804.GA1820183@bhelgaas/
> 
> That previous proposal was driver-based, so VPD would only be readable
> by unprivileged users after mlx5 was loaded.  VPD would be readable at
> any time with either a quirk or the current patch.  The quirk would
> require a new bit in pci_dev but has the advantage of getting the
> Mellanox grunge out of the generic code.
> 
> My biggest concerns are that this exposes VPD data of unknown
> sensitivity and exercises the sometimes-problematic device VPD
> protocol for very little user benefit.  IIUC, the monitoring library
> only wants this to identify the specific device variant in the user
> interface; it doesn't need it to actually *use* the device.
> 
> We think these concerns are minimal for these devices (and I guess for
> *all* present and future Mellanox devices), but I don't think it's a
> great precedent.

Yes, and we can always move this "if ..." to quirks once second device
will appear.

Thanks

> 
> Bjorn