Re: [PATCH 01/39] Annotate module params that specify hardware parameters (eg. ioport)

[Matthew Garrett <mjg59@srcf.ucam.org>]

On Fri, Dec 02, 2016 at 07:55:30AM +0100, Greg KH wrote:
> On Fri, Dec 02, 2016 at 03:07:00AM +0000, Matthew Garrett wrote:
> > If root is able to modify the behaviour of verified code after it was 
> > verified, then the value of that verification is reduced. Ensuring that 
> > the code remains trustworthy is vital in a number of security use cases.
> 
> Ok, but why are you now deciding to somehow try to "classify" the types
> of module parameters?  Why would you want to allow irqs to change, but
> not iobase?  Or something else?  Who is going to do this "I want you and
> you but not you" decision?  Why not just forbid all module parameters at
> all if they are so dangerous?

If the parameters plausibly make it possible for root to modify the 
kernel in interesting ways, then restricting them makes sense. My gut 
sense is that parameters that allow the alteration of the base address 
of memory mapped devices are clearly a problem in this respect, but port 
io and IRQs *probably* aren't. On the other hand, blocking mmio base 
addresses and not blocking the others is kind of inconsistent.

> > If root can tell a driver to probe for hardware at a specific address, 
> > and that driver will then blindly do so, root is trivially able to 
> > modify arbitrary kernel memory and disable arbitrary security features. 
> > IRQ or io port attacks are much more difficult to take advantage of, but 
> > I could imagine that some of them are still plausible.
> 
> Then just mark them all as "bad", why pick and choose?

Most parameters are going to be fine, but sure, flagging all 
IRQ/mmio/pio address parameters seems reasonable.

> > Here's an example. The sysfs option to enable module signing is write 
> > once. If root sets that, root can't unset it. Except there's a whole 
> > bunch of ways that root *can* unset it, including kexec 
> > (https://mjg59.dreamwidth.org/28746.html) and a bunch of other things 
> > that are disabled by this patchset. That feature is entirely useless as 
> > is. This patchset helps make it useful.
> 
> "this" patchset does nothing to disable anything, so I can't speak to
> any of the other goals you might have for that code, that's not what we
> are reviewing here.

This is prep work that makes it possible to block module parameters that 
would otherwise make it possible to avoid those restrictions.

> > Right now, the secure boot patchset is shipped by basically every single 
> > mainstream Linux distribution (and a whole bunch that are niche). Right 
> > now they're having to do extra work to rebase it and ensure that fixes 
> > get distributed to everyone. There's clearly demand, and Linus has been 
> > clear that features that are shipped by everyone should just go into 
> > mainline, so if there are *technical* objections then let's figure them 
> > out and otherwise just get this stuff merged.
> 
> "this stuff" is brand new things, that no one is shipping.  And nothing
> "just goes" into mainline, no matter what foolish stuff distros end up
> shipping (an example, do you want the giant Xen kernel patchset that
> SuSE has been dragging around for 10+ years?)

This is a logical extension to the base patchset, and one maintainer has 
NAKed the base patchset due to it lacking this feature. If you don't 
care about this then just tell Alan that you want the base patchset 
merged anyway and we'll go from there. Let's not get into a situation 
where people are being given incompatible requirements before 
something's merged.

-- 
Matthew Garrett | mjg59@srcf.ucam.org