diff mbox series

[RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO

Message ID 20201016055235.440159-1-allen.lkml@gmail.com
State Not Applicable
Delegated to: Bjorn Helgaas
Headers show
Series [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO | expand

Commit Message

Allen Pais Oct. 16, 2020, 5:52 a.m. UTC
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?
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.

Signed-off-by: Allen Pais <allen.pais@lkml.com>
---
 drivers/pci/pci-sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Greg KH Oct. 16, 2020, 6:20 a.m. UTC | #1
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
Allen Pais Oct. 19, 2020, 1 p.m. UTC | #2
> >
> >  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
Greg KH Oct. 19, 2020, 1:16 p.m. UTC | #3
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
Allen Pais Oct. 19, 2020, 1:21 p.m. UTC | #4
> > > >
> > > >  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.
Greg KH Oct. 19, 2020, 1:47 p.m. UTC | #5
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
Allen Pais Oct. 19, 2020, 2:32 p.m. UTC | #6
> > > > > >  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 mbox series

Patch

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;