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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>