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

[Kees Cook]

On April 6, 2022 4:17:51 AM PDT, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>[+cc Kees]
>
>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.
>
>Can you expand this a bit to explain the purpose of this?  I guess it
>makes "lspci -v" work without having to be root?  How much of a
>problem is that?  Is there some specific use case that needs this
>change?  Maybe there's some way to address that without having to add
>a new parameter that bypasses CAP_SYS_ADMIN.

Yeah, this doesn't seem right to me. There are tons of ways in userspace to deal with these permissions (e.g. sudo with lspci, suid wrapper, etc).

-Kees


>
>> 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.");
>> +
>>  /* 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))
>>  		size = dev->cfg_size;
>>  	else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
>>  		size = 128;
>> -- 
>> 2.35.1.1094.g7c7d902a7c-goog
>> 

-- 
Kees Cook