linux-kernel - Re: [RFC] PCI: sysfs: add bypass for config read admin check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yk1KveOnYfSrUJLD@kroah.com>
Date:   Wed, 6 Apr 2022 10:09:33 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     David Stevens <stevensd@...omium.org>
Cc:     linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC] PCI: sysfs: add bypass for config read admin check

On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
> From: David Stevens <stevensd@...omium.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@...omium.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