Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 08 Apr 2022 08:50:24 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Bjorn Helgaas <helgaas@kernel.org>,
        David Stevens <stevensd@chromium.org>
CC:     linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC] PCI: sysfs: add bypass for config read admin check
User-Agent: K-9 Mail for Android
In-Reply-To: <20220406111751.GA132418@bhelgaas>
References: <20220406111751.GA132418@bhelgaas>
Message-ID: <C36237FA-F23D-4C52-BCB5-1D05D86ED9F4@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On April 6, 2022 4:17:51 AM PDT, Bjorn Helgaas <helgaas@kernel=2Eorg> wrot=
e:
>[+cc Kees]
>
>On Wed, Apr 06, 2022 at 04:11:31PM +0900, David Stevens wrote:
>> From: David Stevens <stevensd@chromium=2Eorg>
>>=20
>> 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=2E This allows systems without problematic hardware to=
 be
>> configured to allow users without CAP_SYS_ADMIN to read PCI
>> capabilities=2E
>
>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=2E

Yeah, this doesn't seem right to me=2E There are tons of ways in userspace=
 to deal with these permissions (e=2Eg=2E sudo with lspci, suid wrapper, et=
c)=2E

-Kees


>
>> Signed-off-by: David Stevens <stevensd@chromium=2Eorg>
>> ---
>>  drivers/pci/pci-sysfs=2Ec | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/drivers/pci/pci-sysfs=2Ec b/drivers/pci/pci-sysfs=2Ec
>> index 602f0fb0b007=2E=2E162423b3c052 100644
>> --- a/drivers/pci/pci-sysfs=2Ec
>> +++ b/drivers/pci/pci-sysfs=2Ec
>> @@ -28,10 +28,17 @@
>>  #include <linux/pm_runtime=2Eh>
>>  #include <linux/msi=2Eh>
>>  #include <linux/of=2Eh>
>> +#include <linux/moduleparam=2Eh>
>>  #include "pci=2Eh"
>> =20
>>  static int sysfs_initialized;	/* =3D 0 */
>> =20
>> +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=2E")=
;
>> +
>>  /* 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, s=
truct kobject *kobj,
>>  	u8 *data =3D (u8 *) buf;
>> =20
>>  	/* 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 =3D dev->cfg_size;
>>  	else if (dev->hdr_type =3D=3D PCI_HEADER_TYPE_CARDBUS)
>>  		size =3D 128;
>> --=20
>> 2=2E35=2E1=2E1094=2Eg7c7d902a7c-goog
>>=20

--=20
Kees Cook