Re: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
- From: Eric Paris <eparis@xxxxxxxxxx>
- Date: Tue, 30 Sep 2008 10:36:28 -0400
On Wed, 2008-10-01 at 00:23 +1000, James Morris wrote:
On Tue, 30 Sep 2008, Eric Paris wrote:
This patch adds a WARN_ONCE() to cap_capable() so we will stop
dereferencing random spots of memory and will cleanly tell the obviously
broken driver that it doesn't have that ridiculous permissions. No idea
if the driver is going to handle EPERM but anything that calls capable
and doesn't expect a denial has got to be the worst piece of code ever
written..... I could return EINVAL, but I think its clear that noone
has capabilities over 64 so clearly they don't have that permission.
This 'could' be considered a regression since 2.6.24. Neither SELinux
nor the capabilities system had a problem with ginormous request values
until we got 64 bit support, although this is OBVIOUSLY a bug with the
out of tree closed source driver....
An issue here is whether we should be adding workarounds in the mainline
kernel for buggy closed drivers. Papering over problems rather than
getting them fixed does not seem like a winning approach. Especially
problems which are unexpectedly messing with kernel security APIs.
I don't know, looking at the feelings on "Can userspace bugs be kernel
regressions" leads me to believe that when we break something that once
worked we are supposed to fix it.
http://lwn.net/Articles/292143/
I don't think the proprietary closed source nature of the driver makes
it any less our problem to not make changes which cause the kernel to
esplode.
Also, won't this encourage vendors of such drivers to continue with this
behavior, while discouraging those vendors who are doing the right thing?
Discouraging people who open source their drivers and put them in the
kernel? obviously not. encouraging crap? well, I hope we fix
regressions no matter how they are found...
Do we know if this even really helps the user? For all we know, the
driver may simply crash differently with an -EPERM.
Well, before the 64 bit capabilities change we did:
(cap_t(c) & CAP_TO_MASK(flag))
so a huge value for "flag" got masked off.
After 64 bit capabilities we do:
((c).cap[CAP_TO_INDEX(flag)] & CAP_TO_MASK(flag))
so a huge flag causes an array index out of bounds and either explodes
here or continues onto SELinux where it BUG().
So this is regression. It would have gotten an EPERM, now it gets a
BUG/panic.
Yes ATI needs to fix their driver, but we broke it and I don't remember
the driver not working on 2.6.24 and earlier....
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- Re: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
- From: Serge E. Hallyn
- Re: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
- References:
- Prev by Date: Re: [GIT pull] timer fixes for .27
- Next by Date: [ksoftirqd/1] CPU reporting.
- Previous by thread: Re: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
- Next by thread: Re: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
- Index(es):
Relevant Pages
|
