LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: Mimi Zohar <zohar@linux.vnet.ibm.com>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Kalle Valo <kvalo@codeaurora.org>,
	Seth Forshee <seth.forshee@canonical.com>,
	Johannes Berg <johannes.berg@intel.com>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>,
	linux-integrity@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Peter Jones <pjones@redhat.com>,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, David Howells <dhowells@redhat.com>,
	Kees Cook <keescook@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andres Rodriguez <andresx7@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
Date: Tue, 8 May 2018 17:34:04 +0000	[thread overview]
Message-ID: <20180508173404.GG27853@wotan.suse.de> (raw)
In-Reply-To: <1525393466.3539.133.camel@linux.vnet.ibm.com>

On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-04 at 00:07 +0000, Luis R. Rodriguez wrote:
> > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > other firmware.
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > Cc: Luis R. Rodriguez <mcgrof@suse.com>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Seth Forshee <seth.forshee@canonical.com>
> > > Cc: Johannes Berg <johannes.berg@intel.com>
> > > ---
> > >  drivers/base/firmware_loader/main.c | 5 +++++
> > >  include/linux/fs.h                  | 1 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > > index eb34089e4299..d7cdf04a8681 100644
> > > --- a/drivers/base/firmware_loader/main.c
> > > +++ b/drivers/base/firmware_loader/main.c
> > > @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> > >  			break;
> > >  		}
> > >  
> > > +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > > +		if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> > > +		    (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
> > > +			id = READING_FIRMWARE_REGULATORY_DB;
> > > +#endif
> > 
> > Whoa, no way.
> 
> There are two methods for the kernel to verify firmware signatures.

Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
mechanism to verify firmware it uses the request_firmware*() API for
regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
files since the firmware API is used.

As such I see no reason to add a new ID for them at all.

Its not providing an *alternative*, its providing an *extra* kernel measure.
If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
stacked LSM. I'd be open to see patches which set that out. May be a
cleaner interface.

> If both are enabled, do we require both signatures or is one enough.

Good question. Considering it as a stacked LSM (although not implemented
as one), I'd say its up to who enabled the Kconfig entries. If IMA and
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
IMA though, then surely I agree that enabling
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to the
system integrator to decide.

If we however want to make it clear that such things as
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
could just make the kconfig depend on !IMA or something? Or perhaps a new
kconfig for IMA which if selected it means that drivers can opt to open code
*further* kernel signature verification, even though IMA already is sufficient.
Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

> Assigning a different id for regdb signed firmware allows LSMs and IMA
> to handle regdb files differently.

That's not the main concern here, its the precedent we are setting here for
any new kernel interface which open codes firmware signing on its own. What
you are doing means other kernel users who open codes their own firmware
signing may need to add yet-another reading ID. That doesn't either look
well on code, and seems kind of silly from a coding perspective given
the above, in which I clarify IMA still is doing its own appraisal on it.

> > >  		fw_priv->size = 0;
> > >  		rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
> > >  						msize, id);
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index dc16a73c3d38..d1153c2884b9 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > >  	id(FIRMWARE, firmware)		\
> > >  	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
> > >  	id(FIRMWARE_FALLBACK, firmware)	\
> > > +	id(FIRMWARE_REGULATORY_DB, firmware)	\
> > 
> > Why could IMA not appriase these files? They are part of the standard path.
> 
> The subsequent patch attempts to verify the IMA-appraisal signature, but on
> failure it falls back to allowing regdb signatures. 
> For systems that only want to load firmware based on IMA-appraisal, then
>regdb wouldn't be enabled.

I think we can codify this a bit better, without a new ID.

  Luis

  reply	other threads:[~2018-05-08 17:34 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-01 13:48 [PATCH 0/6] firmware: kernel signature verification Mimi Zohar
2018-05-01 13:48 ` [PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading Mimi Zohar
2018-05-04  0:02   ` Luis R. Rodriguez
2018-05-04  0:36     ` Mimi Zohar
2018-05-01 13:48 ` [PATCH 2/6] ima: prevent sysfs fallback firmware loading Mimi Zohar
2018-05-04  0:06   ` Luis R. Rodriguez
2018-05-01 13:48 ` [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware Mimi Zohar
2018-05-04  0:07   ` Luis R. Rodriguez
2018-05-04  0:24     ` Mimi Zohar
2018-05-08 17:34       ` Luis R. Rodriguez [this message]
2018-05-09 11:30         ` Mimi Zohar
2018-05-09 19:15           ` Luis R. Rodriguez
2018-05-09 19:57             ` Mimi Zohar
2018-05-09 21:22               ` Luis R. Rodriguez
2018-05-09 22:06                 ` Mimi Zohar
2018-05-09 23:48                   ` Luis R. Rodriguez
2018-05-10  2:00                     ` Mimi Zohar
2018-05-10 23:26                       ` Luis R. Rodriguez
2018-05-11  5:00                         ` Mimi Zohar
2018-05-11 21:52                           ` Luis R. Rodriguez
2018-05-14 12:58                             ` Mimi Zohar
2018-05-14 19:28                               ` Luis R. Rodriguez
2018-05-15  2:02                                 ` Mimi Zohar
2018-05-15  3:26                                   ` Luis R. Rodriguez
2018-05-15 12:32                                     ` Josh Boyer
2018-05-15 12:43                                       ` Mimi Zohar
2018-05-01 13:48 ` [PATCH 4/6] ima: coordinate with signed regulatory.db Mimi Zohar
2018-05-01 13:48 ` [PATCH 5/6] ima: verify kernel firmware signatures when using a preallocated buffer Mimi Zohar
2018-05-01 13:48 ` [RFC PATCH 6/6] ima: prevent loading firmware into a pre-allocated buffer Mimi Zohar
2018-05-04  0:10   ` Luis R. Rodriguez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180508173404.GG27853@wotan.suse.de \
    --to=mcgrof@kernel.org \
    --cc=andresx7@gmail.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=dhowells@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=johannes.berg@intel.com \
    --cc=keescook@chromium.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=pjones@redhat.com \
    --cc=seth.forshee@canonical.com \
    --cc=torvalds@linux-foundation.org \
    --cc=zohar@linux.vnet.ibm.com \
    --subject='Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).