Prev: Loan Application
Next: keys: __rcu annotations
From: Alan Cox on 13 May 2010 06:00
On Wed, 12 May 2010 18:29:57 -0700
Chris Wright <chrisw(a)sous-sol.org> 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.

Or hacking it. You could argue the correct requirement is to change it to
require CAP_SYS_RAWIO in fact !

> With this patch the sysfs file owner is also considered privileged enough
> to read all of the config space.

Which breaks the main reason the check was there in the first place. To
stop accidents of the form

find / -exec grep {} "wibble"

blowing up in people's faces.


I agree with the problem - but IMHO the fix is to require opening the file
checks CAP_SYS_something instead: not to hack the read method and make it
even weirder and more un-Linux than it is now.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 14 May 2010 15:20
On Wed, May 12, 2010 at 06:29:57PM -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 unprivileged 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.
>
> With this patch the sysfs file owner is also considered privileged enough
> to read all of the config space.
>
> Signed-off-by: Chris Wright <chrisw(a)sous-sol.org>
> ---
> drivers/pci/pci-sysfs.c | 4 +++-

Jesse, any objection to this going through my tree as it will depend on
the sysfs change?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jesse Barnes on 14 May 2010 15:30
On Fri, 14 May 2010 12:09:23 -0700
Greg KH <greg(a)kroah.com> wrote:

> On Wed, May 12, 2010 at 06:29:57PM -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 unprivileged 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.
> >
> > With this patch the sysfs file owner is also considered privileged enough
> > to read all of the config space.
> >
> > Signed-off-by: Chris Wright <chrisw(a)sous-sol.org>
> > ---
> > drivers/pci/pci-sysfs.c | 4 +++-
>
> Jesse, any objection to this going through my tree as it will depend on
> the sysfs change?

Aside from the concerns Alan raised, yeah that's fine.

But I guess it was your idea to rely on the CAP bits rather than
ownership, so you get to decide how to clean up the mess. :)

I really wish ownership was enough in sysfs, but we were too weak in
the beginning and allowed userspace bugs (like walking all of sysfs and
reading every file) to scare us into putting weird enable flags and
checks like this in...

--
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Loan Application
Next: keys: __rcu annotations