SELinux detects problem with proprietary binary fglrx driver; however, AMD/ATI will not help

[Eric Paris]

On Mon, 2008-09-29 at 09:13 -0400, Stephen Smalley wrote:
> On Thu, 2008-09-25 at 09:13 -0400, Eric Paris wrote:
> > On Thu, 2008-09-25 at 14:15 +1000, James Morris wrote:
> > > On Wed, 24 Sep 2008, Francis K Shim wrote:
> > > 
> > > > 
> > > > I could disable SELinux and I would not have this problem; however, I
> > > > was hoping that there was a much secure or safer work-around to this
> > > > problem.
> > > 
> > > The video driver is inherently dangerous, so the safe approach is not to 
> > > use it.
> > 
> > James isn't exactly being helpful, but the reason is because as you
> > guessed the problem lies squarely and obviously with AMD/ATI and there
> > isn't much we can do to help with closed source proprietary software.
> > AMD/ATI is obviously doing it wrong and when it comes to security doing
> > it wrong is never a good idea.  Sadly we don't have their source so I
> > can't show you the line of code (or do anything to fix it), but your
> > backtrace should make it pretty obvious if anyone inside ATI decides to
> > care.
> > 
> > Stephen James, what do the two of you think about something like this?
> > Maybe a WARN_ONCE() ?
> > 
> >  security/selinux/hooks.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 03fc6a8..14f1242 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1385,7 +1385,8 @@ static int task_has_capability(struct task_struct *tsk,
> >  	default:
> >  		printk(KERN_ERR
> >  		       "SELinux:  out of range capability %d\n", cap);
> > -		BUG();
> > +		WARN();
> > +		return -EPERM;
> >  	}
> >  	return avc_has_perm(tsec->sid, tsec->sid, sclass, av, &ad);
> >  }
> 
> I'm not opposed to changing it to an error return, although I'm not
> clear that will help.
> 
> Does anyone know what the driver is actually trying to do here?  The
> message said:
> SELinux: out of range capability -157851600
> and -157851600 == 0xf6976030
> Obviously that isn't what they meant to pass to capable().

I don't think any of us have any idea what the driver was trying to do
here, but clearly they are doing it wrong.  Maybe you can turn on your
NSA brain reading satellite and peer into the brain of an ATI
programmer....

I'm actually quite shocked it didn't blow up in cap_raised

#define cap_raised(c, flag) ((c).cap[CAP_TO_INDEX(flag)] & CAP_TO_MASK(flag))

its just got to be darn lucky that c.cap[huge index] actually hit a
legal spot of memory and didn't bug on a pagefault right there. (I'm
pretty sure 64 bit capabilities were in 2.6.25 right?)  Before 64 bit
caps I don't think we had the array index and only had the mask, so
something huge wouldn't explode in the cap code...

We can turn this into a WARN_ONCE() but people with this driver would
still be taking a walk on the wild side since there is no bounds
checking what-so-ever in the cap code....

I feel your pain, and we can make the kernel stop BUGing in SELinux
code, but after looking at the cap code that it already ran your just as
likely to BUG even without SELinux (and not nearly as cleanly...)

I'm sorry to say I think its probably best for us to leave the BUG so at
least the kernel will explode predictably rather than randomly and in a
harder to track down way...

I wonder if I should propose adding a cap_valid() check to cap_capable()
upstream...

-Eric