[RFC PATCH] sysfs: bin_attr permission checking

[RFC PATCH] sysfs: bin_attr permission checking

[Chris Wright]

The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
check to verify privileges before allowing a user to read device
dependent config space.  This is meant to protect from an unprivileged
user potentially locking up the box.

When assigning a PCI device directly to a guest with libvirt and KVM, the 
sysfs config space file is chown'd to the user that the KVM guest will
run as.  The guest needs to have full access to the device's config
space since it's responsible for driving the device.  However, despite
being the owner of the sysfs file, the CAP_SYS_ADMIN check will not
allow read access beyond the config header.

This patch adds a new bin_attr->read_file() callback which adds a struct
file to the normal argument list.  This allows an implementation such as
PCI config space bin_attr read_file handler to check both inode
permission as well as privileges to determine whether to allow
privileged actions through the handler.

This is just RFC, although I've tested that it does allow the chown +
read to work as expected.  Any other ideas of how to handle this are
welcome.

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-sysfs.c |   13 ++++++++-----
 fs/sysfs/bin.c          |   16 +++++++++-------
 include/linux/sysfs.h   |    2 ++
 3 files changed, 19 insertions(+), 12 deletions(-)

--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/capability.h>
 #include <linux/pci-aspm.h>
+#include <linux/fs.h>
 #include "pci.h"
 
 static int sysfs_initialized;	/* = 0 */
@@ -356,16 +357,18 @@ boot_vga_show(struct device *dev, struct
 struct device_attribute vga_attr = __ATTR_RO(boot_vga);
 
 static ssize_t
-pci_read_config(struct kobject *kobj, struct bin_attribute *bin_attr,
-		char *buf, loff_t off, size_t count)
+pci_read_config(struct file *file, struct kobject *kobj,
+		struct bin_attribute *bin_attr, char *buf, loff_t off,
+		size_t count)
 {
 	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
+	struct inode *inode = file->f_path.dentry->d_inode;
 	unsigned int size = 64;
 	loff_t init_off = off;
 	u8 *data = (u8*) buf;
 
 	/* Several chips lock up trying to read undefined config space */
-	if (capable(CAP_SYS_ADMIN)) {
+	if (capable(CAP_SYS_ADMIN) || is_owner_or_cap(inode)) {
 		size = dev->cfg_size;
 	} else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 		size = 128;
@@ -924,7 +927,7 @@ static struct bin_attribute pci_config_a
 		.mode = S_IRUGO | S_IWUSR,
 	},
 	.size = PCI_CFG_SPACE_SIZE,
-	.read = pci_read_config,
+	.read_file = pci_read_config,
 	.write = pci_write_config,
 };
 
@@ -934,7 +937,7 @@ static struct bin_attribute pcie_config_
 		.mode = S_IRUGO | S_IWUSR,
 	},
 	.size = PCI_CFG_SPACE_EXP_SIZE,
-	.read = pci_read_config,
+	.read_file = pci_read_config,
 	.write = pci_write_config,
 };
 
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -46,9 +46,9 @@ struct bin_buffer {
 };
 
 static int
-fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
+fill_read(struct file *file, char *buffer, loff_t off, size_t count)
 {
-	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
+	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
 	struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
 	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
 	int rc;
@@ -58,7 +58,9 @@ fill_read(struct dentry *dentry, char *b
 		return -ENODEV;
 
 	rc = -EIO;
-	if (attr->read)
+	if (attr->read_file)
+		rc = attr->read_file(file, kobj, attr, buffer, off, count);
+	else if (attr->read)
 		rc = attr->read(kobj, attr, buffer, off, count);
 
 	sysfs_put_active_two(attr_sd);
@@ -70,8 +72,7 @@ static ssize_t
 read(struct file *file, char __user *userbuf, size_t bytes, loff_t *off)
 {
 	struct bin_buffer *bb = file->private_data;
-	struct dentry *dentry = file->f_path.dentry;
-	int size = dentry->d_inode->i_size;
+	int size = file->f_path.dentry->d_inode->i_size;
 	loff_t offs = *off;
 	int count = min_t(size_t, bytes, PAGE_SIZE);
 	char *temp;
@@ -92,7 +93,7 @@ read(struct file *file, char __user *use
 
 	mutex_lock(&bb->mutex);
 
-	count = fill_read(dentry, bb->buffer, offs, count);
+	count = fill_read(file, bb->buffer, offs, count);
 	if (count < 0) {
 		mutex_unlock(&bb->mutex);
 		goto out_free;
@@ -405,7 +406,8 @@ static int open(struct inode * inode, st
 	error = -EACCES;
 	if ((file->f_mode & FMODE_WRITE) && !(attr->write || attr->mmap))
 		goto err_out;
-	if ((file->f_mode & FMODE_READ) && !(attr->read || attr->mmap))
+	if ((file->f_mode & FMODE_READ) &&
+	    !(attr->read || attr->read_file || attr->mmap))
 		goto err_out;
 
 	error = -ENOMEM;
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -66,6 +66,8 @@ struct bin_attribute {
 	struct attribute	attr;
 	size_t			size;
 	void			*private;
+	ssize_t (*read_file)(struct file *,struct kobject *, struct bin_attribute *,
+			char *, loff_t, size_t);
 	ssize_t (*read)(struct kobject *, struct bin_attribute *,
 			char *, loff_t, size_t);
 	ssize_t (*write)(struct kobject *, struct bin_attribute *,

Re: [RFC PATCH] sysfs: bin_attr permission checking

[Greg KH]

On Wed, May 12, 2010 at 11:47:13AM -0700, Chris Wright wrote:
> The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
> check to verify privileges before allowing a user to read device
> dependent config space.  This is meant to protect from an unprivileged
> user potentially locking up the box.
> 
> When assigning a PCI device directly to a guest with libvirt and KVM, the 
> sysfs config space file is chown'd to the user that the KVM guest will
> run as.  The guest needs to have full access to the device's config
> space since it's responsible for driving the device.  However, despite
> being the owner of the sysfs file, the CAP_SYS_ADMIN check will not
> allow read access beyond the config header.
> 
> This patch adds a new bin_attr->read_file() callback which adds a struct
> file to the normal argument list.  This allows an implementation such as
> PCI config space bin_attr read_file handler to check both inode
> permission as well as privileges to determine whether to allow
> privileged actions through the handler.

Ick, this is all because we like showing different information if the
user is "privileged or not" :(

Turns out, that this probably isn't the best user api to implement,
remind me never to do that again...

> This is just RFC, although I've tested that it does allow the chown +
> read to work as expected.  Any other ideas of how to handle this are
> welcome.

Can we just pass in the 'file' for all users of the bin files instead of
the dentry?  You can always get the dentry from the file (as your patch
showes), and there isn't that many users of this interface.  I'd really
rather not have two different types of callbacks here.

thanks,

greg k-h

Re: [RFC PATCH] sysfs: bin_attr permission checking

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Wed, May 12, 2010 at 11:47:13AM -0700, Chris Wright wrote:
> > The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
> > check to verify privileges before allowing a user to read device
> > dependent config space.  This is meant to protect from an unprivileged
> > user potentially locking up the box.
> > 
> > When assigning a PCI device directly to a guest with libvirt and KVM, the 
> > sysfs config space file is chown'd to the user that the KVM guest will
> > run as.  The guest needs to have full access to the device's config
> > space since it's responsible for driving the device.  However, despite
> > being the owner of the sysfs file, the CAP_SYS_ADMIN check will not
> > allow read access beyond the config header.
> > 
> > This patch adds a new bin_attr->read_file() callback which adds a struct
> > file to the normal argument list.  This allows an implementation such as
> > PCI config space bin_attr read_file handler to check both inode
> > permission as well as privileges to determine whether to allow
> > privileged actions through the handler.
> 
> Ick, this is all because we like showing different information if the
> user is "privileged or not" :(

yup

> Turns out, that this probably isn't the best user api to implement,
> remind me never to do that again...

Yeah, it's challenging to deal with.  Alternative here is a new config
sysfs entry that doesn't have this 'feature'.  (I looked into trying to
allow manageing the internal capable() check externally, not so pretty).

> > This is just RFC, although I've tested that it does allow the chown +
> > read to work as expected.  Any other ideas of how to handle this are
> > welcome.
> 
> Can we just pass in the 'file' for all users of the bin files instead of
> the dentry? 

The dentry doesn't currently go beyond sysfs/bin.c.  So, yes, I pushed
'file' through to last level in bin.c before ->read(), and can certinaly
just push through to ->read() as well.

>  You can always get the dentry from the file (as your patch
> showes), and there isn't that many users of this interface.  I'd really
> rather not have two different types of callbacks here.

Absolutely, this is just RFC (i.e. quicker to compile and test).  What
about write()?

thanks,
-chris

Re: [RFC PATCH] sysfs: bin_attr permission checking

[Greg KH]

On Wed, May 12, 2010 at 12:28:28PM -0700, Chris Wright wrote:
> * Greg KH (greg@kroah.com) wrote:
> > On Wed, May 12, 2010 at 11:47:13AM -0700, Chris Wright wrote:
> > > The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
> > > check to verify privileges before allowing a user to read device
> > > dependent config space.  This is meant to protect from an unprivileged
> > > user potentially locking up the box.
> > > 
> > > When assigning a PCI device directly to a guest with libvirt and KVM, the 
> > > sysfs config space file is chown'd to the user that the KVM guest will
> > > run as.  The guest needs to have full access to the device's config
> > > space since it's responsible for driving the device.  However, despite
> > > being the owner of the sysfs file, the CAP_SYS_ADMIN check will not
> > > allow read access beyond the config header.
> > > 
> > > This patch adds a new bin_attr->read_file() callback which adds a struct
> > > file to the normal argument list.  This allows an implementation such as
> > > PCI config space bin_attr read_file handler to check both inode
> > > permission as well as privileges to determine whether to allow
> > > privileged actions through the handler.
> > 
> > Ick, this is all because we like showing different information if the
> > user is "privileged or not" :(
> 
> yup
> 
> > Turns out, that this probably isn't the best user api to implement,
> > remind me never to do that again...
> 
> Yeah, it's challenging to deal with.  Alternative here is a new config
> sysfs entry that doesn't have this 'feature'.  (I looked into trying to
> allow manageing the internal capable() check externally, not so pretty).

That would require people to update libpci and maybe their scripts as
well, which wouldn't be as good.

> > > This is just RFC, although I've tested that it does allow the chown +
> > > read to work as expected.  Any other ideas of how to handle this are
> > > welcome.
> > 
> > Can we just pass in the 'file' for all users of the bin files instead of
> > the dentry? 
> 
> The dentry doesn't currently go beyond sysfs/bin.c.  So, yes, I pushed
> 'file' through to last level in bin.c before ->read(), and can certinaly
> just push through to ->read() as well.

That would be better than having a 'read_file' callback, right?

> >  You can always get the dentry from the file (as your patch
> > showes), and there isn't that many users of this interface.  I'd really
> > rather not have two different types of callbacks here.
> 
> Absolutely, this is just RFC (i.e. quicker to compile and test).  What
> about write()?

Sure, might as well make it symmetrical :)

thanks,

greg k-h

Re: [RFC PATCH] sysfs: bin_attr permission checking

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Wed, May 12, 2010 at 12:28:28PM -0700, Chris Wright wrote:
> > * Greg KH (greg@kroah.com) wrote:
> > > On Wed, May 12, 2010 at 11:47:13AM -0700, Chris Wright wrote:
> > > > The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
> > > > check to verify privileges before allowing a user to read device
> > > > dependent config space.  This is meant to protect from an unprivileged
> > > > user potentially locking up the box.
> > > > 
> > > > When assigning a PCI device directly to a guest with libvirt and KVM, the 
> > > > sysfs config space file is chown'd to the user that the KVM guest will
> > > > run as.  The guest needs to have full access to the device's config
> > > > space since it's responsible for driving the device.  However, despite
> > > > being the owner of the sysfs file, the CAP_SYS_ADMIN check will not
> > > > allow read access beyond the config header.
> > > > 
> > > > This patch adds a new bin_attr->read_file() callback which adds a struct
> > > > file to the normal argument list.  This allows an implementation such as
> > > > PCI config space bin_attr read_file handler to check both inode
> > > > permission as well as privileges to determine whether to allow
> > > > privileged actions through the handler.
> > > 
> > > Ick, this is all because we like showing different information if the
> > > user is "privileged or not" :(
> > 
> > yup
> > 
> > > Turns out, that this probably isn't the best user api to implement,
> > > remind me never to do that again...
> > 
> > Yeah, it's challenging to deal with.  Alternative here is a new config
> > sysfs entry that doesn't have this 'feature'.  (I looked into trying to
> > allow manageing the internal capable() check externally, not so pretty).
> 
> That would require people to update libpci and maybe their scripts as
> well, which wouldn't be as good.

Not necessarily.  We'd end up with

  /config <-- legacy w/ CAP_SYS_AMDIN check)
  /config_not_f'd_up <-- new one w/out CAP_SYS_ADMIN, default to 0600 root:root

> > > > This is just RFC, although I've tested that it does allow the chown +
> > > > read to work as expected.  Any other ideas of how to handle this are
> > > > welcome.
> > > 
> > > Can we just pass in the 'file' for all users of the bin files instead of
> > > the dentry? 
> > 
> > The dentry doesn't currently go beyond sysfs/bin.c.  So, yes, I pushed
> > 'file' through to last level in bin.c before ->read(), and can certinaly
> > just push through to ->read() as well.
> 
> That would be better than having a 'read_file' callback, right?

I think so.

> > >  You can always get the dentry from the file (as your patch
> > > showes), and there isn't that many users of this interface.  I'd really
> > > rather not have two different types of callbacks here.
> > 
> > Absolutely, this is just RFC (i.e. quicker to compile and test).  What
> > about write()?
> 
> Sure, might as well make it symmetrical :)

:-)

I'll update, split sysfs from pci and resend shortly.

thanks,
chris