Date: Fri, 2 Dec 2016 07:55:30 +0100
From: Greg KH <gregkh@linuxfoundation.org>
To: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: David Howells <dhowells@redhat.com>, linux-kernel@vger.kernel.org,
        gnomes@lxorguk.ukuu.org.uk, linux-security-module@vger.kernel.org,
        keyrings@vger.kernel.org, minyard@acm.org
Subject: Re: [PATCH 01/39] Annotate module params that specify hardware
 parameters (eg. ioport)
Message-ID: <20161202065530.GA19753@kroah.com>
References: <148059537897.31612.9461043954611464597.stgit@warthog.procyon.org.uk>
 <148059538747.31612.8974972913601108271.stgit@warthog.procyon.org.uk>
 <20161201150135.GA10317@kroah.com>
 <20161202030700.GA21669@srcf.ucam.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161202030700.GA21669@srcf.ucam.org>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3749
Lines: 75

On Fri, Dec 02, 2016 at 03:07:00AM +0000, Matthew Garrett wrote:
> On Thu, Dec 01, 2016 at 04:01:35PM +0100, Greg KH wrote:
> 
> > First off, this "secure boot support" massive patchset has not gone
> > anywhere yet, so why do this now?
> 
> Because David ended up with the short straw when distro maintainers 
> talked about this at LPC.
> 
> > Secure boot is a trust that the previous boot process is now booting
> > your image that it feels is secure (with various levels of "secure").
> > It is not about "lock things down so no one can ever touch the hardware
> > through different options, except through random logic[1] that we
> > somehow trust "more" than configuration options.
> 
> 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?

> > So, what are you really trying to "block" here?  The ability for someone
> > to set an i/o port value?  why?  Why does it matter what root sets for
> > an irq?  For a dma buffer?  For anything else?  What is preventing this
> > going to "secure" somehow?
> 
> 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?

> > Overall, I really don't like this, and honestly, don't like the whole
> > "secure boot" patchset either, as it is really a lot of work for
> > absolutely no gain that I can see.  Who is "asking" for this type of
> > thing, and what are their specific requirements?
> 
> 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.

> 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?)

Come on, you know better than this, each patch/series/feature has to be
justifable on it's own, and this patchset, as-is, doesn't pass that test
to me, if for no other reason than it is just "marking" things that is
never then being used.

thanks,

greg k-h