Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
@ 2020-10-16  5:52 Allen Pais
  2020-10-16  6:20 ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Allen Pais @ 2020-10-16  5:52 UTC (permalink / raw)
  To: linux-pci; +Cc: bhelgaas, ast, gregkh, linux-kernel, Allen Pais, Allen Pais

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(-)

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;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-16  5:52 [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO Allen Pais
@ 2020-10-16  6:20 ` Greg KH
  2020-10-19 13:00   ` Allen
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2020-10-16  6:20 UTC (permalink / raw)
  To: Allen Pais; +Cc: linux-pci, bhelgaas, ast, linux-kernel, Allen Pais, Allen Pais

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-16  6:20 ` Greg KH
@ 2020-10-19 13:00   ` Allen
  2020-10-19 13:16     ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Allen @ 2020-10-19 13:00 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-pci, bhelgaas, ast, Linux Kernel Mailing List, Allen Pais,
	Allen Pais

> >
> >  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



-- 
       - Allen

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-19 13:00   ` Allen
@ 2020-10-19 13:16     ` Greg KH
  2020-10-19 13:21       ` Allen
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2020-10-19 13:16 UTC (permalink / raw)
  To: Allen
  Cc: linux-pci, bhelgaas, ast, Linux Kernel Mailing List, Allen Pais,
	Allen Pais

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-19 13:16     ` Greg KH
@ 2020-10-19 13:21       ` Allen
  2020-10-19 13:47         ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Allen @ 2020-10-19 13:21 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-pci, Bjorn Helgaas, ast, Linux Kernel Mailing List,
	Allen Pais, Allen Pais

> > > >
> > > >  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.

-- 
       - Allen

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-19 13:21       ` Allen
@ 2020-10-19 13:47         ` Greg KH
  2020-10-19 14:32           ` Allen
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2020-10-19 13:47 UTC (permalink / raw)
  To: Allen
  Cc: linux-pci, Bjorn Helgaas, ast, Linux Kernel Mailing List,
	Allen Pais, Allen Pais

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO
  2020-10-19 13:47         ` Greg KH
@ 2020-10-19 14:32           ` Allen
  0 siblings, 0 replies; 7+ messages in thread
From: Allen @ 2020-10-19 14:32 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-pci, Bjorn Helgaas, ast, Linux Kernel Mailing List,
	Allen Pais, Allen Pais

> > > > > >  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



-- 
       - Allen

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16  5:52 [RFC] PCI: allow sysfs file owner to read the config space with CAP_SYS_RAWIO Allen Pais
2020-10-16  6:20 ` Greg KH
2020-10-19 13:00   ` Allen
2020-10-19 13:16     ` Greg KH
2020-10-19 13:21       ` Allen
2020-10-19 13:47         ` Greg KH
2020-10-19 14:32           ` Allen

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git