| Message ID | 20201016055235.440159-1-allen.lkml@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO | expand |
On Fri, Oct 16, 2020 at 11:22:35AM +0530, Allen Pais wrote: > From: Allen Pais <apais@linux.microsoft.com> > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > in order to read configuration space past the frist 64B. > > Since the path is only for reading, could we use CAP_SYS_RAWIO? Why? What needs this reduced capability? > This patch contains a simpler fix, I would love to hear from the > Maintainers on the approach. > > The other approach that I considered was to introduce and API > which would check for multiple capabilities, something similar to > perfmon_capable()/bpf_capable(). But I could not find more users > for the API and hence dropped it. > > The problem I am trying to solve is to avoid handing out > CAP_SYS_ADMIN for extended reads of the PCI config space. Who is reading this config space that doesn't have admin rights? And what are they doing with it? One big problem is that some devices will crash if you do this wrong, which is why we restricted it to root. Hopefully all of those devices are now gone, but I don't think you can count on it. The "guaranteed safe" fields in the config space are already exported by sysfs for all users to read, are they not sufficient? thanks, greg k-h
> > > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > > in order to read configuration space past the frist 64B. > > > > Since the path is only for reading, could we use CAP_SYS_RAWIO? > > Why? What needs this reduced capability? Thanks for the review. We need read access to /sys/bus/pci/devices/, We need write access to config, remove, rescan & enable files under the device directory for each PCIe functions & the downstream PCIe port. We need r/w access to sysfs to unbind and rebind the root complex. > > > This patch contains a simpler fix, I would love to hear from the > > Maintainers on the approach. > > > > The other approach that I considered was to introduce and API > > which would check for multiple capabilities, something similar to > > perfmon_capable()/bpf_capable(). But I could not find more users > > for the API and hence dropped it. > > > > The problem I am trying to solve is to avoid handing out > > CAP_SYS_ADMIN for extended reads of the PCI config space. > > Who is reading this config space that doesn't have admin rights? And > what are they doing with it? > > One big problem is that some devices will crash if you do this wrong, > which is why we restricted it to root. Hopefully all of those devices > are now gone, but I don't think you can count on it. > > The "guaranteed safe" fields in the config space are already exported by > sysfs for all users to read, are they not sufficient? > > thanks, > > greg k-h
On Mon, Oct 19, 2020 at 06:30:16PM +0530, Allen wrote: > > > > > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > > > in order to read configuration space past the frist 64B. > > > > > > Since the path is only for reading, could we use CAP_SYS_RAWIO? > > > > Why? What needs this reduced capability? > > Thanks for the review. > > We need read access to /sys/bus/pci/devices/, We need write access to config, > remove, rescan & enable files under the device directory for each PCIe > functions & the downstream PCIe port. > > We need r/w access to sysfs to unbind and rebind the root complex. That didn't answer my question at all. Why can't you have the process that wants to do all of the above, have admin rights as well? Doing all of that is _very_ low-level and can cause all sorts of horrible things to happen to your machine, and is not really "raw io" in the traditional sense at all, right? greg k-h
> > > > > > > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > > > > in order to read configuration space past the frist 64B. > > > > > > > > Since the path is only for reading, could we use CAP_SYS_RAWIO? > > > > > > Why? What needs this reduced capability? > > > > Thanks for the review. > > > > We need read access to /sys/bus/pci/devices/, We need write access to config, > > remove, rescan & enable files under the device directory for each PCIe > > functions & the downstream PCIe port. > > > > We need r/w access to sysfs to unbind and rebind the root complex. > > That didn't answer my question at all. Sorry about that, breaking it down: When the machine first boots, the VFIO device bindings under /dev/vfio are not present. root@localhost:/tmp# ls -l /dev/vfio/ total 0 crw-rw-rw-. 1 root root 10, 196 Jan 5 01:47 vfio We have an agent which needs to run the following commands (We get access denied here and need permissions to do this). echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id And we want to avoid handing CAP_SYS_ADMIN here. Which is why the thought about CAP_SYS_RAWIO. > > Why can't you have the process that wants to do all of the above, have > admin rights as well? Doing all of that is _very_ low-level and can > cause all sorts of horrible things to happen to your machine, and is not > really "raw io" in the traditional sense at all, right? If the above approach is going to cause the system to do horrible things, then I'll drop the idea.
On Mon, Oct 19, 2020 at 06:51:39PM +0530, Allen wrote: > > > > > > > > > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > > > > > in order to read configuration space past the frist 64B. > > > > > > > > > > Since the path is only for reading, could we use CAP_SYS_RAWIO? > > > > > > > > Why? What needs this reduced capability? > > > > > > Thanks for the review. > > > > > > We need read access to /sys/bus/pci/devices/, We need write access to config, > > > remove, rescan & enable files under the device directory for each PCIe > > > functions & the downstream PCIe port. > > > > > > We need r/w access to sysfs to unbind and rebind the root complex. > > > > That didn't answer my question at all. > > Sorry about that, breaking it down: > > When the machine first boots, the VFIO device bindings under /dev/vfio > are not present. > > root@localhost:/tmp# ls -l /dev/vfio/ > total 0 > crw-rw-rw-. 1 root root 10, 196 Jan 5 01:47 vfio > > We have an agent which needs to run the following commands (We get > access denied here and need permissions to do this). > echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id > echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id > > And we want to avoid handing CAP_SYS_ADMIN here. Which is why the > thought about CAP_SYS_RAWIO. But that is not what you were asking this patch to do at all. So why bring it up? new_id is NOT for "raw io" control, that should be only for admin priviliges. And just because the vfio driver "abuses" this traditionally-debug-functionality doesn't mean you get to abuse the permission levels either. > > Why can't you have the process that wants to do all of the above, have > > admin rights as well? Doing all of that is _very_ low-level and can > > cause all sorts of horrible things to happen to your machine, and is not > > really "raw io" in the traditional sense at all, right? > > > If the above approach is going to cause the system to do horrible things, > then I'll drop the idea. Of course it can cause the system to do horrible things, try it yourself and see! greg k-h
> > > > > > Access to pci config space is explictly checked with CAP_SYS_ADMIN > > > > > > in order to read configuration space past the frist 64B. > > > > > > > > > > > > Since the path is only for reading, could we use CAP_SYS_RAWIO? > > > > > > > > > > Why? What needs this reduced capability? > > > > > > > > Thanks for the review. > > > > > > > > We need read access to /sys/bus/pci/devices/, We need write access to config, > > > > remove, rescan & enable files under the device directory for each PCIe > > > > functions & the downstream PCIe port. > > > > > > > > We need r/w access to sysfs to unbind and rebind the root complex. > > > > > > That didn't answer my question at all. > > > > Sorry about that, breaking it down: > > > > When the machine first boots, the VFIO device bindings under /dev/vfio > > are not present. > > > > root@localhost:/tmp# ls -l /dev/vfio/ > > total 0 > > crw-rw-rw-. 1 root root 10, 196 Jan 5 01:47 vfio > > > > We have an agent which needs to run the following commands (We get > > access denied here and need permissions to do this). > > echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id > > echo -n xxxx yyyy > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id > > > > And we want to avoid handing CAP_SYS_ADMIN here. Which is why the > > thought about CAP_SYS_RAWIO. > > But that is not what you were asking this patch to do at all. So why > bring it up? > > new_id is NOT for "raw io" control, that should be only for admin > priviliges. Okay. Thanks for the explanation. > > And just because the vfio driver "abuses" this > traditionally-debug-functionality doesn't mean you get to abuse the > permission levels either. This makes sense now. I will drop the patch. Thank you very much for the review. > > > > Why can't you have the process that wants to do all of the above, have > > > admin rights as well? Doing all of that is _very_ low-level and can > > > cause all sorts of horrible things to happen to your machine, and is not > > > really "raw io" in the traditional sense at all, right? > > > > > > If the above approach is going to cause the system to do horrible things, > > then I'll drop the idea. > > Of course it can cause the system to do horrible things, try it yourself > and see! > > greg k-h
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 6d78df981d41..6574c0203475 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -666,7 +666,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 (file_ns_capable(filp, &init_user_ns, CAP_SYS_ADMIN) || + file_ns_capable(filp, &init_user_ns, CAP_SYS_RAWIO)) size = dev->cfg_size; else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) size = 128;