[PATCH 0/6] firmware: kernel signature verification

[PATCH 0/6] firmware: kernel signature verification

[Mimi Zohar]

Allow LSMs and IMA to differentiate between different methods of
firmware loading (eg. direct loading, sysfs fallback) and to
differentiate/coordinate between signature verification methods (eg.
regdb, IMA-appraisal).

In addition, the last two patches address the pre-allocated buffer.  The
first of these patches doesn't differentiate between reading the
firmware first into kernel memory and verifying the kernel signature,
versus reading the firmware directly into a pre-allocated buffer.

The last patch, which is posted as an RFC, questions whether the device
can access the pre-allocated buffer before the kernel signature has been
verified.

Mimi Zohar (6):
  firmware: permit LSMs and IMA to fail firmware sysfs fallback loading
  ima: prevent sysfs fallback firmware loading
  firmware: differentiate between signed regulatory.db and other
    firmware
  ima: coordinate with signed regulatory.db
  ima: verify kernel firmware signatures when using a preallocated
    buffer
  ima: prevent loading firmware into a pre-allocated buffer

 drivers/base/firmware_loader/fallback.c |  7 +++++++
 drivers/base/firmware_loader/main.c     |  5 +++++
 include/linux/fs.h                      |  2 ++
 security/integrity/ima/ima_main.c       | 29 +++++++++++++++++++++++++++--
 4 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.7.5

[PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading

[Mimi Zohar]

Add an LSM hook prior to allowing firmware sysfs fallback loading.

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: Matthew Garrett <mjg59@google.com>
---
 drivers/base/firmware_loader/fallback.c | 7 +++++++
 include/linux/fs.h                      | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 31b5015b59fe..23d2af30474e 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+	int ret;
+
 	if (fw_fallback_config.ignore_sysfs_fallback) {
 		pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
 		return false;
@@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 	if ((opt_flags & FW_OPT_NOFALLBACK))
 		return false;
 
+	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
+	ret = security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK);
+	if (ret < 0)
+		return ret;
+
 	return fw_force_sysfs_fallback(opt_flags);
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..dc16a73c3d38 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
 	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
+	id(FIRMWARE_FALLBACK, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
-- 
2.7.5

Re: [PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading

[Luis R. Rodriguez]

If you can add Andres Rodriguez <andresx7@gmail.com>, and Greg to your Cc list
in the future patches that'd be appreciated.

On Tue, May 01, 2018 at 09:48:18AM -0400, Mimi Zohar wrote:
> Add an LSM hook prior to allowing firmware sysfs fallback loading.
> 
> 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: Matthew Garrett <mjg59@google.com>
> ---
>  drivers/base/firmware_loader/fallback.c | 7 +++++++
>  include/linux/fs.h                      | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 31b5015b59fe..23d2af30474e 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
>  
>  static bool fw_run_sysfs_fallback(unsigned int opt_flags)
>  {
> +	int ret;
> +
>  	if (fw_fallback_config.ignore_sysfs_fallback) {
>  		pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
>  		return false;
> @@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
>  	if ((opt_flags & FW_OPT_NOFALLBACK))
>  		return false;
>  
> +	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
> +	ret = security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK);
> +	if (ret < 0)
> +		return ret;
> +
>  	return fw_force_sysfs_fallback(opt_flags);
>  }
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..dc16a73c3d38 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
>  	id(UNKNOWN, unknown)		\
>  	id(FIRMWARE, firmware)		\
>  	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
> +	id(FIRMWARE_FALLBACK, firmware)	\

If you're going to add this perhaps FIRMWARE_FALLBACK_SYSFS as we may later
get FIRMWARE_FALLBACK_EFI.

  Luis

Re: [PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading

[Mimi Zohar]

On Fri, 2018-05-04 at 00:02 +0000, Luis R. Rodriguez wrote:
> If you can add Andres Rodriguez <andresx7@gmail.com>, and Greg to your Cc list
> in the future patches that'd be appreciated.
> 
> On Tue, May 01, 2018 at 09:48:18AM -0400, Mimi Zohar wrote:
> > Add an LSM hook prior to allowing firmware sysfs fallback loading.
> > 
> > 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: Matthew Garrett <mjg59@google.com>
> > ---
> >  drivers/base/firmware_loader/fallback.c | 7 +++++++
> >  include/linux/fs.h                      | 1 +
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> > index 31b5015b59fe..23d2af30474e 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> >  
> >  static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> >  {
> > +	int ret;
> > +
> >  	if (fw_fallback_config.ignore_sysfs_fallback) {
> >  		pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
> >  		return false;
> > @@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> >  	if ((opt_flags & FW_OPT_NOFALLBACK))
> >  		return false;
> >  
> > +	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
> > +	ret = security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	return fw_force_sysfs_fallback(opt_flags);
> >  }
> >  
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 760d8da1b6c7..dc16a73c3d38 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
> >  	id(UNKNOWN, unknown)		\
> >  	id(FIRMWARE, firmware)		\
> >  	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
> > +	id(FIRMWARE_FALLBACK, firmware)	\
> 
> If you're going to add this perhaps FIRMWARE_FALLBACK_SYSFS as we may later
> get FIRMWARE_FALLBACK_EFI.

>From an IMA signature verification perspective, both are buffer based.
 The file signature is stored as a security xattr.  Without a file
descriptor, the kernel cannot verify the firmware signature.

I don't have a problem with defining another enumeration.  Perhaps
other LSMs will want to be able to differentiate between sysfs and EFI
fallback methods.

Mimi

[PATCH 2/6] ima: prevent sysfs fallback firmware loading

[Mimi Zohar]

With an IMA policy requiring signed firmware, this patch prevents
the sysfs fallback method of loading 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: Matthew Garrett <mjg59@google.com>
---
 security/integrity/ima/ima_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 754ece08e1c6..8759280dccf6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -453,7 +453,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		}
 		return 0;
 	}
+
+	if (read_id == READING_FIRMWARE_FALLBACK) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("Prevent firmware sysfs fallback loading.\n");
+			return -EACCES;
+		}
+		return 0;
+	}
 	return 0;
+
 }
 
 static int read_idmap[READING_MAX_ID] = {
-- 
2.7.5

Re: [PATCH 2/6] ima: prevent sysfs fallback firmware loading

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 09:48:19AM -0400, Mimi Zohar wrote:
> With an IMA policy requiring signed firmware, this patch prevents
> the sysfs fallback method of loading 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: Matthew Garrett <mjg59@google.com>
> ---
>  security/integrity/ima/ima_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 754ece08e1c6..8759280dccf6 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -453,7 +453,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  		}
>  		return 0;
>  	}
> +
> +	if (read_id == READING_FIRMWARE_FALLBACK) {
> +		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +			pr_err("Prevent firmware sysfs fallback loading.\n");
> +			return -EACCES;
> +		}
> +		return 0;
> +	}
>  	return 0;
> +
>  }
>  

Due to the lack of ability to appraise these calls, it has me wondering if having
these drivers be wrapped into a their own kconfig may make sense, ie, they use
a mechanism which IMA cannot possibly work with. Then at least some kernel
builds can exist in which we know we can count on this run time to never
happen. Thoughts?

See for instance use of CONFIG_PREVENT_FIRMWARE_BUILD.

  Luis

[PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

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
 		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)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
-- 
2.7.5

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

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.

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

  Luis

>  	id(MODULE, kernel-module)		\
>  	id(KEXEC_IMAGE, kexec-image)		\
>  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> -- 
> 2.7.5
> 
> 

-- 
Do not panic

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

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.
 If both are enabled, do we require both signatures or is one enough.
Assigning a different id for regdb signed firmware allows LSMs and IMA
to handle regdb files differently.

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

Mimi

> 
> >  	id(MODULE, kernel-module)		\
> >  	id(KEXEC_IMAGE, kexec-image)		\
> >  	id(KEXEC_INITRAMFS, kexec-initramfs)	\
> > -- 
> > 2.7.5
> > 
> > 
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

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

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-08 at 17:34 +0000, Luis R. Rodriguez wrote:
> 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.

IMA-appraisal can verify a signature stored as an xattr, but not a
detached signature.  That support could be added, but isn't there
today.  Today, a regulatory.db signature would have to be stored as an
xattr. 

> 
> As such I see no reason to add a new ID for them at all.
> K
> 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.

Just because IMA-appraisal is enabled in the kernel doesn't mean that
firmware signatures will be verified.  That is a run time policy
decision.

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

The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
time IMA config that translated into an IMA policy requiring firmware
signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
be sorted out at build time.

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

Suppose,

1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
"CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.

2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
"CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
appraises the firmware signature could be defined.  In this case, both
signature verification methods would be enforced.

then READING_FIRMWARE_REGULATORY_DB would not be needed.

Mimi

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

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-08 at 17:34 +0000, Luis R. Rodriguez wrote:
> > 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.
> 
> IMA-appraisal can verify a signature stored as an xattr, but not a
> detached signature.  That support could be added, but isn't there
> today.  Today, a regulatory.db signature would have to be stored as an
> xattr. 

Right, my point was that if someone has IMA installed:

	a) they would add those xattr to files in /lib/firmware/ already
	b) upon request_firmware*() calls a security hook would trigger
	   which would enable IMA to appraise those files. So not only
	   would the kernel in turn do double checks on regulatory.db,
	   but also a check on regulatory.db.p7s as well.

The difference I suppose is IMA would use a hash function instead of signature
check, correct?

> > As such I see no reason to add a new ID for them at all.
> > K
> > 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.
> 
> Just because IMA-appraisal is enabled in the kernel doesn't mean that
> firmware signatures will be verified.  That is a run time policy
> decision.

Sure, I accept this if IMA does not do signature verification. However
signature verification seems like a stackable LSM decision, no?

> > 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?
> 
> The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> time IMA config that translated into an IMA policy requiring firmware
> signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> be sorted out at build time.

I see makes sense.

> > > 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.
> 
> Suppose,
> 
> 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> 
> 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> appraises the firmware signature could be defined.  In this case, both
> signature verification methods would be enforced.
> 
> then READING_FIRMWARE_REGULATORY_DB would not be needed.

True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
could just be a mini subsystem stackable LSM.

Its not clear to me why we need to add a new READING id to any open coded
firmware signature checks if we don't have this futuristic option
CONFIG_IMA_APPRAISE_FIRMWARE.

Yes it provides *more*, but IMA is still seeing the exact file descriptor
and do its own thing.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 19:15 +0000, Luis R. Rodriguez wrote:

> > > > 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.
> > 
> > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > firmware signatures will be verified.  That is a run time policy
> > decision.
> 
> Sure, I accept this if IMA does not do signature verification. However
> signature verification seems like a stackable LSM decision, no?

IMA-appraisal can be configured to enforce file signatures.  Refer to
discussion below as to how.

> > > 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?
> > 
> > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > time IMA config that translated into an IMA policy requiring firmware
> > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > be sorted out at build time.
> 
> I see makes sense.

Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
above.

> 
> > > > 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.
> > 
> > Suppose,
> > 
> > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > 
> > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > appraises the firmware signature could be defined.  In this case, both
> > signature verification methods would be enforced.
> > 
> > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> 
> True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> could just be a mini subsystem stackable LSM.

Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
would differentiate between other firmware and the regulatory.db based
on the firmware's pathname.

Making regdb an LSM would have the same issues as currently - deciding
if regdb, IMA-appraisal, or both verify the regdb's signature.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 19:15 +0000, Luis R. Rodriguez wrote:
> 
> > > > > 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.
> > > 
> > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > firmware signatures will be verified.  That is a run time policy
> > > decision.
> > 
> > Sure, I accept this if IMA does not do signature verification. However
> > signature verification seems like a stackable LSM decision, no?
> 
> IMA-appraisal can be configured to enforce file signatures.  Refer to
> discussion below as to how.
> 
> > > > 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?
> > > 
> > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > time IMA config that translated into an IMA policy requiring firmware
> > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > be sorted out at build time.
> > 
> > I see makes sense.
> 
> Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> above.

OK, its still not clear to what it will do. If it does not touch the firmware
loader code, and it just sets and configures IMA to do file signature checking
on its own, then yes I think both mechanisms would be doing the similar work.

Wouldn't IMA do file signature checks then for all files? Or it would just
enable this for whatever files userspace wishes to cover?

One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
the wireless-regdgb maintainer's key for this file, could IMA be configured to
do that?

Because that would be one difference here. The whole point of adding
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
component which checks the signature of regulatory.db before reading it and
passing data to the kernel from it.

Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
you are mentioning, to still enable both to co-exist?

> > > > > 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.
> > > 
> > > Suppose,
> > > 
> > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > 
> > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > appraises the firmware signature could be defined.  In this case, both
> > > signature verification methods would be enforced.
> > > 
> > > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> > 
> > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > could just be a mini subsystem stackable LSM.
> 
> Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> would differentiate between other firmware and the regulatory.db based
> on the firmware's pathname.

If that is the only way then it would be silly to do the mini LSM as all
calls would have to have the check. A special LSM hook for just the
regulatory db also doesn't make much sense.

> Making regdb an LSM would have the same issues as currently - deciding
> if regdb, IMA-appraisal, or both verify the regdb's signature.

Its unclear to me why they can't co-exist yet and not have to touch
the firmware_loader code at all.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 21:22 +0000, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 19:15 +0000, Luis R. Rodriguez wrote:
> > 
> > > > > > 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.
> > > > 
> > > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > > firmware signatures will be verified.  That is a run time policy
> > > > decision.
> > > 
> > > Sure, I accept this if IMA does not do signature verification. However
> > > signature verification seems like a stackable LSM decision, no?
> > 
> > IMA-appraisal can be configured to enforce file signatures.  Refer to
> > discussion below as to how.
> > 
> > > > > 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?
> > > > 
> > > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > > time IMA config that translated into an IMA policy requiring firmware
> > > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > > be sorted out at build time.
> > > 
> > > I see makes sense.
> > 
> > Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> > post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> > above.
> 
> OK, its still not clear to what it will do. If it does not touch the firmware
> loader code, and it just sets and configures IMA to do file signature checking
> on its own, then yes I think both mechanisms would be doing the similar work.
> 
> Wouldn't IMA do file signature checks then for all files? Or it would just
> enable this for whatever files userspace wishes to cover?

Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
signatures on all directly loaded firmware and fail any method of
loading firmware that the signature couldn't be verified.

> One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
> the wireless-regdgb maintainer's key for this file, could IMA be configured to
> do that?

IMA has its own trusted keyring.  So either the maintainer's key would
need to be added to the IMA keyring, or IMA-appraisal would need to
use the regdb keyring.
  
> Because that would be one difference here. The whole point of adding
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
> component which checks the signature of regulatory.db before reading it and
> passing data to the kernel from it.
> 
> Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> you are mentioning, to still enable both to co-exist?

At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
would be enabled, not both.

The builtin IMA-policies could be replaced with a custom policy,
requiring firmware signature verification.  In that case, the regdb
signature would be verified twice.

> 
> > > > > > 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.
> > > > 
> > > > Suppose,
> > > > 
> > > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > > 
> > > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > > appraises the firmware signature could be defined.  In this case, both
> > > > signature verification methods would be enforced.
> > > > 
> > > > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> > > 
> > > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > > could just be a mini subsystem stackable LSM.
> > 
> > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > would differentiate between other firmware and the regulatory.db based
> > on the firmware's pathname.
> 
> If that is the only way then it would be silly to do the mini LSM as all
> calls would have to have the check. A special LSM hook for just the
> regulatory db also doesn't make much sense.

All calls to request_firmware() are already going through this LSM
hook.  I should have said, it would be based on both READING_FIRMWARE
and the firmware's pathname.

> 
> > Making regdb an LSM would have the same issues as currently - deciding
> > if regdb, IMA-appraisal, or both verify the regdb's signature.
> 
> Its unclear to me why they can't co-exist yet and not have to touch
> the firmware_loader code at all.

With the changes discussed above, they will co-exist.  Other than the
Kconfig changes, I don't think it will touch the firmware_loader code.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 21:22 +0000, Luis R. Rodriguez wrote:
> > 
> > OK, its still not clear to what it will do. If it does not touch the firmware
> > loader code, and it just sets and configures IMA to do file signature checking
> > on its own, then yes I think both mechanisms would be doing the similar work.
> > 
> > Wouldn't IMA do file signature checks then for all files? Or it would just
> > enable this for whatever files userspace wishes to cover?
> 
> Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> signatures on all directly loaded firmware and fail any method of
> loading firmware that the signature couldn't be verified.

Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me.
Specially in light of the fact that its what we recommend folks to consider
if they need to address firmware signing for devices which do not have the
ability to do hardware firmware signing verification on their own.

> > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
> > the wireless-regdgb maintainer's key for this file, could IMA be configured to
> > do that?
> 
> IMA has its own trusted keyring.  So either the maintainer's key would
> need to be added to the IMA keyring,

I see so we'd need this documented somehow.

> or IMA-appraisal would need to use the regdb keyring.

Can you describe this a bit more, for those not too familiar with IMA, in terms
of what would be involved in the kernel? Or is this all userspace configuration
stuff?

> > Because that would be one difference here. The whole point of adding
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
> > component which checks the signature of regulatory.db before reading it and
> > passing data to the kernel from it.
> > 
> > Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> > you are mentioning, to still enable both to co-exist?
> 
> At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
> would be enabled, not both.

OK.

> > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > would differentiate between other firmware and the regulatory.db based
> > > on the firmware's pathname.
> > 
> > If that is the only way then it would be silly to do the mini LSM as all
> > calls would have to have the check. A special LSM hook for just the
> > regulatory db also doesn't make much sense.
> 
> All calls to request_firmware() are already going through this LSM
> hook.  I should have said, it would be based on both READING_FIRMWARE
> and the firmware's pathname.

Yes, but it would still be a strcmp() computation added for all
READING_FIRMWARE. In that sense, the current arrangement is only open coding the
signature verification for the regulatory.db file.  One way to avoid this would
be to add an LSM specific to the regulatory db and have the
security_check_regulatory_db() do what it needs per LSM, but that would mean
setting a precedent for open possibly open coded future firmware verification
call. Its not too crazy to consider if an end goal may be avoid further open
coded firmware signature verification hacks.

> > > Making regdb an LSM would have the same issues as currently - deciding
> > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > 
> > Its unclear to me why they can't co-exist yet and not have to touch
> > the firmware_loader code at all.
> 
> With the changes discussed above, they will co-exist.  Other than the
> Kconfig changes, I don't think it will touch the firmware_loader code.

Great.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 21:22 +0000, Luis R. Rodriguez wrote:
> > > 
> > > OK, its still not clear to what it will do. If it does not touch the firmware
> > > loader code, and it just sets and configures IMA to do file signature checking
> > > on its own, then yes I think both mechanisms would be doing the similar work.
> > > 
> > > Wouldn't IMA do file signature checks then for all files? Or it would just
> > > enable this for whatever files userspace wishes to cover?
> > 
> > Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> > signatures on all directly loaded firmware and fail any method of
> > loading firmware that the signature couldn't be verified.
> 
> Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me.
> Specially in light of the fact that its what we recommend folks to consider
> if they need to address firmware signing for devices which do not have the
> ability to do hardware firmware signing verification on their own.
> 
> > > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
> > > the wireless-regdgb maintainer's key for this file, could IMA be configured to
> > > do that?
> > 
> > IMA has its own trusted keyring.  So either the maintainer's key would
> > need to be added to the IMA keyring,
> 
> I see so we'd need this documented somehow.
> 
> > or IMA-appraisal would need to use the regdb keyring.
> 
> Can you describe this a bit more, for those not too familiar with IMA, in terms
> of what would be involved in the kernel? Or is this all userspace configuration
> stuff?

I think it's a bit premature to be discussing how IMA could add the
builtin regulatory key to its keyring or use the regdb keyring, as
IMA-appraisal doesn't (yet) support detached signatures.

The other option would be to include the regulatory.db signature in
the package.  For rpm, the file signature is included in the RPM
header.  Multiple attempts have been made to have Debian packages
include file signatures.  This is the most recent attempt - https://li
sts.debian.org/debian-dpkg/2018/05/msg00005.html

> > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > would differentiate between other firmware and the regulatory.db based
> > > > on the firmware's pathname.
> > > 
> > > If that is the only way then it would be silly to do the mini LSM as all
> > > calls would have to have the check. A special LSM hook for just the
> > > regulatory db also doesn't make much sense.
> > 
> > All calls to request_firmware() are already going through this LSM
> > hook.  I should have said, it would be based on both READING_FIRMWARE
> > and the firmware's pathname.
> 
> Yes, but it would still be a strcmp() computation added for all
> READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> signature verification for the regulatory.db file.  One way to avoid this would
> be to add an LSM specific to the regulatory db

Casey already commented on this suggestion.

Mimi

> and have the
> security_check_regulatory_db() do what it needs per LSM, but that would mean
> setting a precedent for open possibly open coded future firmware verification
> call. Its not too crazy to consider if an end goal may be avoid further open
> coded firmware signature verification hacks.
> 
> > > > Making regdb an LSM would have the same issues as currently - deciding
> > > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > > 
> > > Its unclear to me why they can't co-exist yet and not have to touch
> > > the firmware_loader code at all.
> > 
> > With the changes discussed above, they will co-exist.  Other than the
> > Kconfig changes, I don't think it will touch the firmware_loader code.
> 
> Great.
> 
>   Luis
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> 
> > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > would differentiate between other firmware and the regulatory.db based
> > > > > on the firmware's pathname.
> > > > 
> > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > calls would have to have the check. A special LSM hook for just the
> > > > regulatory db also doesn't make much sense.
> > > 
> > > All calls to request_firmware() are already going through this LSM
> > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > and the firmware's pathname.
> > 
> > Yes, but it would still be a strcmp() computation added for all
> > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > signature verification for the regulatory.db file.  One way to avoid this would
> > be to add an LSM specific to the regulatory db
> 
> Casey already commented on this suggestion.

Sorry but I must have missed this, can you send me the email or URL where he did that?
I never got a copy of that email I think.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > 
> > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > would differentiate between other firmware and the regulatory.db based
> > > > > > on the firmware's pathname.
> > > > > 
> > > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > > calls would have to have the check. A special LSM hook for just the
> > > > > regulatory db also doesn't make much sense.
> > > > 
> > > > All calls to request_firmware() are already going through this LSM
> > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > and the firmware's pathname.
> > > 
> > > Yes, but it would still be a strcmp() computation added for all
> > > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > > signature verification for the regulatory.db file.  One way to avoid this would
> > > be to add an LSM specific to the regulatory db
> > 
> > Casey already commented on this suggestion.
> 
> Sorry but I must have missed this, can you send me the email or URL where he did that?
> I never got a copy of that email I think.

My mistake.  I've posted similar patches for kexec_load and for the
firmware sysfs fallback, both call security_kernel_read_file().
Casey's comment was in regards to kexec_load[1], not for the sysfs
fallback mode.  Here's the link to the most recent version of the
kexec_load patches.[2]

[1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
[2] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> > On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > > 
> > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > > would differentiate between other firmware and the regulatory.db based
> > > > > > > on the firmware's pathname.
> > > > > > 
> > > > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > > > calls would have to have the check. A special LSM hook for just the
> > > > > > regulatory db also doesn't make much sense.
> > > > > 
> > > > > All calls to request_firmware() are already going through this LSM
> > > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > > and the firmware's pathname.
> > > > 
> > > > Yes, but it would still be a strcmp() computation added for all
> > > > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > > > signature verification for the regulatory.db file.  One way to avoid this would
> > > > be to add an LSM specific to the regulatory db
> > > 
> > > Casey already commented on this suggestion.
> > 
> > Sorry but I must have missed this, can you send me the email or URL where he did that?
> > I never got a copy of that email I think.
> 
> My mistake.  I've posted similar patches for kexec_load and for the
> firmware sysfs fallback, both call security_kernel_read_file().
> Casey's comment was in regards to kexec_load[1], not for the sysfs
> fallback mode.  Here's the link to the most recent version of the
> kexec_load patches.[2]
> 
> [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
> [2] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html

It seems I share Eric's concern on these threads are over general architecture,
below some notes which I think may help for the long term on that regards.

In the firmware_loader case we have *one* subsystem which as open coded firmware
signing -- the wireless subsystem open codes firmware verification by doing two
request_firmware() calls, one for the regulatory.bin and one for regulatory.bin.p7s,
and then it does its own check. In this patch set you suggested adding
a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also
adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of
the old syfs loading facility.

My concerns are two fold for this case:

a) This would mean adding a new READING_* ID tag per any kernel mechanism which open
codes its own signature verification scheme.

b) The way it was implemented was to do (just showing
READING_FIRMWARE_REGULATORY_DB here):

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
                fw_priv->size = 0;
                rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
                                                msize, id);

This is eye-soring, and in turn would mean adding yet more #ifdefs for any
code on the kernel which open codes other firmware signing efforts with
its own kconfig...

I gather from reading the threads above that Eric's concerns are the re-use of
an API for security to read files for something which is really not a file, but
a binary blob of some sort and Casey's rebuttal is adding more hooks for small
things is a bad idea.

In light of all this I'll say that the concerns Eric has are unfortunately
too late, that ship has sailed eons ago. The old non-fd API for module loading
init_module() calls       security_kernel_read_file(NULL, READING_MODULE). Your
patch in this series adds security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK)
for the old syfs loading facility.

So in this regard, I think we have no other option but what you suggested, to
add a wrapper, say a security_kernel_read_blob() wrapper that calls
security_kernel_read_file(NULL, id); and make the following legacy calls use
it:

  o kernel/module.c for init_module()
  o kexec_load()
  o firmware loader sysfs facility

I think its fair then to add a new READING entry per functionality here
*but* with the compromise that we *document* that such interfaces are
discouraged, in preference for interfaces where at least the fd can be
captured some how. This should hopefully put a stop gap to such interfaces.

Then as for my concern on extending and adding new READING_* ID tags
for other future open-coded firmware calls, since:

a) You seem to be working on  a CONFIG_IMA_APPRAISE_FIRMWARE which would
enable similar functionality as firmware signing but in userspace

b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
CRDA, with an in-kernel interface. CRDA just did a signature check on
the regulatory.bin prior to tossing regulatory data over the kernel.

c) We've taken a position to *not* implement generic firmware singing
upstream in light of the fact that UEFI has pushed hw manufacturers
to implement FW singing in hardware, and for devices outside of these
we're happy to suggest IMA use.

d) It may be possible to have CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
depend on !CONFIG_IMA_APPRAISE_FIRMWARE this way we'd take a policy
that CONFIG_IMA_APPRAISE_FIRMWARE replaces in-kernel open coded
firmware singing facilities

Then I think it makes sense to adapt a policy of being *very careful* allowing
future open coded firmware signing efforts such as
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, and recommend folks to do work for things
like this through IMA with CONFIG_IMA_APPRAISE_FIRMWARE. This would limit our
needs to extend READING_* ID tags for new open coded firmwares.

Then as for the eye-sore you added for CONFIG_CFG80211_REQUIRE_SIGNED_REGDB,
in light of all this, I accept we have no other way to deal with it but with
#ifdefs.. however it could be dealt with, as helpers where if
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is not set we just set the id to
READING_FIRMWARE, ie, just keep the #ifdefs out of the actual code we read.
Perhaps it makes sense to throw this check into the helper:

        /* Already populated data member means we're loading into a buffer */
        if (fw_priv->data) {
                id = READING_FIRMWARE_PREALLOC_BUFFER;
                msize = fw_priv->allocated_size;
        }

PS: the work Alexei is doing with fork_usermode_blob() may sound similar
to the above legacy cases, but as I have noted before -- it already uses
an LSM hook to vet the data loaded as the data gets loaded as module.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote:
> On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> > On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> > > On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > > > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > > > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > > > 
> > > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > > > would differentiate between other firmware and the regulatory.db based
> > > > > > > > on the firmware's pathname.
> > > > > > > 
> > > > > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > > > > calls would have to have the check. A special LSM hook for just the
> > > > > > > regulatory db also doesn't make much sense.
> > > > > > 
> > > > > > All calls to request_firmware() are already going through this LSM
> > > > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > > > and the firmware's pathname.
> > > > > 
> > > > > Yes, but it would still be a strcmp() computation added for all
> > > > > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > > > > signature verification for the regulatory.db file.  One way to avoid this would
> > > > > be to add an LSM specific to the regulatory db
> > > > 
> > > > Casey already commented on this suggestion.
> > > 
> > > Sorry but I must have missed this, can you send me the email or URL where he did that?
> > > I never got a copy of that email I think.
> > 
> > My mistake.  I've posted similar patches for kexec_load and for the
> > firmware sysfs fallback, both call security_kernel_read_file().
> > Casey's comment was in regards to kexec_load[1], not for the sysfs
> > fallback mode.  Here's the link to the most recent version of the
> > kexec_load patches.[2]
> > 
> > [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
> > [2] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html
> 
> It seems I share Eric's concern on these threads are over general architecture,
> below some notes which I think may help for the long term on that regards.
> 
> In the firmware_loader case we have *one* subsystem which as open coded firmware
> signing -- the wireless subsystem open codes firmware verification by doing two
> request_firmware() calls, one for the regulatory.bin and one for regulatory.bin.p7s,
> and then it does its own check. In this patch set you suggested adding
> a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also
> adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of
> the old syfs loading facility.
> 
> My concerns are two fold for this case:
> 
> a) This would mean adding a new READING_* ID tag per any kernel mechanism which open
> codes its own signature verification scheme.

Yes, that's true.  In order to differentiate between different
methods, there needs to be some way of differentiating between them.
> 
> b) The way it was implemented was to do (just showing
> READING_FIRMWARE_REGULATORY_DB here):

The purpose for READING_FIRMWARE_REGULATORY_DB is different than for
adding enumerations for other firmware verification methods (eg.
fallback sysfs).  In this case, both IMA-appraisal and REGDB are being
called to verify the firmware signature.  Adding
READING_FIRMWARE_REGULATORY_DB was in order to coordinate between
them.

continued below ...

> 
> 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
>                 fw_priv->size = 0;
>                 rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
>                                                 msize, id);
> 
> This is eye-soring, and in turn would mean adding yet more #ifdefs for any
> code on the kernel which open codes other firmware signing efforts with
> its own kconfig...

Agreed, adding ifdef's is ugly.  As previously discussed, this code
will be removed.

To coordinate the signature verification, at build time either regdb
or IMA-appraisal firmware will be enabled.  At runtime, in the case
that regdb is enabled and a custom policy requires IMA-appraisal
firmware signature verification, then both signature verification
methods will verify the signatures.  If either fails, then the
signature verification will fail.

> I gather from reading the threads above that Eric's concerns are the re-use of
> an API for security to read files for something which is really not a file, but
> a binary blob of some sort and Casey's rebuttal is adding more hooks for small
> things is a bad idea.
> 
> In light of all this I'll say that the concerns Eric has are unfortunately
> too late, that ship has sailed eons ago. The old non-fd API for module loading
> init_module() calls       security_kernel_read_file(NULL, READING_MODULE). Your
> patch in this series adds security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK)
> for the old syfs loading facility.

It goes back even farther than that.  Commit 2e72d51 ("security:
introduce kernel_module_from_file hook") introduced calling
security_kernel_module_from_file() in copy_module_from_user().
Commit a1db74209483 ("module: replace copy_module_from_fd with kernel
version") replaced it with the call to security_kernel_read_file().

> So in this regard, I think we have no other option but what you suggested, to
> add a wrapper, say a security_kernel_read_blob() wrapper that calls
> security_kernel_read_file(NULL, id); and make the following legacy calls use
> it:
> 
>   o kernel/module.c for init_module()
>   o kexec_load()
>   o firmware loader sysfs facility
> 
> I think its fair then to add a new READING entry per functionality here
> *but* with the compromise that we *document* that such interfaces are
> discouraged, in preference for interfaces where at least the fd can be
> captured some how. This should hopefully put a stop gap to such interfaces.

Thanks!  Eric, are you on board with this?

> Then as for my concern on extending and adding new READING_* ID tags
> for other future open-coded firmware calls, since:
> 
> a) You seem to be working on  a CONFIG_IMA_APPRAISE_FIRMWARE which would
> enable similar functionality as firmware signing but in userspace.

There are a number of different builtin policies.  The "secure_boot"
builtin policy enables file signature verification for firmware,
kernel modules, kexec'ed image, and the IMA policy, but builtin
policies are enabled on the boot command line.

There are two problems:
- there's no way of configuring a builtin policy to verify firmware
signatures.
- CONFIG_IMA_APPRAISE is not fine enough grained.

The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
Kconfig options will require kernel modules, kexec'ed image, and the
IMA policy to be signed.

With this, option "d", below, will be possible.

> b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> CRDA, with an in-kernel interface. CRDA just did a signature check on
> the regulatory.bin prior to tossing regulatory data over the kernel.
> 
> c) We've taken a position to *not* implement generic firmware singing
> upstream in light of the fact that UEFI has pushed hw manufacturers
> to implement FW singing in hardware, and for devices outside of these
> we're happy to suggest IMA use.

There are a number of reasons that the kernel should be verifying
firmware signatures (eg. requiring a specific version of the firmware,
that was locally signed).

> d) It may be possible to have CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> depend on !CONFIG_IMA_APPRAISE_FIRMWARE this way we'd take a policy
> that CONFIG_IMA_APPRAISE_FIRMWARE replaces in-kernel open coded
> firmware singing facilities
> 
> Then I think it makes sense to adapt a policy of being *very careful* allowing
> future open coded firmware signing efforts such as
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, and recommend folks to do work for things
> like this through IMA with CONFIG_IMA_APPRAISE_FIRMWARE. This would limit our
> needs to extend READING_* ID tags for new open coded firmwares.
> 
> Then as for the eye-sore you added for CONFIG_CFG80211_REQUIRE_SIGNED_REGDB,
> in light of all this, I accept we have no other way to deal with it but with
> #ifdefs.. however it could be dealt with, as helpers where if
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is not set we just set the id to
> READING_FIRMWARE, ie, just keep the #ifdefs out of the actual code we read.

Assuming you agree with either REGDB or IMA-appraisal firmware being
configured at build, but allowing a custom policy to require firmware
signature verification based on IMA-appraisal at runtime, so that both
REGDB and IMA-appraisal firmware signature verification methods are
required, then the REGDB ifdef's can be removed.

> Perhaps it makes sense to throw this check into the helper:
> 
>         /* Already populated data member means we're loading into a buffer */
>         if (fw_priv->data) {
>                 id = READING_FIRMWARE_PREALLOC_BUFFER;
>                 msize = fw_priv->allocated_size;
>         }

Thanks, this looks good.  What IMA-appraisal should do with
READING_FIRMWARE_PREALLOC_BUFFER still needs to be determined.

> PS: the work Alexei is doing with fork_usermode_blob() may sound similar
> to the above legacy cases, but as I have noted before -- it already uses
> an LSM hook to vet the data loaded as the data gets loaded as module.

Thank you for the clarification.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 08:58:12AM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote:
> > 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
> >                 fw_priv->size = 0;
> >                 rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
> >                                                 msize, id);
> > 
> > This is eye-soring, and in turn would mean adding yet more #ifdefs for any
> > code on the kernel which open codes other firmware signing efforts with
> > its own kconfig...
> 
> Agreed, adding ifdef's is ugly.  As previously discussed, this code
> will be removed.
> 
> To coordinate the signature verification, at build time either regdb
> or IMA-appraisal firmware will be enabled. 

But not both, right?

> At runtime, in the case
> that regdb is enabled and a custom policy requires IMA-appraisal
> firmware signature verification, then both signature verification
> methods will verify the signatures.  If either fails, then the
> signature verification will fail.

OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
custom policy which requires IMA-appraisal for the certain firmware signature
verifications?

> > Then as for my concern on extending and adding new READING_* ID tags
> > for other future open-coded firmware calls, since:
> > 
> > a) You seem to be working on  a CONFIG_IMA_APPRAISE_FIRMWARE which would
> > enable similar functionality as firmware signing but in userspace.
> 
> There are a number of different builtin policies.  The "secure_boot"
> builtin policy enables file signature verification for firmware,
> kernel modules, kexec'ed image, and the IMA policy, but builtin
> policies are enabled on the boot command line.
> 
> There are two problems:
> - there's no way of configuring a builtin policy to verify firmware
> signatures.

I'm not too familiar with IMA however it sounds like you can extend the IMA
built-in policy on the boot command line. If so I'll note MODULE_FIRMWARE()
macro is typically used to annotate firmware description -- not all drivers use
this though for a few reasons, for instance once is that some names are
constructed dynamically at run time. Consider how some drivers rev firmware
with versions, and as they do this they want to keep certain features only for
certain firmware versions. Despite this lack of a direct 1-1 mapping for all
firmwares needed, I *believe* one current use case for this macro is to extract
required firmwares needed on early boot so they can be stashed into the
initramfs if you have these modules enabled on the initramfs. Dracut folks
can confirm but -- dracut *seems* broken then given the semantic gap I
mentioned above.

dracut-init.sh:    for _fw in $(modinfo -k $kernel -F firmware $1 2>/dev/null); do

If I read this correctly this just complains if the firmware file is
missing if the module is installed on initramfs and the firmware file
is not present. If so we have a current semantic gap and modules with
dynamic names are not handled. And its unclear which modules would be
affected. This is a not a big issue for dracut though since not all drivers
strive to be included on initramfs, unless their storage I suppose -- not
sure how common these storage drivers are for initramfs with dynamic firmware
names which do *not* use MODULE_FIRMWARE().

While the idea of MODULE_FIRMWARE() may need to be given some love or adjusted
to incorporate globs / regexps to fix this existing gap, or we come up with
something more reliable, if fixed, it in theory could end up being re-used to
enable you to extract and build policies for firmware signing at build time...

> - CONFIG_IMA_APPRAISE is not fine enough grained.
> 
> The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> Kconfig options will require kernel modules, kexec'ed image, and the
> IMA policy to be signed.

Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
doing firmware verification in userspace or in the kernel.

> With this, option "d", below, will be possible.

nit: To be clear I was not stating options, I was stating premises to
justify my recommendations.

> > b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> > CRDA, with an in-kernel interface. CRDA just did a signature check on
> > the regulatory.bin prior to tossing regulatory data over the kernel.
> > 
> > c) We've taken a position to *not* implement generic firmware singing
> > upstream in light of the fact that UEFI has pushed hw manufacturers
> > to implement FW singing in hardware, and for devices outside of these
> > we're happy to suggest IMA use.
> 
> There are a number of reasons that the kernel should be verifying
> firmware signatures (eg. requiring a specific version of the firmware,
> that was locally signed).

Oh I agree, Linux enterprise distributions also have a strong reason to
have this, so that for instance we only trust and run vendor-approved
signed firmware. Otherwise the driver should reject the firmware. Every
now and then enterprise distros may run into cases were certain customers
may run oddball firmwares, and its unclear if we expect proper functionality
with that firmware. Having some form of firmware signing would help with
this pipeline, but this is currently dealt with at the packaging, and
noting other than logs ensures the driver is using an intended firmware.
But these needs *IMHO* have not been enough to push to generalize a kernel
firmware signing facility.

If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
I'm happy to hear it.

> > d) It may be possible to have CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > depend on !CONFIG_IMA_APPRAISE_FIRMWARE this way we'd take a policy
> > that CONFIG_IMA_APPRAISE_FIRMWARE replaces in-kernel open coded
> > firmware singing facilities
> > 
> > Then I think it makes sense to adapt a policy of being *very careful* allowing
> > future open coded firmware signing efforts such as
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, and recommend folks to do work for things
> > like this through IMA with CONFIG_IMA_APPRAISE_FIRMWARE. This would limit our
> > needs to extend READING_* ID tags for new open coded firmwares.
> > 
> > Then as for the eye-sore you added for CONFIG_CFG80211_REQUIRE_SIGNED_REGDB,
> > in light of all this, I accept we have no other way to deal with it but with
> > #ifdefs.. however it could be dealt with, as helpers where if
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is not set we just set the id to
> > READING_FIRMWARE, ie, just keep the #ifdefs out of the actual code we read.
> 
> Assuming you agree with either REGDB or IMA-appraisal firmware being
> configured at build, but allowing a custom policy to require firmware
> signature verification based on IMA-appraisal at runtime, so that both
> REGDB and IMA-appraisal firmware signature verification methods are
> required, then the REGDB ifdef's can be removed.

Yes, this sounds fine.

But I'm also saying we can *live* with them if they are wrapped in #ifdefs with
functions sot hat when something is not required they are a no-op. Ideally if
we could avoid this it would be great and it seems you're saying its possible.
Keeping the #ifdefs with function wrappers would make sense as a trade-off if we
really are going to make toss firmware under CONFIG_IMA_APPRAISE_FIRMWARE and
want to review carefully the addition of new open coded firmware solutions (as
the regulatory.db work).

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Mon, 2018-05-14 at 19:28 +0000, Luis R. Rodriguez wrote:

[...] 

> > At runtime, in the case
> > that regdb is enabled and a custom policy requires IMA-appraisal
> > firmware signature verification, then both signature verification
> > methods will verify the signatures.  If either fails, then the
> > signature verification will fail.
> 
> OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
> still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
> custom policy which requires IMA-appraisal for the certain firmware signature
> verifications?

Right



> > There are two problems:
> > - there's no way of configuring a builtin policy to verify firmware
> > signatures.
> 
> I'm not too familiar with IMA however it sounds like you can extend the IMA
> built-in policy on the boot command line.

No, there are a couple of policies predefined in the kernel that can
be loaded by specifying them on the boot command line.  A custom
policy can be loaded later.  Only after specifying a policy on the
boot command line or loading a custom policy, does IMA do anything.


> > - CONFIG_IMA_APPRAISE is not fine enough grained.
> > 
> > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> > Kconfig options will require kernel modules, kexec'ed image, and the
> > IMA policy to be signed.
> 
> Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> doing firmware verification in userspace or in the kernel.

The kernel is verifying signatures.



> > There are a number of reasons that the kernel should be verifying
> > firmware signatures (eg. requiring a specific version of the firmware,
> > that was locally signed).
> 
> Oh I agree, Linux enterprise distributions also have a strong reason to
> have this, so that for instance we only trust and run vendor-approved
> signed firmware. Otherwise the driver should reject the firmware. Every
> now and then enterprise distros may run into cases were certain customers
> may run oddball firmwares, and its unclear if we expect proper functionality
> with that firmware. Having some form of firmware signing would help with
> this pipeline, but this is currently dealt with at the packaging, and
> noting other than logs ensures the driver is using an intended firmware.
> But these needs *IMHO* have not been enough to push to generalize a kernel
> firmware signing facility.

In order for IMA-appraisal to verify firmware signatures, the
signatures need to be distributed with the firmware.  Perhaps this
will be enough of an incentive for distros to start including firmware
signatures in the packages.

> If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
> I'm happy to hear it.

The functionality has been there since commit 5a9196d ("ima: add
support for measuring and appraising firmware").  The
security_kernel_fw_from_file() hook was later replaced with the
generic security_kernel_read_file() hook.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> On Mon, 2018-05-14 at 19:28 +0000, Luis R. Rodriguez wrote:
> > > - CONFIG_IMA_APPRAISE is not fine enough grained.
> > > 
> > > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> > > Kconfig options will require kernel modules, kexec'ed image, and the
> > > IMA policy to be signed.
> > 
> > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> > doing firmware verification in userspace or in the kernel.
> 
> The kernel is verifying signatures.
> 
> > > There are a number of reasons that the kernel should be verifying
> > > firmware signatures (eg. requiring a specific version of the firmware,
> > > that was locally signed).
> > 
> > Oh I agree, Linux enterprise distributions also have a strong reason to
> > have this, so that for instance we only trust and run vendor-approved
> > signed firmware. Otherwise the driver should reject the firmware. Every
> > now and then enterprise distros may run into cases were certain customers
> > may run oddball firmwares, and its unclear if we expect proper functionality
> > with that firmware. Having some form of firmware signing would help with
> > this pipeline, but this is currently dealt with at the packaging, and
> > noting other than logs ensures the driver is using an intended firmware.
> > But these needs *IMHO* have not been enough to push to generalize a kernel
> > firmware signing facility.
> 
> In order for IMA-appraisal to verify firmware signatures, the
> signatures need to be distributed with the firmware.  Perhaps this
> will be enough of an incentive for distros to start including firmware
> signatures in the packages.

Best to poke the maintainers about that... We have been sending mixed messages
about firmware signing over years now. Josh, heads up the new one is we can
do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
bounce you a few emails related to this.

> > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
> > I'm happy to hear it.
> 
> The functionality has been there since commit 5a9196d ("ima: add
> support for measuring and appraising firmware").  The
> security_kernel_fw_from_file() hook was later replaced with the
> generic security_kernel_read_file() hook.

Groovy, its unclear from the code on that commit how this is done, so I
suppose I need to study this a bit more. Josh, do you grok it?

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Josh Boyer]

On Mon, May 14, 2018 at 11:27 PM Luis R. Rodriguez <mcgrof@kernel.org>
wrote:

> On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> > On Mon, 2018-05-14 at 19:28 +0000, Luis R. Rodriguez wrote:
> > > > - CONFIG_IMA_APPRAISE is not fine enough grained.
> > > >
> > > > The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
> > > > Kconfig options will require kernel modules, kexec'ed image, and the
> > > > IMA policy to be signed.
> > >
> > > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will
be
> > > doing firmware verification in userspace or in the kernel.
> >
> > The kernel is verifying signatures.
> >
> > > > There are a number of reasons that the kernel should be verifying
> > > > firmware signatures (eg. requiring a specific version of the
firmware,
> > > > that was locally signed).
> > >
> > > Oh I agree, Linux enterprise distributions also have a strong reason
to
> > > have this, so that for instance we only trust and run vendor-approved
> > > signed firmware. Otherwise the driver should reject the firmware.
Every
> > > now and then enterprise distros may run into cases were certain
customers
> > > may run oddball firmwares, and its unclear if we expect proper
functionality
> > > with that firmware. Having some form of firmware signing would help
with
> > > this pipeline, but this is currently dealt with at the packaging, and
> > > noting other than logs ensures the driver is using an intended
firmware.
> > > But these needs *IMHO* have not been enough to push to generalize a
kernel
> > > firmware signing facility.
> >
> > In order for IMA-appraisal to verify firmware signatures, the
> > signatures need to be distributed with the firmware.  Perhaps this
> > will be enough of an incentive for distros to start including firmware
> > signatures in the packages.

> Best to poke the maintainers about that... We have been sending mixed
messages
> about firmware signing over years now. Josh, heads up the new one is we
can
> do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
> bounce you a few emails related to this.

> > > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this
functionality somehow
> > > I'm happy to hear it.
> >
> > The functionality has been there since commit 5a9196d ("ima: add
> > support for measuring and appraising firmware").  The
> > security_kernel_fw_from_file() hook was later replaced with the
> > generic security_kernel_read_file() hook.

> Groovy, its unclear from the code on that commit how this is done, so I
> suppose I need to study this a bit more. Josh, do you grok it?

I haven't looked to be honest.  I don't do much in the way of kernel
maintenance on the distro side any longer.  You already have David copied
and I've added Justin Forbes and Laura Abbott to cover Fedora.

One aspect that was always a concern to some is whether the firmware files
were modified directly to have the signature attached to them.  That may
run afoul of the "no modification" license that most blobs are shipped
under.  Does IMA have the signatures for the files stored in xattrs or in
some other detached manner?

josh

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-15 at 08:32 -0400, Josh Boyer wrote:

> One aspect that was always a concern to some is whether the firmware files
> were modified directly to have the signature attached to them.  That may
> run afoul of the "no modification" license that most blobs are shipped
> under.  Does IMA have the signatures for the files stored in xattrs or in
> some other detached manner?

They're stored as xattrs.  RPM has support for including file
signatures in the RPM header.

Mimi

[PATCH 4/6] ima: coordinate with signed regulatory.db

[Mimi Zohar]

Based on IMA policy, measure and appraise regulatory.db firmware as
usual, but on signature verification failure rely on regdb signature.

For systems wanting IMA-appraisal enforcement on all firmware, including
regdb, do not enable CONFIG_CFG80211_REQUIRE_SIGNED_REGDB.

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>
---
 security/integrity/ima/ima_main.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8759280dccf6..71b5a51c6709 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -468,6 +468,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 
 static int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
+	[READING_FIRMWARE_REGULATORY_DB] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
 	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
 	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
@@ -515,8 +516,12 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 
 	func = read_idmap[read_id] ?: FILE_CHECK;
 	security_task_getsecid(current, &secid);
-	return process_measurement(file, current_cred(), secid, buf, size,
-				   MAY_READ, func, 0);
+	ret = process_measurement(file, current_cred(), secid, buf, size,
+				  MAY_READ, func, 0);
+
+	/* Co-ordination with signed regdb */
+	if (ret < -EACCES && read_id == READING_FIRMWARE_REGULATORY_DB)
+		return 0;
 }
 
 static int __init init_ima(void)
-- 
2.7.5

[PATCH 5/6] ima: verify kernel firmware signatures when using a preallocated buffer

[Mimi Zohar]

Don't differentiate between kernel_read_file_id READING_FIRMWARE and
READING_FIRMWARE_PREALLOC_BUFFER enumerations.

Fixes: a098ecd firmware: support loading into a pre-allocated buffer (since 4.8)
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: Serge E. Hallyn <serge@hallyn.com>
Cc: Stephen Boyd <stephen.boyd@linaro.org>
---
 security/integrity/ima/ima_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 71b5a51c6709..eb9c273ab81d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -469,6 +469,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 static int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
 	[READING_FIRMWARE_REGULATORY_DB] = FIRMWARE_CHECK,
+	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
 	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
 	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
-- 
2.7.5

[RFC PATCH 6/6] ima: prevent loading firmware into a pre-allocated buffer

[Mimi Zohar]

Question: can the device access the pre-allocated buffer at any time?

By allowing devices to request firmware be loaded directly into a
pre-allocated buffer, will this allow the device access to the firmware
before the kernel has verified the firmware signature?

Is it dependent on the type of buffer allocated (eg. DMA)?  For example,
qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent().

With an IMA policy requiring signed firmware, this patch would prevent
loading firmware into a pre-allocated buffer.

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: Serge E. Hallyn <serge@hallyn.com>
Cc: Stephen Boyd <stephen.boyd@linaro.org>
---
 security/integrity/ima/ima_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index eb9c273ab81d..3098131f77c4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -454,6 +454,15 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		return 0;
 	}
 
+	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("Prevent device from accessing firmware prior to verifying the firmware signature.\n");
+			return -EACCES;
+		}
+		return 0;
+	}
+
 	if (read_id == READING_FIRMWARE_FALLBACK) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
-- 
2.7.5

Re: [RFC PATCH 6/6] ima: prevent loading firmware into a pre-allocated buffer

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 09:48:23AM -0400, Mimi Zohar wrote:
> Question: can the device access the pre-allocated buffer at any time?
> 
> By allowing devices to request firmware be loaded directly into a
> pre-allocated buffer, will this allow the device access to the firmware
> before the kernel has verified the firmware signature?
> 
> Is it dependent on the type of buffer allocated (eg. DMA)?  For example,
> qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent().
> 
> With an IMA policy requiring signed firmware, this patch would prevent
> loading firmware into a pre-allocated buffer.

Android folks went silent on the other thread .. Best poke them there?

  Luis
> 
> 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: Serge E. Hallyn <serge@hallyn.com>
> Cc: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  security/integrity/ima/ima_main.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index eb9c273ab81d..3098131f77c4 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -454,6 +454,15 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  		return 0;
>  	}
>  
> +	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> +		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +			pr_err("Prevent device from accessing firmware prior to verifying the firmware signature.\n");
> +			return -EACCES;
> +		}
> +		return 0;
> +	}
> +
>  	if (read_id == READING_FIRMWARE_FALLBACK) {
>  		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
>  		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> -- 
> 2.7.5
> 
> 

-- 
Do not panic