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

From: Greg KH <gregkh@linuxfoundation.org>
To: David Howells <dhowells@redhat.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>,
	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)
Date: Mon, 5 Dec 2016 16:47:14 +0100	[thread overview]
Message-ID: <20161205154714.GA3353@kroah.com> (raw)
In-Reply-To: <23342.1480690762@warthog.procyon.org.uk>

On Fri, Dec 02, 2016 at 02:59:22PM +0000, David Howells wrote:
> Greg KH <gregkh@linuxfoundation.org> 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?
> 
> Because Alan says that locking down the module parameters needs to be done
> first.  Since I had to go through and modify each module parameter to mark the
> hardware config ones, it seemed like a good opportunity to label their type
> (ioport, iomem, irq, etc.) whilst I was at it.
> 
> > Then just mark them all as "bad", why pick and choose?
> 
> Because some drivers, IPMI for example, can also be autoconfigured via PCI,
> PNP, ACPI or whatever and still be useful, if not important, to the system's
> operation.
> 
> Simply marking all drivers that can be so configured as "bad" and rejecting
> them outright in lockdown mode is a non-starter.

Sorry, I meant to mark all of these types of attributes as "bad", not
trying to classify all of the different types of attributes, given that
you will probably want to just ban all of them or none, right?

> > > Right now, the secure boot patchset
> >
> > "this stuff" is brand new things, that no one is shipping.
> 
> True, but my other two patchsets are primarily made up of things people *are*
> shipping.  If you're happy to for those to go in and can persuade Alan to okay
> deferral of module parameter lockdown for an extra cycle, that's fine by me.

I'll defer to Alan as to what he feels is needed here, given that this
patchset isn't being shipped by anyone I think it's odd to somehow make
this a pre-requisite for anything to be merged as no real user of the
patchset seemed to feel this type of thing was needed :)

> > 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.
> 
> You're being unreasonable.  The complete set is on the order of 90 patches, I
> think.  I could submit them all in one go in a single series, but then people
> would be complaining that it's too big and that I have to split it up.
> 
> I have broken it up into a number of logical series, of which I've published
> some:
> 
>  (1) Determining the EFI secure boot state.  This only depends on tip
>      efi/core.  This mostly takes what the ARM arch already does upstream and
>      extends it to x86 too.
> 
>  (2) Marking hardware config module params.  Patches 2+ all depend on patch 1,
>      but there are no dependencies outside of that series.  If I could get
>      patch 1 upstream, I could distribute patches 2+ individually to the
>      maintainers.
> 
>  (3) Kernel lockdown.  This takes the determination made by (1) and applies
>      it, enabling various lockdowns, including locking down anything annotated
>      in (2).
> 
>  (4) System blacklist.  List hashes to be blacklisted.  This is independent of
>      all other series.

These are hashes of what?

>  (5) UEFI/SHIM whitelist/blacklist loading.  This has a dependency on some
>      constants added in (1).
> 
> I have to do them in some order.  Doing it this way means that some of these
> are self-contained, making it technically easier to upstream those pieces.

I think patch 3 is going to be the "hardest", along with 2, given the
large area it touches.  Why not work on the other bits first?

thanks,

greg k-h