Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 6 Apr 2022 15:13:00 +0200
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     David Stevens <stevensd@chromium.org>, linux-pci@vger.kernel.org,
        Bjorn Helgaas <bhelgaas@google.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC] PCI: sysfs: add bypass for config read admin check
Message-ID: <20220406131300.7pgdcpdhexwvczsb@pali>
References: <20220406071131.2930035-1-stevensd@google.com>
 <Yk1KveOnYfSrUJLD@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yk1KveOnYfSrUJLD@kroah.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

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