Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 6 Apr 2022 10:09:33 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     David Stevens <stevensd@chromium.org>
Cc:     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: <Yk1KveOnYfSrUJLD@kroah.com>
References: <20220406071131.2930035-1-stevensd@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220406071131.2930035-1-stevensd@google.com>
Precedence: bulk

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