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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > ---
> > >  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