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
