[RFC] PCI: sysfs: add bypass for config read admin check

[RFC] PCI: sysfs: add bypass for config read admin check

[David Stevens]

From: David Stevens <stevensd@chromium.org>

Add a moduleparam that can be set to bypass the check that limits users
without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
the config space. This allows systems without problematic hardware to be
configured to allow users without CAP_SYS_ADMIN to read PCI
capabilities.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/pci/pci-sysfs.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 602f0fb0b007..162423b3c052 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -28,10 +28,17 @@
 #include <linux/pm_runtime.h>
 #include <linux/msi.h>
 #include <linux/of.h>
+#include <linux/moduleparam.h>
 #include "pci.h"
 
 static int sysfs_initialized;	/* = 0 */
 
+static bool allow_unsafe_config_reads;
+module_param_named(allow_unsafe_config_reads,
+		   allow_unsafe_config_reads, bool, 0644);
+MODULE_PARM_DESC(allow_unsafe_config_reads,
+		 "Enable full read access to config space without CAP_SYS_ADMIN.");
+
 /* show configuration fields */
 #define pci_config_attr(field, format_string)				\
 static ssize_t								\
@@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
 	u8 *data = (u8 *) buf;
 
 	/* Several chips lock up trying to read undefined config space */
-	if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
+	if (allow_unsafe_config_reads ||
+	    file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
 		size = dev->cfg_size;
 	else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
 		size = 128;
-- 
2.35.1.1094.g7c7d902a7c-goog

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[Greg Kroah-Hartman]

On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> Add a moduleparam that can be set to bypass the check that limits users
> without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
> the config space. This allows systems without problematic hardware to be
> configured to allow users without CAP_SYS_ADMIN to read PCI
> capabilities.
> 
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  drivers/pci/pci-sysfs.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 602f0fb0b007..162423b3c052 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -28,10 +28,17 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
> +#include <linux/moduleparam.h>
>  #include "pci.h"
>  
>  static int sysfs_initialized;	/* = 0 */
>  
> +static bool allow_unsafe_config_reads;
> +module_param_named(allow_unsafe_config_reads,
> +		   allow_unsafe_config_reads, bool, 0644);
> +MODULE_PARM_DESC(allow_unsafe_config_reads,
> +		 "Enable full read access to config space without CAP_SYS_ADMIN.");

No, this is not the 1990's, please do not add system-wide module
parameters like this.  Especially ones that circumvent security
protections.

Also, where did you document this new option?

Why not just add this to a LSM instead?

>  /* show configuration fields */
>  #define pci_config_attr(field, format_string)				\
>  static ssize_t								\
> @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>  	u8 *data = (u8 *) buf;
>  
>  	/* Several chips lock up trying to read undefined config space */
> -	if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> +	if (allow_unsafe_config_reads ||
> +	    file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))

This feels really dangerous.  What benifit are you getting here by
allowing an unpriviliged user to read this information?

What userspace problem are you trying to solve here that deserves this
change?

thanks,

greg k-h

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[Pali Rohár]

On Wednesday 06 April 2022 10:09:33 Greg Kroah-Hartman wrote:
> On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> > From: David Stevens <stevensd@chromium.org>
> > 
> > Add a moduleparam that can be set to bypass the check that limits users
> > without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
> > the config space. This allows systems without problematic hardware to be
> > configured to allow users without CAP_SYS_ADMIN to read PCI
> > capabilities.
> > 
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> >  drivers/pci/pci-sysfs.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 602f0fb0b007..162423b3c052 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -28,10 +28,17 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/msi.h>
> >  #include <linux/of.h>
> > +#include <linux/moduleparam.h>
> >  #include "pci.h"
> >  
> >  static int sysfs_initialized;	/* = 0 */
> >  
> > +static bool allow_unsafe_config_reads;
> > +module_param_named(allow_unsafe_config_reads,
> > +		   allow_unsafe_config_reads, bool, 0644);
> > +MODULE_PARM_DESC(allow_unsafe_config_reads,
> > +		 "Enable full read access to config space without CAP_SYS_ADMIN.");
> 
> No, this is not the 1990's, please do not add system-wide module
> parameters like this.  Especially ones that circumvent security
> protections.
> 
> Also, where did you document this new option?
> 
> Why not just add this to a LSM instead?
> 
> >  /* show configuration fields */
> >  #define pci_config_attr(field, format_string)				\
> >  static ssize_t								\
> > @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> >  	u8 *data = (u8 *) buf;
> >  
> >  	/* Several chips lock up trying to read undefined config space */
> > -	if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> > +	if (allow_unsafe_config_reads ||
> > +	    file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> 
> This feels really dangerous.  What benifit are you getting here by
> allowing an unpriviliged user to read this information?

Hello! This is really dangerous.

Nowadays operating systems are in progress to completely disallow PCI
config space access from userspace. So doing opposite thing and even
enable it for unprivileged users in Linux is hazard.

For example NT kernel in Windows 11 already completely disallowed access
to PCI config space from userspace unless NT kernel is booted in mode
for local debugging with disabled UEFI secure boot. And access in this
case is only for highly privileged processes (debug privilege in access
token).

So... should not we move into same direction like other operating system
and start disallowing access to PCI config space from userspace
completely too? For example when kernel lockdown feature is enabled?

In PCI config space of some devices are stored also non-PCI registers
and accessing them was not really mean for userspace and for sure not
for unprivileged users. On AMD x86 platforms is into PCI config space of
some device mapped for example CPU MSR registers (at fixed offset, after
linked listed of capabilities). Probably in Intel x86 is something
similar too. On Synopsis Designware based PCIe HW is into PCI config
space of Root Port mapped large range of IP configuration registers.

So "This allows systems without problematic hardware" means that such
system must be non-AMD, non-Designware and probably also non-Intel.

> What userspace problem are you trying to solve here that deserves this
> change?
> 
> thanks,
> 
> greg k-h

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[David Stevens]

On Wed, Apr 6, 2022 at 10:13 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 06 April 2022 10:09:33 Greg Kroah-Hartman wrote:
> > On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> > > From: David Stevens <stevensd@chromium.org>
> > >
> > > Add a moduleparam that can be set to bypass the check that limits users
> > > without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
> > > the config space. This allows systems without problematic hardware to be
> > > configured to allow users without CAP_SYS_ADMIN to read PCI
> > > capabilities.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > ---
> > >  drivers/pci/pci-sysfs.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 602f0fb0b007..162423b3c052 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -28,10 +28,17 @@
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/msi.h>
> > >  #include <linux/of.h>
> > > +#include <linux/moduleparam.h>
> > >  #include "pci.h"
> > >
> > >  static int sysfs_initialized;      /* = 0 */
> > >
> > > +static bool allow_unsafe_config_reads;
> > > +module_param_named(allow_unsafe_config_reads,
> > > +              allow_unsafe_config_reads, bool, 0644);
> > > +MODULE_PARM_DESC(allow_unsafe_config_reads,
> > > +            "Enable full read access to config space without CAP_SYS_ADMIN.");
> >
> > No, this is not the 1990's, please do not add system-wide module
> > parameters like this.  Especially ones that circumvent security
> > protections.
> >
> > Also, where did you document this new option?
> >
> > Why not just add this to a LSM instead?
> >
> > >  /* show configuration fields */
> > >  #define pci_config_attr(field, format_string)                              \
> > >  static ssize_t                                                             \
> > > @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> > >     u8 *data = (u8 *) buf;
> > >
> > >     /* Several chips lock up trying to read undefined config space */
> > > -   if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> > > +   if (allow_unsafe_config_reads ||
> > > +       file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> >
> > This feels really dangerous.  What benifit are you getting here by
> > allowing an unpriviliged user to read this information?
>
> Hello! This is really dangerous.
>
> Nowadays operating systems are in progress to completely disallow PCI
> config space access from userspace. So doing opposite thing and even
> enable it for unprivileged users in Linux is hazard.
>
> For example NT kernel in Windows 11 already completely disallowed access
> to PCI config space from userspace unless NT kernel is booted in mode
> for local debugging with disabled UEFI secure boot. And access in this
> case is only for highly privileged processes (debug privilege in access
> token).
>
> So... should not we move into same direction like other operating system
> and start disallowing access to PCI config space from userspace
> completely too? For example when kernel lockdown feature is enabled?
>
> In PCI config space of some devices are stored also non-PCI registers
> and accessing them was not really mean for userspace and for sure not
> for unprivileged users. On AMD x86 platforms is into PCI config space of
> some device mapped for example CPU MSR registers (at fixed offset, after
> linked listed of capabilities). Probably in Intel x86 is something
> similar too. On Synopsis Designware based PCIe HW is into PCI config
> space of Root Port mapped large range of IP configuration registers.
>
> So "This allows systems without problematic hardware" means that such
> system must be non-AMD, non-Designware and probably also non-Intel.

It's interesting to hear that what seems to have been added 18 years
ago as a safeguard against faulty hardware (i.e. "Several chips lock
up trying to read undefined config space") has become a load bearing
security check. I guess because that check is there, it wasn't worth
adding a LOCKDOWN_PCI_ACCESS check to pci_read_config?

Regardless, I've found that with a bit of work in userspace, vfio is
sufficient for my use case.

-David

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[Greg Kroah-Hartman]

On Tue, Apr 12, 2022 at 04:51:06PM +0900, David Stevens wrote:
> On Wed, Apr 6, 2022 at 10:13 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Wednesday 06 April 2022 10:09:33 Greg Kroah-Hartman wrote:
> > > On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> > > > From: David Stevens <stevensd@chromium.org>
> > > >
> > > > Add a moduleparam that can be set to bypass the check that limits users
> > > > without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
> > > > the config space. This allows systems without problematic hardware to be
> > > > configured to allow users without CAP_SYS_ADMIN to read PCI
> > > > capabilities.
> > > >
> > > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > > ---
> > > >  drivers/pci/pci-sysfs.c | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > index 602f0fb0b007..162423b3c052 100644
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -28,10 +28,17 @@
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/msi.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/moduleparam.h>
> > > >  #include "pci.h"
> > > >
> > > >  static int sysfs_initialized;      /* = 0 */
> > > >
> > > > +static bool allow_unsafe_config_reads;
> > > > +module_param_named(allow_unsafe_config_reads,
> > > > +              allow_unsafe_config_reads, bool, 0644);
> > > > +MODULE_PARM_DESC(allow_unsafe_config_reads,
> > > > +            "Enable full read access to config space without CAP_SYS_ADMIN.");
> > >
> > > No, this is not the 1990's, please do not add system-wide module
> > > parameters like this.  Especially ones that circumvent security
> > > protections.
> > >
> > > Also, where did you document this new option?
> > >
> > > Why not just add this to a LSM instead?
> > >
> > > >  /* show configuration fields */
> > > >  #define pci_config_attr(field, format_string)                              \
> > > >  static ssize_t                                                             \
> > > > @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> > > >     u8 *data = (u8 *) buf;
> > > >
> > > >     /* Several chips lock up trying to read undefined config space */
> > > > -   if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> > > > +   if (allow_unsafe_config_reads ||
> > > > +       file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> > >
> > > This feels really dangerous.  What benifit are you getting here by
> > > allowing an unpriviliged user to read this information?
> >
> > Hello! This is really dangerous.
> >
> > Nowadays operating systems are in progress to completely disallow PCI
> > config space access from userspace. So doing opposite thing and even
> > enable it for unprivileged users in Linux is hazard.
> >
> > For example NT kernel in Windows 11 already completely disallowed access
> > to PCI config space from userspace unless NT kernel is booted in mode
> > for local debugging with disabled UEFI secure boot. And access in this
> > case is only for highly privileged processes (debug privilege in access
> > token).
> >
> > So... should not we move into same direction like other operating system
> > and start disallowing access to PCI config space from userspace
> > completely too? For example when kernel lockdown feature is enabled?
> >
> > In PCI config space of some devices are stored also non-PCI registers
> > and accessing them was not really mean for userspace and for sure not
> > for unprivileged users. On AMD x86 platforms is into PCI config space of
> > some device mapped for example CPU MSR registers (at fixed offset, after
> > linked listed of capabilities). Probably in Intel x86 is something
> > similar too. On Synopsis Designware based PCIe HW is into PCI config
> > space of Root Port mapped large range of IP configuration registers.
> >
> > So "This allows systems without problematic hardware" means that such
> > system must be non-AMD, non-Designware and probably also non-Intel.
> 
> It's interesting to hear that what seems to have been added 18 years
> ago as a safeguard against faulty hardware (i.e. "Several chips lock
> up trying to read undefined config space") has become a load bearing
> security check.

Evolution is a wonderful thing :)

Security models are different today than they used to be back then, and
we still have broken hardware that will lock up with accesses like this.

> I guess because that check is there, it wasn't worth
> adding a LOCKDOWN_PCI_ACCESS check to pci_read_config?

That's up to the lockdown people to add, if they so desire.

> Regardless, I've found that with a bit of work in userspace, vfio is
> sufficient for my use case.

That sounds crazy, and probably wrong.  Good luck!  :)

greg k-h

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[Bjorn Helgaas]

[+cc Kees]

On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> Add a moduleparam that can be set to bypass the check that limits users
> without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
> the config space. This allows systems without problematic hardware to be
> configured to allow users without CAP_SYS_ADMIN to read PCI
> capabilities.

Can you expand this a bit to explain the purpose of this?  I guess it
makes "lspci -v" work without having to be root?  How much of a
problem is that?  Is there some specific use case that needs this
change?  Maybe there's some way to address that without having to add
a new parameter that bypasses CAP_SYS_ADMIN.

> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  drivers/pci/pci-sysfs.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 602f0fb0b007..162423b3c052 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -28,10 +28,17 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
> +#include <linux/moduleparam.h>
>  #include "pci.h"
>  
>  static int sysfs_initialized;	/* = 0 */
>  
> +static bool allow_unsafe_config_reads;
> +module_param_named(allow_unsafe_config_reads,
> +		   allow_unsafe_config_reads, bool, 0644);
> +MODULE_PARM_DESC(allow_unsafe_config_reads,
> +		 "Enable full read access to config space without CAP_SYS_ADMIN.");
> +
>  /* show configuration fields */
>  #define pci_config_attr(field, format_string)				\
>  static ssize_t								\
> @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>  	u8 *data = (u8 *) buf;
>  
>  	/* Several chips lock up trying to read undefined config space */
> -	if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
> +	if (allow_unsafe_config_reads ||
> +	    file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
>  		size = dev->cfg_size;
>  	else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
>  		size = 128;
> -- 
> 2.35.1.1094.g7c7d902a7c-goog
> 

Re: [RFC] PCI: sysfs: add bypass for config read admin check

[Kees Cook]

On April 6, 2022 4:17:51 AM PDT, Bjorn Helgaas <helgaas@kernel.org> wrote:
>[+cc Kees]
>
>On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
>> From: David Stevens <stevensd@chromium.org>
>> 
>> Add a moduleparam that can be set to bypass the check that limits users
>> without CAP_SYS_ADMIN to only being able to read the first 64 bytes of
>> the config space. This allows systems without problematic hardware to be
>> configured to allow users without CAP_SYS_ADMIN to read PCI
>> capabilities.
>
>Can you expand this a bit to explain the purpose of this?  I guess it
>makes "lspci -v" work without having to be root?  How much of a
>problem is that?  Is there some specific use case that needs this
>change?  Maybe there's some way to address that without having to add
>a new parameter that bypasses CAP_SYS_ADMIN.

Yeah, this doesn't seem right to me. There are tons of ways in userspace to deal with these permissions (e.g. sudo with lspci, suid wrapper, etc).

-Kees


>
>> Signed-off-by: David Stevens <stevensd@chromium.org>
>> ---
>>  drivers/pci/pci-sysfs.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 602f0fb0b007..162423b3c052 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -28,10 +28,17 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/msi.h>
>>  #include <linux/of.h>
>> +#include <linux/moduleparam.h>
>>  #include "pci.h"
>>  
>>  static int sysfs_initialized;	/* = 0 */
>>  
>> +static bool allow_unsafe_config_reads;
>> +module_param_named(allow_unsafe_config_reads,
>> +		   allow_unsafe_config_reads, bool, 0644);
>> +MODULE_PARM_DESC(allow_unsafe_config_reads,
>> +		 "Enable full read access to config space without CAP_SYS_ADMIN.");
>> +
>>  /* show configuration fields */
>>  #define pci_config_attr(field, format_string)				\
>>  static ssize_t								\
>> @@ -696,7 +703,8 @@ static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>>  	u8 *data = (u8 *) buf;
>>  
>>  	/* Several chips lock up trying to read undefined config space */
>> -	if (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
>> +	if (allow_unsafe_config_reads ||
>> +	    file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN))
>>  		size = dev->cfg_size;
>>  	else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
>>  		size = 128;
>> -- 
>> 2.35.1.1094.g7c7d902a7c-goog
>> 

-- 
Kees Cook