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

From: David Howells <dhowells@redhat.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: dhowells@redhat.com, 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: Fri, 02 Dec 2016 14:59:22 +0000	[thread overview]
Message-ID: <23342.1480690762@warthog.procyon.org.uk> (raw)
In-Reply-To: <20161202065530.GA19753@kroah.com>

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.

> "this" patchset does nothing to disable anything,

That is correct, I even say as much in the cover note and patch 1.

> 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.

With this patchset, I'm hoping maintainers will check the annotations are
correct and point out anything I've missed.  There are a lot of module
parameters and not so much consistency in schemes for naming parameters and
their variables.

> > 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.

> 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.

 (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.

David