PCI/sysfs: Fix read permissions for VPD attributes

Commit Message

Leon Romanovsky Oct. 28, 2024, 8:05 a.m. UTC

From: Leon Romanovsky <[e-mail]>

The Virtual Product Data (VPD) attribute is not readable by regular
user without root permissions. Such restriction is not really needed,
as data presented in that VPD is not sensitive at all.

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

Cc: [e-mail]
Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
Signed-off-by: Leon Romanovsky <[e-mail]>
---
I added stable@ as it was discovered during our hardware ennoblement
and it is important to be picked by distributions too.
---
 drivers/pci/vpd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bjorn Helgaas Oct. 30, 2024, 12:04 a.m. UTC | #1

On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <[e-mail]>
> 
> The Virtual Product Data (VPD) attribute is not readable by regular
> user without root permissions. Such restriction is not really needed,
> as data presented in that VPD is not sensitive at all.
> 
> This change aligns the permissions of the VPD attribute to be accessible
> for read by all users, while write being restricted to root only.
> 
> Cc: [e-mail]
> Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> Signed-off-by: Leon Romanovsky <[e-mail]>

Applied to pci/vpd for v6.13, thanks!

> ---
> I added stable@ as it was discovered during our hardware ennoblement
> and it is important to be picked by distributions too.
> ---
>  drivers/pci/vpd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index e4300f5f304f..2537685cac90 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -317,7 +317,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,
> -- 
> 2.46.2
>

Bjorn Helgaas Oct. 31, 2024, 11:22 p.m. UTC | #2

[+cc LKML etc, should PCI VPD be readable by non-root?]

On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <[e-mail]>
> > 
> > The Virtual Product Data (VPD) attribute is not readable by regular

("Vital Product Data" is the term in the spec)

> > user without root permissions. Such restriction is not really needed,
> > as data presented in that VPD is not sensitive at all.
> > 
> > This change aligns the permissions of the VPD attribute to be accessible
> > for read by all users, while write being restricted to root only.
> > 
> > Cc: [e-mail]
> > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > Signed-off-by: Leon Romanovsky <[e-mail]>
> 
> Applied to pci/vpd for v6.13, thanks!

I think this deserves a little more consideration than I gave it
initially.

Obviously somebody is interested in using this; can we include some
examples so we know there's an actual user?

Are we confident that VPD never contains anything sensitive?  It may
contain arbitrary vendor-specific information, so we can't know what
might be in that part.

Reading VPD is fairly complicated and we've had problems in the past
(we have quirk_blacklist_vpd() for devices that behave
"unpredictably"), so it's worth considering whether allowing non-root
to do this could be exploited or could allow DOS attacks.

For reference, here are the fields defined in PCIe r6.0, sec 6.27.2
(although VPD can contain anything a manufacturer wants to put there):

  PN Add-in Card Part Number
  EC Engineering Change Level of the Add-in Card
  FG Fabric Geography
  LC Location
  MN Manufacture ID
  PG PCI Geography
  SN Serial Number
  TR Thermal Reporting
  Vx Vendor Specific
  CP Extended Capability
  RV Checksum and Reserved
  FF Form Factor
  Yx System Specific
  YA Asset Tag Identifier
  RW Remaining Read/Write Area

The Conventional PCI spec, r3.0, sec 6.4, says:

  Vital Product Data (VPD) is the information that uniquely defines
  items such as the hardware, software, and microcode elements of a
  system. The VPD provides the system with information on various FRUs
  (Field Replaceable Unit) including Part Number, Serial Number, and
  other detailed information. VPD also provides a mechanism for
  storing information such as performance and failure data on the
  device being monitored. The objective, from a system point of view,
  is to collect this information by reading it from the hardware,
  software, and microcode components.

Some of that, e.g., performance and failure data, might be considered
sensitive in some environments.

> > ---
> > I added stable@ as it was discovered during our hardware ennoblement
> > and it is important to be picked by distributions too.
> > ---
> >  drivers/pci/vpd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index e4300f5f304f..2537685cac90 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -317,7 +317,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,
> > -- 
> > 2.46.2
> >

Leon Romanovsky Nov. 1, 2024, 2:33 p.m. UTC | #3

On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> [+cc LKML etc, should PCI VPD be readable by non-root?]
> 
> On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <[e-mail]>
> > > 
> > > The Virtual Product Data (VPD) attribute is not readable by regular
> 
> ("Vital Product Data" is the term in the spec)

Ohh, sorry for the confusion. I relied to much on auto-completion.

> 
> > > user without root permissions. Such restriction is not really needed,
> > > as data presented in that VPD is not sensitive at all.
> > > 
> > > This change aligns the permissions of the VPD attribute to be accessible
> > > for read by all users, while write being restricted to root only.
> > > 
> > > Cc: [e-mail]
> > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > 
> > Applied to pci/vpd for v6.13, thanks!
> 
> I think this deserves a little more consideration than I gave it
> initially.
> 
> Obviously somebody is interested in using this; can we include some
> examples so we know there's an actual user?

I'll provide it after the weekend.

> 
> Are we confident that VPD never contains anything sensitive?  It may
> contain arbitrary vendor-specific information, so we can't know what
> might be in that part.

It depends on the vendor, but I'm pretty confident that any sane vendor who
read the PCI spec will not put sensitive information in the VPD. The
spec is very clear that this open to everyone.

> 
> Reading VPD is fairly complicated and we've had problems in the past
> (we have quirk_blacklist_vpd() for devices that behave
> "unpredictably"), so it's worth considering whether allowing non-root
> to do this could be exploited or could allow DOS attacks.

It is not different from any other PCI field. If you are afraid of DOS,
you should limit to read all other fields too.

> 
> For reference, here are the fields defined in PCIe r6.0, sec 6.27.2
> (although VPD can contain anything a manufacturer wants to put there):
> 
>   PN Add-in Card Part Number
>   EC Engineering Change Level of the Add-in Card
>   FG Fabric Geography
>   LC Location
>   MN Manufacture ID
>   PG PCI Geography
>   SN Serial Number
>   TR Thermal Reporting
>   Vx Vendor Specific
>   CP Extended Capability
>   RV Checksum and Reserved
>   FF Form Factor
>   Yx System Specific
>   YA Asset Tag Identifier
>   RW Remaining Read/Write Area
> 
> The Conventional PCI spec, r3.0, sec 6.4, says:
> 
>   Vital Product Data (VPD) is the information that uniquely defines
>   items such as the hardware, software, and microcode elements of a
>   system. The VPD provides the system with information on various FRUs
>   (Field Replaceable Unit) including Part Number, Serial Number, and
>   other detailed information. VPD also provides a mechanism for
>   storing information such as performance and failure data on the
>   device being monitored. The objective, from a system point of view,
>   is to collect this information by reading it from the hardware,
>   software, and microcode components.
> 
> Some of that, e.g., performance and failure data, might be considered
> sensitive in some environments.

I'm enabling it for modern device which is compliant to PCI spec v6.0.
Do you want me to add quirk_allow_vpd() to allow only specific devices to
read that field?a It is doable but not scalable.

> 
> > > ---
> > > I added stable@ as it was discovered during our hardware ennoblement
> > > and it is important to be picked by distributions too.
> > > ---
> > >  drivers/pci/vpd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > > index e4300f5f304f..2537685cac90 100644
> > > --- a/drivers/pci/vpd.c
> > > +++ b/drivers/pci/vpd.c
> > > @@ -317,7 +317,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,
> > > -- 
> > > 2.46.2
> > >

Bjorn Helgaas Nov. 1, 2024, 4:47 p.m. UTC | #4

On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <[e-mail]>
> > > > 
> > > > The Virtual Product Data (VPD) attribute is not readable by regular
> > > > user without root permissions. Such restriction is not really needed,
> > > > as data presented in that VPD is not sensitive at all.
> > > > 
> > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > for read by all users, while write being restricted to root only.
> > > > 
> > > > Cc: [e-mail]
> > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > 
> > > Applied to pci/vpd for v6.13, thanks!
> > 
> > I think this deserves a little more consideration than I gave it
> > initially.
> > 
> > Obviously somebody is interested in using this; can we include some
> > examples so we know there's an actual user?
> 
> I'll provide it after the weekend.
> 
> > Are we confident that VPD never contains anything sensitive?  It may
> > contain arbitrary vendor-specific information, so we can't know what
> > might be in that part.
> 
> It depends on the vendor, but I'm pretty confident that any sane vendor who
> read the PCI spec will not put sensitive information in the VPD. The
> spec is very clear that this open to everyone.

I don't think the spec really defines "everyone" in this context, does
it?  The concept of privileged vs unprivileged users is an OS
construct, not really something the PCIe spec covers.

> > Reading VPD is fairly complicated and we've had problems in the past
> > (we have quirk_blacklist_vpd() for devices that behave
> > "unpredictably"), so it's worth considering whether allowing non-root
> > to do this could be exploited or could allow DOS attacks.
> 
> It is not different from any other PCI field. If you are afraid of DOS,
> you should limit to read all other fields too.

Reading VPD is much different than reading things from config space.

To read VPD, software needs to:

  - Mutex with any other read/write path

  - Write the VPD address to read to the VPD Address register, with F
    bit clear

  - Wait (with timeout) for hardware to set the F bit of VPD Address
    register

  - Read VPD information from the VPD Data register

  - Repeat as necessary

The address is 15 bits wide, so there may be up to 32KB of VPD data.
The only way to determine the actual length is to read the data and
parse the data items, which is vulnerable to corrupted EEPROMs and
hardware issues if we read beyond the implemented size.

The PCI core currently doesn't touch VPD until a driver or userspace
(via sysfs) reads or writes it, so this path is not tested on most
devices.

> > For reference, here are the fields defined in PCIe r6.0, sec 6.27.2
> > (although VPD can contain anything a manufacturer wants to put there):
> > 
> >   PN Add-in Card Part Number
> >   EC Engineering Change Level of the Add-in Card
> >   FG Fabric Geography
> >   LC Location
> >   MN Manufacture ID
> >   PG PCI Geography
> >   SN Serial Number
> >   TR Thermal Reporting
> >   Vx Vendor Specific
> >   CP Extended Capability
> >   RV Checksum and Reserved
> >   FF Form Factor
> >   Yx System Specific
> >   YA Asset Tag Identifier
> >   RW Remaining Read/Write Area
> > 
> > The Conventional PCI spec, r3.0, sec 6.4, says:
> > 
> >   Vital Product Data (VPD) is the information that uniquely defines
> >   items such as the hardware, software, and microcode elements of a
> >   system. The VPD provides the system with information on various FRUs
> >   (Field Replaceable Unit) including Part Number, Serial Number, and
> >   other detailed information. VPD also provides a mechanism for
> >   storing information such as performance and failure data on the
> >   device being monitored. The objective, from a system point of view,
> >   is to collect this information by reading it from the hardware,
> >   software, and microcode components.
> > 
> > Some of that, e.g., performance and failure data, might be considered
> > sensitive in some environments.
> 
> I'm enabling it for modern device which is compliant to PCI spec v6.0.
> Do you want me to add quirk_allow_vpd() to allow only specific devices to
> read that field? It is doable but not scalable.

None of these questions really has to do with old vs new devices.  An
"allow-list" quirk is possible, but I agree it would be a maintenance
headache.  To me it feels like VPD is kind of in the same category as
dmesg logs.  We try to avoid putting secret stuff in dmesg, but
generally distros still don't make it completely public.

> > > > ---
> > > > I added stable@ as it was discovered during our hardware ennoblement
> > > > and it is important to be picked by distributions too.
> > > > ---
> > > >  drivers/pci/vpd.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > > > index e4300f5f304f..2537685cac90 100644
> > > > --- a/drivers/pci/vpd.c
> > > > +++ b/drivers/pci/vpd.c
> > > > @@ -317,7 +317,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,
> > > > -- 
> > > > 2.46.2
> > > >

Leon Romanovsky Nov. 3, 2024, 12:33 p.m. UTC | #5

On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <[e-mail]>
> > > > > 
> > > > > The Virtual Product Data (VPD) attribute is not readable by regular
> > > > > user without root permissions. Such restriction is not really needed,
> > > > > as data presented in that VPD is not sensitive at all.
> > > > > 
> > > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > > for read by all users, while write being restricted to root only.
> > > > > 
> > > > > Cc: [e-mail]
> > > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > > 
> > > > Applied to pci/vpd for v6.13, thanks!
> > > 
> > > I think this deserves a little more consideration than I gave it
> > > initially.
> > > 
> > > Obviously somebody is interested in using this; can we include some
> > > examples so we know there's an actual user?
> > 
> > I'll provide it after the weekend.

As it is seen through lspci, nothing criminal here.
08:00.0 Ethernet controller: Mellanox Technologies MT2910 Family [ConnectX-7]
...
        Capabilities: [48] Vital Product Data
                Product Name: NVIDIA ConnectX-7 HHHL adapter Card, 200GbE / NDR200 IB, Dual-port QSFP112, PCIe 5.0 x16 with x16 PCIe extension option, Crypto, Secure Boot Capable
                Read-only fields:
                        [PN] Part number: MCX713106AEHEA_QP1
                        [EC] Engineering changes: A5
                        [V2] Vendor specific: MCX713106AEHEA_QP1
                        [SN] Serial number: MT2314XZ0JUZ
                        [V3] Vendor specific: 0a5efb8958deed118000946dae7db798
                        [VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A
                        [V0] Vendor specific: PCIeGen5 x16
                        [VU] Vendor specific: MT2314XZ0JUZMLNXS0D0F0
                        [RV] Reserved: checksum good, 1 byte(s) reserved
                End

> > 
> > > Are we confident that VPD never contains anything sensitive?  It may
> > > contain arbitrary vendor-specific information, so we can't know what
> > > might be in that part.
> > 
> > It depends on the vendor, but I'm pretty confident that any sane vendor who
> > read the PCI spec will not put sensitive information in the VPD. The
> > spec is very clear that this open to everyone.
> 
> I don't think the spec really defines "everyone" in this context, does
> it?  The concept of privileged vs unprivileged users is an OS
> construct, not really something the PCIe spec covers.

Agree that it OS specific, but for me, the fields like manufacturer ID,
serial number e.t.c shows that the VPD doesn't contain sensitive information.

> 
> > > Reading VPD is fairly complicated and we've had problems in the past
> > > (we have quirk_blacklist_vpd() for devices that behave
> > > "unpredictably"), so it's worth considering whether allowing non-root
> > > to do this could be exploited or could allow DOS attacks.
> > 
> > It is not different from any other PCI field. If you are afraid of DOS,
> > you should limit to read all other fields too.
> 
> Reading VPD is much different than reading things from config space.
> 
> To read VPD, software needs to:
> 
>   - Mutex with any other read/write path
> 
>   - Write the VPD address to read to the VPD Address register, with F
>     bit clear
> 
>   - Wait (with timeout) for hardware to set the F bit of VPD Address
>     register
> 
>   - Read VPD information from the VPD Data register
> 
>   - Repeat as necessary
> 
> The address is 15 bits wide, so there may be up to 32KB of VPD data.
> The only way to determine the actual length is to read the data and
> parse the data items, which is vulnerable to corrupted EEPROMs and
> hardware issues if we read beyond the implemented size.
> 
> The PCI core currently doesn't touch VPD until a driver or userspace
> (via sysfs) reads or writes it, so this path is not tested on most
> devices.

The patch yes, but the flow is tested very well. It is hard to imagine
situation where "lspci -vv" or corresponding library, never used to read
data from device. Maybe it is not used daily on all computers, but all
devices at least once in their lifetime were accessed.

> 
> > > For reference, here are the fields defined in PCIe r6.0, sec 6.27.2
> > > (although VPD can contain anything a manufacturer wants to put there):
> > > 
> > >   PN Add-in Card Part Number
> > >   EC Engineering Change Level of the Add-in Card
> > >   FG Fabric Geography
> > >   LC Location
> > >   MN Manufacture ID
> > >   PG PCI Geography
> > >   SN Serial Number
> > >   TR Thermal Reporting
> > >   Vx Vendor Specific
> > >   CP Extended Capability
> > >   RV Checksum and Reserved
> > >   FF Form Factor
> > >   Yx System Specific
> > >   YA Asset Tag Identifier
> > >   RW Remaining Read/Write Area
> > > 
> > > The Conventional PCI spec, r3.0, sec 6.4, says:
> > > 
> > >   Vital Product Data (VPD) is the information that uniquely defines
> > >   items such as the hardware, software, and microcode elements of a
> > >   system. The VPD provides the system with information on various FRUs
> > >   (Field Replaceable Unit) including Part Number, Serial Number, and
> > >   other detailed information. VPD also provides a mechanism for
> > >   storing information such as performance and failure data on the
> > >   device being monitored. The objective, from a system point of view,
> > >   is to collect this information by reading it from the hardware,
> > >   software, and microcode components.
> > > 
> > > Some of that, e.g., performance and failure data, might be considered
> > > sensitive in some environments.
> > 
> > I'm enabling it for modern device which is compliant to PCI spec v6.0.
> > Do you want me to add quirk_allow_vpd() to allow only specific devices to
> > read that field? It is doable but not scalable.
> 
> None of these questions really has to do with old vs new devices.  An
> "allow-list" quirk is possible, but I agree it would be a maintenance
> headache.  To me it feels like VPD is kind of in the same category as
> dmesg logs.  We try to avoid putting secret stuff in dmesg, but
> generally distros still don't make it completely public.

They hide it as dmesg already exposes a lot of sensitive data. For
example, the kernel panic reveals a lot of such data. It is definitely
not the case for VPD, and VPD vs. dmesg comparison is not correct one.

Thanks

> 
> > > > > ---
> > > > > I added stable@ as it was discovered during our hardware ennoblement
> > > > > and it is important to be picked by distributions too.
> > > > > ---
> > > > >  drivers/pci/vpd.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > > > > index e4300f5f304f..2537685cac90 100644
> > > > > --- a/drivers/pci/vpd.c
> > > > > +++ b/drivers/pci/vpd.c
> > > > > @@ -317,7 +317,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,
> > > > > -- 
> > > > > 2.46.2
> > > > >

Bjorn Helgaas Nov. 5, 2024, 12:10 a.m. UTC | #6

On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <[e-mail]>
> > > > > > 
> > > > > > The Virtual Product Data (VPD) attribute is not readable by regular
> > > > > > user without root permissions. Such restriction is not really needed,
> > > > > > as data presented in that VPD is not sensitive at all.
> > > > > > 
> > > > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > > > for read by all users, while write being restricted to root only.
> > > > > > 
> > > > > > Cc: [e-mail]
> > > > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > > > 
> > > > > Applied to pci/vpd for v6.13, thanks!
> > > > 
> > > > I think this deserves a little more consideration than I gave it
> > > > initially.
> > > > 
> > > > Obviously somebody is interested in using this; can we include some
> > > > examples so we know there's an actual user?
> > > 
> > > I'll provide it after the weekend.
> 
> As it is seen through lspci, nothing criminal here.
> 08:00.0 Ethernet controller: Mellanox Technologies MT2910 Family [ConnectX-7]
> ...
>         Capabilities: [48] Vital Product Data
>                 Product Name: NVIDIA ConnectX-7 HHHL adapter Card, 200GbE / NDR200 IB, Dual-port QSFP112, PCIe 5.0 x16 with x16 PCIe extension option, Crypto, Secure Boot Capable
>                 Read-only fields:
>                         [PN] Part number: MCX713106AEHEA_QP1
>                         [EC] Engineering changes: A5
>                         [V2] Vendor specific: MCX713106AEHEA_QP1
>                         [SN] Serial number: MT2314XZ0JUZ
>                         [V3] Vendor specific: 0a5efb8958deed118000946dae7db798
>                         [VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A
>                         [V0] Vendor specific: PCIeGen5 x16
>                         [VU] Vendor specific: MT2314XZ0JUZMLNXS0D0F0
>                         [RV] Reserved: checksum good, 1 byte(s) reserved
>                 End
> 
> > > > Are we confident that VPD never contains anything sensitive?
> > > > It may contain arbitrary vendor-specific information, so we
> > > > can't know what might be in that part.
> > > 
> > > It depends on the vendor, but I'm pretty confident that any sane
> > > vendor who read the PCI spec will not put sensitive information
> > > in the VPD. The spec is very clear that this open to everyone.
> > 
> > I don't think the spec really defines "everyone" in this context,
> > does it?  The concept of privileged vs unprivileged users is an OS
> > construct, not really something the PCIe spec covers.
> 
> Agree that it OS specific, but for me, the fields like manufacturer
> ID, serial number e.t.c shows that the VPD doesn't contain sensitive
> information.

I don't follow the reasoning that because these fields don't seem
sensitive, other fields won't be :)

> > > > Reading VPD is fairly complicated and we've had problems in
> > > > the past (we have quirk_blacklist_vpd() for devices that
> > > > behave "unpredictably"), so it's worth considering whether
> > > > allowing non-root to do this could be exploited or could allow
> > > > DOS attacks.
> > > 
> > > It is not different from any other PCI field. If you are afraid
> > > of DOS, you should limit to read all other fields too.
> > 
> > Reading VPD is much different than reading things from config space.
> > 
> > To read VPD, software needs to:
> > 
> >   - Mutex with any other read/write path
> > 
> >   - Write the VPD address to read to the VPD Address register, with F
> >     bit clear
> > 
> >   - Wait (with timeout) for hardware to set the F bit of VPD Address
> >     register
> > 
> >   - Read VPD information from the VPD Data register
> > 
> >   - Repeat as necessary
> > 
> > The address is 15 bits wide, so there may be up to 32KB of VPD data.
> > The only way to determine the actual length is to read the data and
> > parse the data items, which is vulnerable to corrupted EEPROMs and
> > hardware issues if we read beyond the implemented size.
> > 
> > The PCI core currently doesn't touch VPD until a driver or userspace
> > (via sysfs) reads or writes it, so this path is not tested on most
> > devices.
> 
> The patch yes, but the flow is tested very well. It is hard to imagine
> situation where "lspci -vv" or corresponding library, never used to read
> data from device. Maybe it is not used daily on all computers, but all
> devices at least once in their lifetime were accessed.

Well, true, but I think "lspci -vv" requires root privilege to read
the VPD data, doesn't it?

> > > I'm enabling it for modern device which is compliant to PCI spec
> > > v6.0.  Do you want me to add quirk_allow_vpd() to allow only
> > > specific devices to read that field? It is doable but not
> > > scalable.
> > 
> > None of these questions really has to do with old vs new devices.
> > An "allow-list" quirk is possible, but I agree it would be a
> > maintenance headache.  To me it feels like VPD is kind of in the
> > same category as dmesg logs.  We try to avoid putting secret stuff
> > in dmesg, but generally distros still don't make it completely
> > public.
> 
> They hide it as dmesg already exposes a lot of sensitive data. For
> example, the kernel panic reveals a lot of such data. It is
> definitely not the case for VPD, and VPD vs. dmesg comparison is not
> correct one.

dmidecode is another similar case, which is also not public.

What's the use case?  How does an unprivileged user use the VPD
information?

I can certainly imagine using VPD for bug reporting, but that would
typically involve dmesg, dmidecode, lspci -vv, etc, all of which
already require privilege, so it's not clear to me how public VPD info
would help in that scenario.

Bjorn

Leon Romanovsky Nov. 5, 2024, 7:51 a.m. UTC | #7

On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
> On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > > > > From: Leon Romanovsky <[e-mail]>
> > > > > > > 
> > > > > > > The Virtual Product Data (VPD) attribute is not readable by regular
> > > > > > > user without root permissions. Such restriction is not really needed,
> > > > > > > as data presented in that VPD is not sensitive at all.
> > > > > > > 
> > > > > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > > > > for read by all users, while write being restricted to root only.
> > > > > > > 
> > > > > > > Cc: [e-mail]
> > > > > > > Fixes: d93f8399053d ("PCI/sysfs: Convert "vpd" to static attribute")
> > > > > > > Signed-off-by: Leon Romanovsky <[e-mail]>
> > > > > > 
> > > > > > Applied to pci/vpd for v6.13, thanks!
> > > > > 
> > > > > I think this deserves a little more consideration than I gave it
> > > > > initially.
> > > > > 
> > > > > Obviously somebody is interested in using this; can we include some
> > > > > examples so we know there's an actual user?
> > > > 
> > > > I'll provide it after the weekend.
> > 
> > As it is seen through lspci, nothing criminal here.
> > 08:00.0 Ethernet controller: Mellanox Technologies MT2910 Family [ConnectX-7]
> > ...
> >         Capabilities: [48] Vital Product Data
> >                 Product Name: NVIDIA ConnectX-7 HHHL adapter Card, 200GbE / NDR200 IB, Dual-port QSFP112, PCIe 5.0 x16 with x16 PCIe extension option, Crypto, Secure Boot Capable
> >                 Read-only fields:
> >                         [PN] Part number: MCX713106AEHEA_QP1
> >                         [EC] Engineering changes: A5
> >                         [V2] Vendor specific: MCX713106AEHEA_QP1
> >                         [SN] Serial number: MT2314XZ0JUZ
> >                         [V3] Vendor specific: 0a5efb8958deed118000946dae7db798
> >                         [VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A
> >                         [V0] Vendor specific: PCIeGen5 x16
> >                         [VU] Vendor specific: MT2314XZ0JUZMLNXS0D0F0
> >                         [RV] Reserved: checksum good, 1 byte(s) reserved
> >                 End
> > 
> > > > > Are we confident that VPD never contains anything sensitive?
> > > > > It may contain arbitrary vendor-specific information, so we
> > > > > can't know what might be in that part.
> > > > 
> > > > It depends on the vendor, but I'm pretty confident that any sane
> > > > vendor who read the PCI spec will not put sensitive information
> > > > in the VPD. The spec is very clear that this open to everyone.
> > > 
> > > I don't think the spec really defines "everyone" in this context,
> > > does it?  The concept of privileged vs unprivileged users is an OS
> > > construct, not really something the PCIe spec covers.
> > 
> > Agree that it OS specific, but for me, the fields like manufacturer
> > ID, serial number e.t.c shows that the VPD doesn't contain sensitive
> > information.
> 
> I don't follow the reasoning that because these fields don't seem
> sensitive, other fields won't be :)

It was not my best argument :)

> 
> > > > > Reading VPD is fairly complicated and we've had problems in
> > > > > the past (we have quirk_blacklist_vpd() for devices that
> > > > > behave "unpredictably"), so it's worth considering whether
> > > > > allowing non-root to do this could be exploited or could allow
> > > > > DOS attacks.
> > > > 
> > > > It is not different from any other PCI field. If you are afraid
> > > > of DOS, you should limit to read all other fields too.
> > > 
> > > Reading VPD is much different than reading things from config space.
> > > 
> > > To read VPD, software needs to:
> > > 
> > >   - Mutex with any other read/write path
> > > 
> > >   - Write the VPD address to read to the VPD Address register, with F
> > >     bit clear
> > > 
> > >   - Wait (with timeout) for hardware to set the F bit of VPD Address
> > >     register
> > > 
> > >   - Read VPD information from the VPD Data register
> > > 
> > >   - Repeat as necessary
> > > 
> > > The address is 15 bits wide, so there may be up to 32KB of VPD data.
> > > The only way to determine the actual length is to read the data and
> > > parse the data items, which is vulnerable to corrupted EEPROMs and
> > > hardware issues if we read beyond the implemented size.
> > > 
> > > The PCI core currently doesn't touch VPD until a driver or userspace
> > > (via sysfs) reads or writes it, so this path is not tested on most
> > > devices.
> > 
> > The patch yes, but the flow is tested very well. It is hard to imagine
> > situation where "lspci -vv" or corresponding library, never used to read
> > data from device. Maybe it is not used daily on all computers, but all
> > devices at least once in their lifetime were accessed.
> 
> Well, true, but I think "lspci -vv" requires root privilege to read
> the VPD data, doesn't it?

The point was different, I argued that VPD flow is tested thoroughly.
BTW, from my experience with testing in HW companies, they run all tests
as root.

> 
> > > > I'm enabling it for modern device which is compliant to PCI spec
> > > > v6.0.  Do you want me to add quirk_allow_vpd() to allow only
> > > > specific devices to read that field? It is doable but not
> > > > scalable.
> > > 
> > > None of these questions really has to do with old vs new devices.
> > > An "allow-list" quirk is possible, but I agree it would be a
> > > maintenance headache.  To me it feels like VPD is kind of in the
> > > same category as dmesg logs.  We try to avoid putting secret stuff
> > > in dmesg, but generally distros still don't make it completely
> > > public.
> > 
> > They hide it as dmesg already exposes a lot of sensitive data. For
> > example, the kernel panic reveals a lot of such data. It is
> > definitely not the case for VPD, and VPD vs. dmesg comparison is not
> > correct one.
> 
> dmidecode is another similar case, which is also not public.
> 
> What's the use case?  How does an unprivileged user use the VPD
> information?

We have to add new field keyword=value in VA section of VPD, which will
indicate very specific sub-model for devices used as a bridge.

> 
> I can certainly imagine using VPD for bug reporting, but that would
> typically involve dmesg, dmidecode, lspci -vv, etc, all of which
> already require privilege, so it's not clear to me how public VPD info
> would help in that scenario.

I'm targeting other scenario - monitoring tool, which doesn't need root
permissions for reading data. It needs to distinguish between NIC sub-models.

Thanks

> 
> Bjorn

Bjorn Helgaas Nov. 5, 2024, 3:24 p.m. UTC | #8

On Tue, Nov 05, 2024 at 09:51:30AM +0200, Leon Romanovsky wrote:
> On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
> > On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> > > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> > > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > > > > > From: Leon Romanovsky <[e-mail]>
> > > > > > > > 
> > > > > > > > The Virtual Product Data (VPD) attribute is not
> > > > > > > > readable by regular user without root permissions.
> > > > > > > > Such restriction is not really needed, as data
> > > > > > > > presented in that VPD is not sensitive at all.
> > > > > > > > 
> > > > > > > > This change aligns the permissions of the VPD
> > > > > > > > attribute to be accessible for read by all users,
> > > > > > > > while write being restricted to root only.
> ...

> > What's the use case?  How does an unprivileged user use the VPD
> > information?
> 
> We have to add new field keyword=value in VA section of VPD, which
> will indicate very specific sub-model for devices used as a bridge.
> 
> > I can certainly imagine using VPD for bug reporting, but that
> > would typically involve dmesg, dmidecode, lspci -vv, etc, all of
> > which already require privilege, so it's not clear to me how
> > public VPD info would help in that scenario.
> 
> I'm targeting other scenario - monitoring tool, which doesn't need
> root permissions for reading data. It needs to distinguish between
> NIC sub-models.

Maybe the driver could expose something in sysfs?  Maybe the driver
needs to know the sub-model as well, and reading VPD once in the
driver would make subsequent userspace sysfs reads trivial and fast.

Bjorn

Leon Romanovsky Nov. 5, 2024, 4:26 p.m. UTC | #9

On Tue, Nov 05, 2024 at 09:24:55AM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 05, 2024 at 09:51:30AM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
> > > On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> > > > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> > > > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> > > > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> > > > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> > > > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> > > > > > > > > From: Leon Romanovsky <[e-mail]>
> > > > > > > > > 
> > > > > > > > > The Virtual Product Data (VPD) attribute is not
> > > > > > > > > readable by regular user without root permissions.
> > > > > > > > > Such restriction is not really needed, as data
> > > > > > > > > presented in that VPD is not sensitive at all.
> > > > > > > > > 
> > > > > > > > > This change aligns the permissions of the VPD
> > > > > > > > > attribute to be accessible for read by all users,
> > > > > > > > > while write being restricted to root only.
> > ...
> 
> > > What's the use case?  How does an unprivileged user use the VPD
> > > information?
> > 
> > We have to add new field keyword=value in VA section of VPD, which
> > will indicate very specific sub-model for devices used as a bridge.
> > 
> > > I can certainly imagine using VPD for bug reporting, but that
> > > would typically involve dmesg, dmidecode, lspci -vv, etc, all of
> > > which already require privilege, so it's not clear to me how
> > > public VPD info would help in that scenario.
> > 
> > I'm targeting other scenario - monitoring tool, which doesn't need
> > root permissions for reading data. It needs to distinguish between
> > NIC sub-models.
> 
> Maybe the driver could expose something in sysfs?  Maybe the driver
> needs to know the sub-model as well, and reading VPD once in the
> driver would make subsequent userspace sysfs reads trivial and fast.

Our PCI driver lays in netdev subsystem and they have long-standing
position do not allow any custom sysfs files. To be fair, we (RDMA)
don't allow custom sysfs files too.

Driver doesn't need to know this information as it is extra key=value in
existing [VA] field, while driver relies on multiple FW capabilities
to enable/disable functionality.

Current [VA] line:
"[VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A"
Future [VA] line:
"[VA] Vendor specific: MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A,SMDL=SOMETHING"

Also the idea that we will duplicate existing functionality doesn't
sound like a good approach to me, and there is no way that it is
possible to expose as subsystem specific file.

What about to allow existing VPD sysfs file to be readable for everyone for our devices?
And if this allow list grows to much, we will open it for all devices in the world?

Thanks

> 
> Bjorn

Leon Romanovsky Nov. 7, 2024, 11:31 a.m. UTC | #10

On Tue, Nov 5, 2024, at 18:26, Leon Romanovsky wrote:
> On Tue, Nov 05, 2024 at 09:24:55AM -0600, Bjorn Helgaas wrote:
>> On Tue, Nov 05, 2024 at 09:51:30AM +0200, Leon Romanovsky wrote:
>> > On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
>> > > On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
>> > > > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
>> > > > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
>> > > > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
>> > > > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
>> > > > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
>> > > > > > > > > From: Leon Romanovsky <[e-mail]>
>> > > > > > > > > 
>> > > > > > > > > The Virtual Product Data (VPD) attribute is not
>> > > > > > > > > readable by regular user without root permissions.
>> > > > > > > > > Such restriction is not really needed, as data
>> > > > > > > > > presented in that VPD is not sensitive at all.
>> > > > > > > > > 
>> > > > > > > > > This change aligns the permissions of the VPD
>> > > > > > > > > attribute to be accessible for read by all users,
>> > > > > > > > > while write being restricted to root only.
>> > ...
>> 
>> > > What's the use case?  How does an unprivileged user use the VPD
>> > > information?
>> > 
>> > We have to add new field keyword=value in VA section of VPD, which
>> > will indicate very specific sub-model for devices used as a bridge.
>> > 
>> > > I can certainly imagine using VPD for bug reporting, but that
>> > > would typically involve dmesg, dmidecode, lspci -vv, etc, all of
>> > > which already require privilege, so it's not clear to me how
>> > > public VPD info would help in that scenario.
>> > 
>> > I'm targeting other scenario - monitoring tool, which doesn't need
>> > root permissions for reading data. It needs to distinguish between
>> > NIC sub-models.
>> 
>> Maybe the driver could expose something in sysfs?  Maybe the driver
>> needs to know the sub-model as well, and reading VPD once in the
>> driver would make subsequent userspace sysfs reads trivial and fast.
>
> Our PCI driver lays in netdev subsystem and they have long-standing
> position do not allow any custom sysfs files. To be fair, we (RDMA)
> don't allow custom sysfs files too.
>
> Driver doesn't need to know this information as it is extra key=value in
> existing [VA] field, while driver relies on multiple FW capabilities
> to enable/disable functionality.
>
> Current [VA] line:
> "[VA] Vendor specific: 
> MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A"
> Future [VA] line:
> "[VA] Vendor specific: 
> MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A,SMDL=SOMETHING"
>
> Also the idea that we will duplicate existing functionality doesn't
> sound like a good approach to me, and there is no way that it is
> possible to expose as subsystem specific file.
>
> What about to allow existing VPD sysfs file to be readable for everyone 
> for our devices?
> And if this allow list grows to much, we will open it for all devices 
> in the world?

Bjorn,

I don't see this patch in https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next
So what did you decide? How can we enable existing VPD access to regular users?

Thanks

>
> Thanks
>
>> 
>> Bjorn

Bjorn Helgaas Nov. 7, 2024, 2:59 p.m. UTC | #11

On Thu, Nov 07, 2024 at 01:31:44PM +0200, Leon Romanovsky wrote:
> On Tue, Nov 5, 2024, at 18:26, Leon Romanovsky wrote:
> > On Tue, Nov 05, 2024 at 09:24:55AM -0600, Bjorn Helgaas wrote:
> >> On Tue, Nov 05, 2024 at 09:51:30AM +0200, Leon Romanovsky wrote:
> >> > On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
> >> > > On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
> >> > > > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
> >> > > > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
> >> > > > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
> >> > > > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
> >> > > > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
> >> > > > > > > > > From: Leon Romanovsky <[e-mail]>
> >> > > > > > > > > 
> >> > > > > > > > > The Virtual Product Data (VPD) attribute is not
> >> > > > > > > > > readable by regular user without root permissions.
> >> > > > > > > > > Such restriction is not really needed, as data
> >> > > > > > > > > presented in that VPD is not sensitive at all.
> >> > > > > > > > > 
> >> > > > > > > > > This change aligns the permissions of the VPD
> >> > > > > > > > > attribute to be accessible for read by all users,
> >> > > > > > > > > while write being restricted to root only.
> >> > ...
> >> 
> >> > > What's the use case?  How does an unprivileged user use the VPD
> >> > > information?
> >> > 
> >> > We have to add new field keyword=value in VA section of VPD, which
> >> > will indicate very specific sub-model for devices used as a bridge.
> >> > 
> >> > > I can certainly imagine using VPD for bug reporting, but that
> >> > > would typically involve dmesg, dmidecode, lspci -vv, etc, all of
> >> > > which already require privilege, so it's not clear to me how
> >> > > public VPD info would help in that scenario.
> >> > 
> >> > I'm targeting other scenario - monitoring tool, which doesn't need
> >> > root permissions for reading data. It needs to distinguish between
> >> > NIC sub-models.
> >> 
> >> Maybe the driver could expose something in sysfs?  Maybe the driver
> >> needs to know the sub-model as well, and reading VPD once in the
> >> driver would make subsequent userspace sysfs reads trivial and fast.
> >
> > Our PCI driver lays in netdev subsystem and they have long-standing
> > position do not allow any custom sysfs files. To be fair, we (RDMA)
> > don't allow custom sysfs files too.
> >
> > Driver doesn't need to know this information as it is extra key=value in
> > existing [VA] field, while driver relies on multiple FW capabilities
> > to enable/disable functionality.
> >
> > Current [VA] line:
> > "[VA] Vendor specific: 
> > MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A"
> > Future [VA] line:
> > "[VA] Vendor specific: 
> > MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A,SMDL=SOMETHING"
> >
> > Also the idea that we will duplicate existing functionality doesn't
> > sound like a good approach to me, and there is no way that it is
> > possible to expose as subsystem specific file.
> >
> > What about to allow existing VPD sysfs file to be readable for everyone 
> > for our devices?
> > And if this allow list grows to much, we will open it for all devices 
> > in the world?
> 
> Bjorn,
> 
> I don't see this patch in
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next
> So what did you decide? How can we enable existing VPD access to
> regular users?

I think it's too risky to enable VPD to be readable by all users.

Bjorn

Leon Romanovsky Nov. 7, 2024, 4:15 p.m. UTC | #12

On Thu, Nov 7, 2024, at 16:59, Bjorn Helgaas wrote:
> On Thu, Nov 07, 2024 at 01:31:44PM +0200, Leon Romanovsky wrote:
>> On Tue, Nov 5, 2024, at 18:26, Leon Romanovsky wrote:
>> > On Tue, Nov 05, 2024 at 09:24:55AM -0600, Bjorn Helgaas wrote:
>> >> On Tue, Nov 05, 2024 at 09:51:30AM +0200, Leon Romanovsky wrote:
>> >> > On Mon, Nov 04, 2024 at 06:10:27PM -0600, Bjorn Helgaas wrote:
>> >> > > On Sun, Nov 03, 2024 at 02:33:44PM +0200, Leon Romanovsky wrote:
>> >> > > > On Fri, Nov 01, 2024 at 11:47:37AM -0500, Bjorn Helgaas wrote:
>> >> > > > > On Fri, Nov 01, 2024 at 04:33:00PM +0200, Leon Romanovsky wrote:
>> >> > > > > > On Thu, Oct 31, 2024 at 06:22:52PM -0500, Bjorn Helgaas wrote:
>> >> > > > > > > On Tue, Oct 29, 2024 at 07:04:50PM -0500, Bjorn Helgaas wrote:
>> >> > > > > > > > On Mon, Oct 28, 2024 at 10:05:33AM +0200, Leon Romanovsky wrote:
>> >> > > > > > > > > From: Leon Romanovsky <[e-mail]>
>> >> > > > > > > > > 
>> >> > > > > > > > > The Virtual Product Data (VPD) attribute is not
>> >> > > > > > > > > readable by regular user without root permissions.
>> >> > > > > > > > > Such restriction is not really needed, as data
>> >> > > > > > > > > presented in that VPD is not sensitive at all.
>> >> > > > > > > > > 
>> >> > > > > > > > > This change aligns the permissions of the VPD
>> >> > > > > > > > > attribute to be accessible for read by all users,
>> >> > > > > > > > > while write being restricted to root only.
>> >> > ...
>> >> 
>> >> > > What's the use case?  How does an unprivileged user use the VPD
>> >> > > information?
>> >> > 
>> >> > We have to add new field keyword=value in VA section of VPD, which
>> >> > will indicate very specific sub-model for devices used as a bridge.
>> >> > 
>> >> > > I can certainly imagine using VPD for bug reporting, but that
>> >> > > would typically involve dmesg, dmidecode, lspci -vv, etc, all of
>> >> > > which already require privilege, so it's not clear to me how
>> >> > > public VPD info would help in that scenario.
>> >> > 
>> >> > I'm targeting other scenario - monitoring tool, which doesn't need
>> >> > root permissions for reading data. It needs to distinguish between
>> >> > NIC sub-models.
>> >> 
>> >> Maybe the driver could expose something in sysfs?  Maybe the driver
>> >> needs to know the sub-model as well, and reading VPD once in the
>> >> driver would make subsequent userspace sysfs reads trivial and fast.
>> >
>> > Our PCI driver lays in netdev subsystem and they have long-standing
>> > position do not allow any custom sysfs files. To be fair, we (RDMA)
>> > don't allow custom sysfs files too.
>> >
>> > Driver doesn't need to know this information as it is extra key=value in
>> > existing [VA] field, while driver relies on multiple FW capabilities
>> > to enable/disable functionality.
>> >
>> > Current [VA] line:
>> > "[VA] Vendor specific: 
>> > MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A"
>> > Future [VA] line:
>> > "[VA] Vendor specific: 
>> > MLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=CX713106A,SMDL=SOMETHING"
>> >
>> > Also the idea that we will duplicate existing functionality doesn't
>> > sound like a good approach to me, and there is no way that it is
>> > possible to expose as subsystem specific file.
>> >
>> > What about to allow existing VPD sysfs file to be readable for everyone 
>> > for our devices?
>> > And if this allow list grows to much, we will open it for all devices 
>> > in the world?
>> 
>> Bjorn,
>> 
>> I don't see this patch in
>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next
>> So what did you decide? How can we enable existing VPD access to
>> regular users?
>
> I think it's too risky to enable VPD to be readable by all users.

So what about to enable it for mlx5 devices?

Thanks

>
> Bjorn

Patch

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e4300f5f304f..2537685cac90 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -317,7 +317,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,