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@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.

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