diff mbox series

[v1,1/2] PCI/sysfs: Change read permissions for VPD attributes

Message ID f93e6b2393301df6ac960ef6891b1b2812da67f3.1731005223.git.leonro@nvidia.com
State New
Headers show
Series Fix read permissions for VPD attributes | expand

Commit Message

Leon Romanovsky Nov. 7, 2024, 6:56 p.m. UTC
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 really needed
for many devices in the world, as data presented in that VPD is not
sensitive and access to the HW is safe and tested.

This change aligns the permissions of the VPD attribute to be accessible
for read by all users, while write being restricted to root only.

For the driver, there is a need to opt-in in order to allow this
functionality.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/pci/vpd.c   | 9 ++++++++-
 include/linux/pci.h | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Nov. 11, 2024, 8:41 p.m. UTC | #1
On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> for many devices in the world, as data presented in that VPD is not
> sensitive and access to the HW is safe and tested.
> 
> This change aligns the permissions of the VPD attribute to be accessible
> for read by all users, while write being restricted to root only.
> 
> For the driver, there is a need to opt-in in order to allow this
> functionality.

I don't think the use case is very strong (and not included at all
here).

If we do need to do this, I think it's a property of the device, not
the driver.

> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/pci/vpd.c   | 9 ++++++++-
>  include/linux/pci.h | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index e4300f5f304f..7c70930abaa0 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -156,6 +156,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  			    void *arg, bool check_size)
>  {
>  	struct pci_vpd *vpd = &dev->vpd;
> +	struct pci_driver *drv;
>  	unsigned int max_len;
>  	int ret = 0;
>  	loff_t end = pos + count;
> @@ -167,6 +168,12 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  	if (pos < 0)
>  		return -EINVAL;
>  
> +	if (!capable(CAP_SYS_ADMIN)) {
> +		drv = to_pci_driver(dev->dev.driver);
> +		if (!drv || !drv->downgrade_vpd_read)
> +			return -EPERM;
> +	}
> +
>  	max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE;
>  
>  	if (pos >= max_len)
> @@ -317,7 +324,7 @@ static ssize_t vpd_write(struct file *filp, struct kobject *kobj,
>  
>  	return ret;
>  }
> -static BIN_ATTR(vpd, 0600, vpd_read, vpd_write, 0);
> +static BIN_ATTR_RW(vpd, 0);
>  
>  static struct bin_attribute *vpd_attrs[] = {
>  	&bin_attr_vpd,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 573b4c4c2be6..b8fed74e742e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -943,6 +943,10 @@ struct module;
>   *		how to manage the DMA themselves and set this flag so that
>   *		the IOMMU layer will allow them to setup and manage their
>   *		own I/O address space.
> + * @downgrade_vpd_read: Device doesn't require root permissions from the users
> + *              to read VPD information. The driver doesn't expose any sensitive
> + *              information through that interface and safe to be accessed by
> + *              unprivileged users.
>   */
>  struct pci_driver {
>  	const char		*name;
> @@ -960,7 +964,8 @@ struct pci_driver {
>  	const struct attribute_group **dev_groups;
>  	struct device_driver	driver;
>  	struct pci_dynids	dynids;
> -	bool driver_managed_dma;
> +	bool driver_managed_dma : 1;
> +	bool downgrade_vpd_read : 1;
>  };
>  
>  #define to_pci_driver(__drv)	\
> -- 
> 2.47.0
>
Thomas Weißschuh Nov. 11, 2024, 9:08 p.m. UTC | #2
On 2024-11-07 20:56:56+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 really needed
> for many devices in the world, as data presented in that VPD is not
> sensitive and access to the HW is safe and tested.
> 
> This change aligns the permissions of the VPD attribute to be accessible
> for read by all users, while write being restricted to root only.
> 
> For the driver, there is a need to opt-in in order to allow this
> functionality.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/pci/vpd.c   | 9 ++++++++-
>  include/linux/pci.h | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index e4300f5f304f..7c70930abaa0 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -156,6 +156,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  			    void *arg, bool check_size)
>  {
>  	struct pci_vpd *vpd = &dev->vpd;
> +	struct pci_driver *drv;
>  	unsigned int max_len;
>  	int ret = 0;
>  	loff_t end = pos + count;
> @@ -167,6 +168,12 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  	if (pos < 0)
>  		return -EINVAL;
>  
> +	if (!capable(CAP_SYS_ADMIN)) {
> +		drv = to_pci_driver(dev->dev.driver);
> +		if (!drv || !drv->downgrade_vpd_read)
> +			return -EPERM;
> +	}

If you move the check into vpd_attr_is_visible() then the sysfs core
will enforce the permissions and it's obvious for the user if they can
or can't read/write the file.

> +
>  	max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE;
>  
>  	if (pos >= max_len)
> @@ -317,7 +324,7 @@ static ssize_t vpd_write(struct file *filp, struct kobject *kobj,
>  
>  	return ret;
>  }
> -static BIN_ATTR(vpd, 0600, vpd_read, vpd_write, 0);
> +static BIN_ATTR_RW(vpd, 0);
>  
>  static struct bin_attribute *vpd_attrs[] = {
>  	&bin_attr_vpd,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 573b4c4c2be6..b8fed74e742e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -943,6 +943,10 @@ struct module;
>   *		how to manage the DMA themselves and set this flag so that
>   *		the IOMMU layer will allow them to setup and manage their
>   *		own I/O address space.
> + * @downgrade_vpd_read: Device doesn't require root permissions from the users
> + *              to read VPD information. The driver doesn't expose any sensitive
> + *              information through that interface and safe to be accessed by
> + *              unprivileged users.
>   */
>  struct pci_driver {
>  	const char		*name;
> @@ -960,7 +964,8 @@ struct pci_driver {
>  	const struct attribute_group **dev_groups;
>  	struct device_driver	driver;
>  	struct pci_dynids	dynids;
> -	bool driver_managed_dma;
> +	bool driver_managed_dma : 1;
> +	bool downgrade_vpd_read : 1;
>  };
>  
>  #define to_pci_driver(__drv)	\
> -- 
> 2.47.0
>
Leon Romanovsky Nov. 11, 2024, 9:17 p.m. UTC | #3
On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > for many devices in the world, as data presented in that VPD is not
> > sensitive and access to the HW is safe and tested.
> > 
> > This change aligns the permissions of the VPD attribute to be accessible
> > for read by all users, while write being restricted to root only.
> > 
> > For the driver, there is a need to opt-in in order to allow this
> > functionality.
> 
> I don't think the use case is very strong (and not included at all
> here).

I will add the use case, which is running monitoring application without
need to be root. IMHO reducing number of applications that require
privileged access is a very strong case. I personally try to avoid
applications with root/setuid privileges.

> 
> If we do need to do this, I think it's a property of the device, not
> the driver.

But how will device inform PCI core about safe VPD read?
Should I add new field to struct pci_device_id? Add a quirk?
Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true"
to mlx5 probe function and it won't change a lot from current
implementation.

> 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/pci/vpd.c   | 9 ++++++++-
> >  include/linux/pci.h | 7 ++++++-
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index e4300f5f304f..7c70930abaa0 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -156,6 +156,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
> >  			    void *arg, bool check_size)
> >  {
> >  	struct pci_vpd *vpd = &dev->vpd;
> > +	struct pci_driver *drv;
> >  	unsigned int max_len;
> >  	int ret = 0;
> >  	loff_t end = pos + count;
> > @@ -167,6 +168,12 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
> >  	if (pos < 0)
> >  		return -EINVAL;
> >  
> > +	if (!capable(CAP_SYS_ADMIN)) {
> > +		drv = to_pci_driver(dev->dev.driver);
> > +		if (!drv || !drv->downgrade_vpd_read)
> > +			return -EPERM;
> > +	}
> > +
> >  	max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE;
> >  
> >  	if (pos >= max_len)
> > @@ -317,7 +324,7 @@ static ssize_t vpd_write(struct file *filp, struct kobject *kobj,
> >  
> >  	return ret;
> >  }
> > -static BIN_ATTR(vpd, 0600, vpd_read, vpd_write, 0);
> > +static BIN_ATTR_RW(vpd, 0);
> >  
> >  static struct bin_attribute *vpd_attrs[] = {
> >  	&bin_attr_vpd,
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 573b4c4c2be6..b8fed74e742e 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -943,6 +943,10 @@ struct module;
> >   *		how to manage the DMA themselves and set this flag so that
> >   *		the IOMMU layer will allow them to setup and manage their
> >   *		own I/O address space.
> > + * @downgrade_vpd_read: Device doesn't require root permissions from the users
> > + *              to read VPD information. The driver doesn't expose any sensitive
> > + *              information through that interface and safe to be accessed by
> > + *              unprivileged users.
> >   */
> >  struct pci_driver {
> >  	const char		*name;
> > @@ -960,7 +964,8 @@ struct pci_driver {
> >  	const struct attribute_group **dev_groups;
> >  	struct device_driver	driver;
> >  	struct pci_dynids	dynids;
> > -	bool driver_managed_dma;
> > +	bool driver_managed_dma : 1;
> > +	bool downgrade_vpd_read : 1;
> >  };
> >  
> >  #define to_pci_driver(__drv)	\
> > -- 
> > 2.47.0
> >
Bjorn Helgaas Nov. 11, 2024, 9:48 p.m. UTC | #4
[+cc Thomas]

On Mon, Nov 11, 2024 at 11:17:38PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > > for many devices in the world, as data presented in that VPD is not
> > > sensitive and access to the HW is safe and tested.
> > > 
> > > This change aligns the permissions of the VPD attribute to be accessible
> > > for read by all users, while write being restricted to root only.
> > > 
> > > For the driver, there is a need to opt-in in order to allow this
> > > functionality.
> > 
> > I don't think the use case is very strong (and not included at all
> > here).
> 
> I will add the use case, which is running monitoring application without
> need to be root. IMHO reducing number of applications that require
> privileged access is a very strong case. I personally try to avoid
> applications with root/setuid privileges.

Avoiding root/setuid is a very good thing.  I just don't think using
VPD directly from userspace is a great idea because VPD is so slow and
sometimes unreliable to read.  And apparently this is a pretty unusual
situation since I haven't heard similar requests for other devices.

Sort of ironic that some vendors, especially Intel and AMD, add new
Device IDs for devices that work exactly the same as their
predecessors, so we are continually adding to the pci_device_id
tables, while here we apparently the same Device ID is used for
devices that differ in ways we actually want to know about.

> > If we do need to do this, I think it's a property of the device, not
> > the driver.
> 
> But how will device inform PCI core about safe VPD read?
> Should I add new field to struct pci_device_id? Add a quirk?
> Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true"
> to mlx5 probe function and it won't change a lot from current
> implementation.

To me it looks like a pci_dev bit, not a pci_driver bit.

I would set it .probe() so all the code is in one place.  And it's not
related to a device defect, as most quirks are.

Bjorn
Stephen Hemminger Nov. 12, 2024, 12:34 a.m. UTC | #5
On Mon, 11 Nov 2024 14:41:04 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > for many devices in the world, as data presented in that VPD is not
> > sensitive and access to the HW is safe and tested.
> > 
> > This change aligns the permissions of the VPD attribute to be accessible
> > for read by all users, while write being restricted to root only.
> > 
> > For the driver, there is a need to opt-in in order to allow this
> > functionality.  
> 
> I don't think the use case is very strong (and not included at all
> here).
> 
> If we do need to do this, I think it's a property of the device, not
> the driver.

I remember some broken PCI devices, which will crash if VPD is read.
Probably not worth opening this can of worms.
Leon Romanovsky Nov. 12, 2024, 6:12 a.m. UTC | #6
On Mon, Nov 11, 2024 at 04:34:30PM -0800, Stephen Hemminger wrote:
> On Mon, 11 Nov 2024 14:41:04 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > > for many devices in the world, as data presented in that VPD is not
> > > sensitive and access to the HW is safe and tested.
> > > 
> > > This change aligns the permissions of the VPD attribute to be accessible
> > > for read by all users, while write being restricted to root only.
> > > 
> > > For the driver, there is a need to opt-in in order to allow this
> > > functionality.  
> > 
> > I don't think the use case is very strong (and not included at all
> > here).
> > 
> > If we do need to do this, I think it's a property of the device, not
> > the driver.
> 
> I remember some broken PCI devices, which will crash if VPD is read.

This is opt-in feature for devices which are known to be working.
Broken devices will continue to be broken and will continue to require
root permissions for read.

Thanks
Leon Romanovsky Nov. 12, 2024, 6:36 a.m. UTC | #7
On Mon, Nov 11, 2024 at 03:48:04PM -0600, Bjorn Helgaas wrote:
> [+cc Thomas]
> 
> On Mon, Nov 11, 2024 at 11:17:38PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > > > for many devices in the world, as data presented in that VPD is not
> > > > sensitive and access to the HW is safe and tested.
> > > > 
> > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > for read by all users, while write being restricted to root only.
> > > > 
> > > > For the driver, there is a need to opt-in in order to allow this
> > > > functionality.
> > > 
> > > I don't think the use case is very strong (and not included at all
> > > here).
> > 
> > I will add the use case, which is running monitoring application without
> > need to be root. IMHO reducing number of applications that require
> > privileged access is a very strong case. I personally try to avoid
> > applications with root/setuid privileges.
> 
> Avoiding root/setuid is a very good thing.  I just don't think using
> VPD directly from userspace is a great idea because VPD is so slow and
> sometimes unreliable to read.

This is one time operation during application initialization, which is
fast in our devices. It is used by the NVML https://developer.nvidia.com/management-library-nvml.

> And apparently this is a pretty unusual situation since I haven't heard
> similar requests for other devices.

Maybe they didn't bother to ask.

> 
> Sort of ironic that some vendors, especially Intel and AMD, add new
> Device IDs for devices that work exactly the same as their
> predecessors, so we are continually adding to the pci_device_id
> tables, while here we apparently the same Device ID is used for
> devices that differ in ways we actually want to know about.

I'm not Intel or AMD employee and never worked there, but from what I
heard it is not technical decision but outcome of how their development
process is structured.

> 
> > > If we do need to do this, I think it's a property of the device, not
> > > the driver.
> > 
> > But how will device inform PCI core about safe VPD read?
> > Should I add new field to struct pci_device_id? Add a quirk?
> > Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true"
> > to mlx5 probe function and it won't change a lot from current
> > implementation.
> 
> To me it looks like a pci_dev bit, not a pci_driver bit.
> 
> I would set it .probe() so all the code is in one place.  And it's not
> related to a device defect, as most quirks are.

The advantage of quirks is that we will be able to set proper file
permissions from the beginning without need to load/bind driver.

As Thomas suggested, the vpd_attr_is_visible() will be something
like this, which is neat:

if (pdev->downgrade_vpd_read)
	return 644;
else
	return 600;

Thanks

> 
> Bjorn
>
Heiner Kallweit Nov. 12, 2024, 6:44 a.m. UTC | #8
On 12.11.2024 01:34, Stephen Hemminger wrote:
> On Mon, 11 Nov 2024 14:41:04 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>> On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
>>> for many devices in the world, as data presented in that VPD is not
>>> sensitive and access to the HW is safe and tested.
>>>
>>> This change aligns the permissions of the VPD attribute to be accessible
>>> for read by all users, while write being restricted to root only.
>>>
>>> For the driver, there is a need to opt-in in order to allow this
>>> functionality.  
>>
>> I don't think the use case is very strong (and not included at all
>> here).
>>
>> If we do need to do this, I think it's a property of the device, not
>> the driver.
> 
> I remember some broken PCI devices, which will crash if VPD is read.
> Probably not worth opening this can of worms.

These crashes shouldn't occur any longer. There are two problematic cases:
1. Reading past end of VPD
   This used to crash certain devices and was fixed by stop reading at
   the VPD end tag.
2. Accessing VPD if device firmware isn't correctly loaded and initialized
   This affects certain LSI devices, which are blacklisted so that PCI core
   prevents VPD access.
Leon Romanovsky Nov. 12, 2024, 7:26 a.m. UTC | #9
On Tue, Nov 12, 2024 at 07:44:09AM +0100, Heiner Kallweit wrote:
> On 12.11.2024 01:34, Stephen Hemminger wrote:
> > On Mon, 11 Nov 2024 14:41:04 -0600
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 
> >> On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> >>> for many devices in the world, as data presented in that VPD is not
> >>> sensitive and access to the HW is safe and tested.
> >>>
> >>> This change aligns the permissions of the VPD attribute to be accessible
> >>> for read by all users, while write being restricted to root only.
> >>>
> >>> For the driver, there is a need to opt-in in order to allow this
> >>> functionality.  
> >>
> >> I don't think the use case is very strong (and not included at all
> >> here).
> >>
> >> If we do need to do this, I think it's a property of the device, not
> >> the driver.
> > 
> > I remember some broken PCI devices, which will crash if VPD is read.
> > Probably not worth opening this can of worms.
> 
> These crashes shouldn't occur any longer. There are two problematic cases:
> 1. Reading past end of VPD
>    This used to crash certain devices and was fixed by stop reading at
>    the VPD end tag.
> 2. Accessing VPD if device firmware isn't correctly loaded and initialized
>    This affects certain LSI devices, which are blacklisted so that PCI core
>    prevents VPD access.

Thanks for the information.

Bjorn,

After this response, do you still think that v0 [1] is not the right way
to change the read permission?

[1] https://lore.kernel.org/all/65791906154e3e5ea12ea49127cf7c707325ca56.1730102428.git.leonro@nvidia.com/

>
Bjorn Helgaas Nov. 12, 2024, 9:48 p.m. UTC | #10
On Tue, Nov 12, 2024 at 09:26:04AM +0200, Leon Romanovsky wrote:
> On Tue, Nov 12, 2024 at 07:44:09AM +0100, Heiner Kallweit wrote:
> > On 12.11.2024 01:34, Stephen Hemminger wrote:
> > > On Mon, 11 Nov 2024 14:41:04 -0600
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > 
> > >> On Thu, Nov 07, 2024 at 08:56:56PM +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 really needed
> > >>> for many devices in the world, as data presented in that VPD is not
> > >>> sensitive and access to the HW is safe and tested.
> > >>>
> > >>> This change aligns the permissions of the VPD attribute to be accessible
> > >>> for read by all users, while write being restricted to root only.
> > >>>
> > >>> For the driver, there is a need to opt-in in order to allow this
> > >>> functionality.  
> > >>
> > >> I don't think the use case is very strong (and not included at all
> > >> here).
> > >>
> > >> If we do need to do this, I think it's a property of the device, not
> > >> the driver.
> > > 
> > > I remember some broken PCI devices, which will crash if VPD is read.
> > > Probably not worth opening this can of worms.
> > 
> > These crashes shouldn't occur any longer. There are two problematic cases:
> > 1. Reading past end of VPD
> >    This used to crash certain devices and was fixed by stop reading at
> >    the VPD end tag.
> > 2. Accessing VPD if device firmware isn't correctly loaded and initialized
> >    This affects certain LSI devices, which are blacklisted so that PCI core
> >    prevents VPD access.
> 
> Thanks for the information.
> 
> Bjorn,
> 
> After this response, do you still think that v0 [1] is not the right way
> to change the read permission?
> 
> [1] https://lore.kernel.org/all/65791906154e3e5ea12ea49127cf7c707325ca56.1730102428.git.leonro@nvidia.com/

Yes, I still think it's unnecessarily risky to make VPD readable
by ordinary users.  This is a pretty niche use case.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e4300f5f304f..7c70930abaa0 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -156,6 +156,7 @@  static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 			    void *arg, bool check_size)
 {
 	struct pci_vpd *vpd = &dev->vpd;
+	struct pci_driver *drv;
 	unsigned int max_len;
 	int ret = 0;
 	loff_t end = pos + count;
@@ -167,6 +168,12 @@  static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (pos < 0)
 		return -EINVAL;
 
+	if (!capable(CAP_SYS_ADMIN)) {
+		drv = to_pci_driver(dev->dev.driver);
+		if (!drv || !drv->downgrade_vpd_read)
+			return -EPERM;
+	}
+
 	max_len = check_size ? vpd->len : PCI_VPD_MAX_SIZE;
 
 	if (pos >= max_len)
@@ -317,7 +324,7 @@  static ssize_t vpd_write(struct file *filp, struct kobject *kobj,
 
 	return ret;
 }
-static BIN_ATTR(vpd, 0600, vpd_read, vpd_write, 0);
+static BIN_ATTR_RW(vpd, 0);
 
 static struct bin_attribute *vpd_attrs[] = {
 	&bin_attr_vpd,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4c2be6..b8fed74e742e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -943,6 +943,10 @@  struct module;
  *		how to manage the DMA themselves and set this flag so that
  *		the IOMMU layer will allow them to setup and manage their
  *		own I/O address space.
+ * @downgrade_vpd_read: Device doesn't require root permissions from the users
+ *              to read VPD information. The driver doesn't expose any sensitive
+ *              information through that interface and safe to be accessed by
+ *              unprivileged users.
  */
 struct pci_driver {
 	const char		*name;
@@ -960,7 +964,8 @@  struct pci_driver {
 	const struct attribute_group **dev_groups;
 	struct device_driver	driver;
 	struct pci_dynids	dynids;
-	bool driver_managed_dma;
+	bool driver_managed_dma : 1;
+	bool downgrade_vpd_read : 1;
 };
 
 #define to_pci_driver(__drv)	\