[PATCH v3 0/5] firmware_class: extensible firmware API

2015-12-23 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

This v3 builds up on the last v2 series from October [0], I had not send
out any updates after that as we had the kernel summit and figured it'd
be best to hash out any kinks there. This patch set *only* provides a new
set of extensible firmware helpers with strong semantics and which avoids
the usermode helper completely. Support for firmware signing was discussed
and we seem to have come to an agreement that it is needed, and how we'd
handle certificates. This patch set does not address that -- that will be
handled in another patch set. If you're curious to take a peak at how that
will be handled though please have a look at the firmware enhancements wiki
page [1], it provides Andy's latest recommendations.

Changes since last v2 (thanks to Josh Boyer and Johannes Berg for feedback):

  * remove WARN_ON() and BUG_ON() - josh
  * make union sysdata_file_cbs const - johannes
  * refer to wiki on future enhancements page
  * upon request by gregkh added myself as maintainer

Please note, I will be actually leaving on vacation far away from computers
tomorrow, so posting this as it was overdue, I just had not had time to get
to this yet. I'll be back January 13th. These patches are based on top of
linux-next tag next-20151223.

[0] 
http://lkml.kernel.org/r/1443721449-22882-1-git-send-email-mcg...@do-not-panic.com
[1] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements

David Howells (2):
  firmware: fold successful fw read early
  firmware: generalize reading file contents as a helper

Luis R. Rodriguez (3):
  firmware: generalize "firmware" as "system data" helpers
  firmware: move completing fw into a helper
  firmware: add an extensible system data helpers

 Documentation/firmware_class/system_data.txt |  79 ++
 MAINTAINERS  |   4 +-
 drivers/base/firmware_class.c| 385 ---
 include/linux/sysdata.h  | 212 +++
 4 files changed, 642 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/5] firmware: move completing fw into a helper

2015-12-23 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

This will be re-used later through a new extensible interface.

Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6f5fcda71a60..d8148aa89b01 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,6 +322,15 @@ fail:
return rc;
 }
 
+static void fw_finish_direct_load(struct device *device,
+ struct firmware_buf *buf)
+{
+   mutex_lock(_lock);
+   set_bit(FW_STATUS_DONE, >status);
+   complete_all(>completion);
+   mutex_unlock(_lock);
+}
+
 static int fw_get_filesystem_firmware(struct device *device,
   struct firmware_buf *buf)
 {
@@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device 
*device,
if (!rc) {
dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
-   mutex_lock(_lock);
-   set_bit(FW_STATUS_DONE, >status);
-   complete_all(>completion);
-   mutex_unlock(_lock);
+   fw_finish_direct_load(device, buf);
}
 
return rc;
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/5] firmware: generalize reading file contents as a helper

2015-12-23 Thread Luis R. Rodriguez 
From: David Howells <dhowe...@redhat.com>

We'll want to reuse this same code later in order to read
two separate types of file contents. This generalizes
fw_read_file_contents() for reading a file and rebrands it
as fw_read_file(). This new caller is now generic: the path
used can be arbitrary and the caller is also agnostic to the
firmware_class code now, this begs the possibility of code
re-use with other similar callers in the kernel. For instance
in the future we may want to share a solution with:

- firmware_class: fw_read_file()
- module: kernel_read()
- kexec: copy_file_fd()

While at it this also cleans up the exit paths on fw_read_file().

Reviewed-by: Josh Boyer <jwbo...@fedoraproject.org>
Signed-off-by: David Howells <dhowe...@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 62 +++
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e10a5349aa61..cd51a90ed012 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -291,34 +291,51 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher 
priority than default path");
 
-static int fw_read_file_contents(struct file *file, struct firmware_buf 
*fw_buf)
+/*
+ * Read the contents of a file.
+ */
+static int fw_read_file(const char *path, void **_buf, size_t *_size)
 {
-   int size;
+   struct file *file;
+   size_t size;
char *buf;
int rc;
 
+   file = filp_open(path, O_RDONLY, 0);
+   if (IS_ERR(file))
+   return PTR_ERR(file);
+
+   rc = -EINVAL;
if (!S_ISREG(file_inode(file)->i_mode))
-   return -EINVAL;
+   goto err_file;
size = i_size_read(file_inode(file));
if (size <= 0)
-   return -EINVAL;
+   goto err_file;
+   rc = -ENOMEM;
buf = vmalloc(size);
if (!buf)
-   return -ENOMEM;
+   goto err_file;
+
rc = kernel_read(file, 0, buf, size);
+   if (rc < 0)
+   goto err_buf;
if (rc != size) {
-   if (rc > 0)
-   rc = -EIO;
-   goto fail;
+   rc = -EIO;
+   goto err_buf;
}
+
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
-   goto fail;
-   fw_buf->data = buf;
-   fw_buf->size = size;
+   goto err_buf;
+
+   *_buf = buf;
+   *_size = size;
return 0;
-fail:
+
+err_buf:
vfree(buf);
+err_file:
+   fput(file);
return rc;
 }
 
@@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
 }
 
 static int fw_get_filesystem_firmware(struct device *device,
-  struct firmware_buf *buf)
+ struct firmware_buf *buf)
 {
int i, len;
int rc = -ENOENT;
-   char *path;
+   char *path = NULL;
 
path = __getname();
if (!path)
return -ENOMEM;
 
+   /*
+* Try each possible firmware blob in turn till one doesn't produce
+* ENOENT.
+*/
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-   struct file *file;
-
/* skip the unset customized path */
if (!fw_path[i][0])
continue;
@@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device 
*device,
break;
}
 
-   file = filp_open(path, O_RDONLY, 0);
-   if (IS_ERR(file))
-   continue;
-   rc = fw_read_file_contents(file, buf);
-   fput(file);
+   rc = fw_read_file(path, >data, >size);
if (rc == 0) {
dev_dbg(device, "system data: direct-loading firmware 
%s\n",
buf->fw_id);
fw_finish_direct_load(device, buf);
goto out;
-   } else
+   } else if (rc != -ENOENT) {
dev_warn(device, "system data, attempted to load %s, 
but failed with error %d\n",
 path, rc);
+   goto out;
+   }
}
 out:
__putname(path);
-
return rc;
 }
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/5] firmware: add an extensible system data helpers

2015-12-23 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

The firmware API has evolved over the years slowly, as it
grows we extend it by adding new routines or at times we extend
existing routines with more or less arguments. This doesn't scale
well, when new arguments are added to existing routines it means
we need to traverse the kernel with a slew of collateral
evolutions to adjust old driver users. The firmware API is also
now being used for things outside of the scope of what typically
would be considered "firmware", an example here is the p54 driver
enables users to provide a custom EEPROM through this interface.
Another example is optional CPU microcode updates.

There are other subsystems which would like to make use of the
APIs for similar things but have different requirements and
criteria which they'd like to be met for the requested file.
If different requirements are needed it would again mean adding
more arguments and making a slew of collateral evolutions, or
adding yet-another-new-API-call.

Instead of extending the existing firmware API even more this
provides an new extensible API which can be used to supercede the
old firmware API without causing a series of collateral evolutions
as requirements grow. This leaves the old firmware API as-is,
ignores all consideration for usermode-helpers, labels the new API
to reflect its broad use outside of the scope of firmware: system
data helpers, and builds on top of the original firmware core code.

The new extensible "system data" set of helpers accepts that there
really are only two types of requests for accessing system data:

a) synchronous requests
b) asynchronous requests

Both of these requests may have a different set of requirements
which must be met. These requirements can simply be passed as a
descriptors to each type of request. The descriptor can be extended
over time to support different requirements as the kernel evolves.

Using the new system data helpers is only necessary if you have
requirements outside of what the existing old firmware API accepts.
We encourage developers to leave the old API as-is and extend the
new descriptors and system data code to provide support for new
features.

A few simple features added as part of the new set of system data
request APIs, other than making the new API easily extensible for
the future:

 - By default the kernel will free the system data file for you after
   your callbacks are called, you however are allowed to request to that
   you wish to keep the system data file on the descriptor. This is
   dealt with by requiring a consumer callback for the system data file.
 - Allow both asynchronous and synchronous request to specify that system data
   files are optional. With the old APIs we had added one full API call,
   request_firmware_direct() just for this purpose -- although it should be
   noted another of its goal was to also skip the usermode helper.
   The system data request APIs allow for you to annotate that a system
   data file is optional for both synchronous or asynchronous requests
   through the same two basic set of APIs.
 - Usermode helpers are completely ignored, always
 - The system data request APIs currently match the old synchronous firmware
   API calls to refcounted firmware_class module, but it should be easy
   to add support now to enable also refcounting the caller's module
   should it be be needed. Likewise the system data request APIs match the
   old asynchronous firmware API call and refcounts the caller's module.

In order to try to help phase out user mode helpers this makes no use of
the old user mode helper code *at all*, and if we wish to can easily
phase this code out with time then.

Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 Documentation/firmware_class/system_data.txt |  79 
 MAINTAINERS  |   4 +-
 drivers/base/firmware_class.c| 291 +++
 include/linux/sysdata.h  | 212 +++
 4 files changed, 585 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

diff --git a/Documentation/firmware_class/system_data.txt 
b/Documentation/firmware_class/system_data.txt
new file mode 100644
index ..ab509243455a
--- /dev/null
+++ b/Documentation/firmware_class/system_data.txt
@@ -0,0 +1,79 @@
+System data requests API
+
+
+As the kernel evolves we keep extending the firmware_class set of APIs
+with more or less arguments, this creates a slew of collateral evolutions.
+The set of users of firmware request APIs has also grown now to include
+users which are not looking for "firmware" per se, but instead general
+system data files which for one reason or another has been decided to be
+kept oustide of the kernel, and/or to allow dynamic updates. The system data
+request set of APIs add

[PATCH v3 3/5] firmware: fold successful fw read early

2015-12-23 Thread Luis R. Rodriguez 
From: David Howells <dhowe...@redhat.com>

We'll be folding in some more checks on fw_read_file_contents(),
this will make the success case easier to follow.

Signed-off-by: David Howells <dhowe...@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d8148aa89b01..e10a5349aa61 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device 
*device,
continue;
rc = fw_read_file_contents(file, buf);
fput(file);
-   if (rc)
+   if (rc == 0) {
+   dev_dbg(device, "system data: direct-loading firmware 
%s\n",
+   buf->fw_id);
+   fw_finish_direct_load(device, buf);
+   goto out;
+   } else
dev_warn(device, "system data, attempted to load %s, 
but failed with error %d\n",
 path, rc);
-   else
-   break;
}
+out:
__putname(path);
 
-   if (!rc) {
-   dev_dbg(device, "system data: direct-loading firmware %s\n",
-   buf->fw_id);
-   fw_finish_direct_load(device, buf);
-   }
-
return rc;
 }
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/5] firmware: generalize "firmware" as "system data" helpers

2015-12-23 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.

Annotate this before we extend firmare_class more,
as this is expected. We want to generalize the code
as much as possible.

Cc: Rusty Russell <ru...@rustcorp.com.au>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: David Howells <dhowe...@redhat.com>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Casey Schaufler <ca...@schaufler-ca.com>
Cc: Ming Lei <ming@canonical.com>
Cc: Takashi Iwai <ti...@suse.de>
Cc: Vojtěch Pavlík <vojt...@suse.cz>
Cc: Kyle McMartin <k...@kernel.org>
Cc: Matthew Garrett <mj...@srcf.ucam.org>
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450e75bd..6f5fcda71a60 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device 
*device,
rc = fw_read_file_contents(file, buf);
fput(file);
if (rc)
-   dev_warn(device, "firmware, attempted to load %s, but 
failed with error %d\n",
-   path, rc);
+   dev_warn(device, "system data, attempted to load %s, 
but failed with error %d\n",
+path, rc);
else
break;
}
__putname(path);
 
if (!rc) {
-   dev_dbg(device, "firmware: direct-loading firmware %s\n",
+   dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
mutex_lock(_lock);
set_bit(FW_STATUS_DONE, >status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, 
const char *name,
}
 
if (fw_get_builtin_firmware(firmware, name)) {
-   dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+   dev_dbg(device, "system data: using built-in system data%s\n", 
name);
return 0; /* assigned */
}
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 6/7] ima: measure and appraise the IMA policy itself

2015-12-21 Thread Luis R. Rodriguez 
On Sun, Dec 20, 2015 at 12:11:04AM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 23:03 +0100, Luis R. Rodriguez wrote:
> > On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > index 8a45576..4d149c9 100644
> > > --- a/security/integrity/iint.c
> > > +++ b/security/integrity/iint.c
> > > @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char 
> > > **data)
> > >   return rc;
> > >   }
> > >  
> > > + if (!S_ISREG(file_inode(file)->i_mode)) {
> > > + rc = -EACCES;
> > > + goto out;
> > > + }
> > > +
> > >   size = i_size_read(file_inode(file));
> > >   if (size <= 0)
> > >   goto out;
> > 
> > This hunk seems to be unrelated to this patch? If so can it be split out?
> 
> Yes, sure.   Up to now, 'cat' was used to load the IMA policy.   A lot
> of the problems related to opening and reading a file were hidden.  So
> besides making sure that only a regular file is opened, what other
> things should we be checking?   For example,  do we permit the kernel to
> read NFS mounted files?   Should the kernel be limited to opening only
> local files?   Answering these questions becomes important as we move to
> a single kernel file read function.

Answering this properly should include effort to study and consolidate other
kernel read routines. From the little that I've so far reviewed these we don't
have much differences in requirements even between this IMA one and the sound 
one
you just pointed out, the small changes for correctness however are important to
capture for all. Because of this we should be able to still provide a generic
read routine that takes all considerations into account, enables flexibility
but more importantly shares the best practices for correctness.

I can understand you might want to not wait for that yet, and I think that's
fine, but lets work in parallel to make that happen.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/7] ima: load policy using path

2015-12-21 Thread Luis R. Rodriguez 
On Thu, Dec 17, 2015 at 11:33 AM, Luis R. Rodriguez <mcg...@suse.com> wrote:
> Please no, instead of adding yet-another kernel file-loading facility which is
> likely error prone we should consolidate *all kernel file-loading facilities*
> into a *common generic shared one*. So please work to make that happen since 
> you
> need yet-another user for it.m.com
>
> Since you need yet-naother kernel file-loader please do the work to generalize
> it, or at least try it.

As per review in another thread with Mimi we determined they're not
adding a *new* reader, but using an existing one. The possible issues
with early read and pivot_root() as well as possible considerations
for a common user mode helper are still relevant for when we
generalize a common kernel loader. Mimi has also pointed out a few
other kernel loaders. It seems we'll try to tackle this after the
holidays. To help keep track of progress and consolidate notes on this
I've stuffed details about this on this wiki:

http://kernelnewbies.org/KernelProjects/common-kernel-loader

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

2015-12-21 Thread Luis R. Rodriguez 
On Sat, Dec 19, 2015 at 11:44:41PM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 22:06 +0100, Luis R. Rodriguez wrote:
> > On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 8524450..dcd902f 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -29,6 +29,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  
> > > @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> > > struct firmware_buf *fw_buf)
> > >   buf = vmalloc(size);
> > >   if (!buf)
> > >   return -ENOMEM;
> > > - rc = kernel_read(file, 0, buf, size);
> > > - if (rc != size) {
> > > - if (rc > 0)
> > > - rc = -EIO;
> > > +
> > > + rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> > > + if (rc == -EIO)
> > >   goto fail;
> > > + else if (rc != -EOPNOTSUPP) {
> > > + rc = kernel_read(file, 0, buf, size);
> > > + if (rc != size) {
> > > + if (rc > 0)
> > > + rc = -EIO;
> > > + goto fail;
> > > + }
> > >   }
> > >   rc = security_kernel_fw_from_file(file, buf, size);
> > >   if (rc)
> > 
> > This is one way, the other way is to generalize the kernel-read from path
> > routine. I have some changes which help generalize this routine a bit so
> > help on review there would be appreciated. 
> 
> Sure.  Where are the patches?

http://lkml.kernel.org/r/1431996325-8840-2-git-send-email-mcg...@do-not-panic.com

I'll post these in PATCH form now.

> > I'm personally indifferent
> > as to needing or not *now* a generic kernel read routine that is shared
> > for this purpose *but* since this patch set *also* seems to be adding
> > yet-another file reading I'm more inclined to wish for that to be addressed
> > now instead.
> > 
> > Please let me know if this logic is fair.
> 
> Commit e3c4abb - "integrity: define a new function
> integrity_read_file()" defined a method of reading a file from the
> kernel.  It's used to load an x509 key onto the IMA keyring for systems
> without an initramfs.   Dmitry's patch, included in this patch set,
> calls this function to load the IMA policy as well.  So this patch set
> isn't defining a new function for reading a file from the kernel.  It's
> using an existing one.

I see thanks, 

> FYI, sound/sound_firmware.c: do_mod_firmware_load() also reads a file.

Thanks, this should be generalized as well the only reason for a different
implementation I see here is the size constraint to 128k max. I think we can
move that crap check out to take advantage of a common read.

The integrity_read_file() seems rather generic as well and just skips
locking checks and security checks, a generic solution doesn't have to happen
now because as you note this has been in the kernel for a while.

Eventually, once we generalize a common read perhaps we should stuff this
into VFS common code and provide arguments to enable callers to provide 
restrictions or requirements. Let's work together on that after the holidays.

Let's consolidate notes here:

http://kernelnewbies.org/KernelProjects/common-kernel-loader

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs

2015-12-17 Thread Luis R. Rodriguez 
On Thu, Dec 17, 2015 at 07:32:10AM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 14:45 +0800, Dave Young wrote:
> > On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> > > There's a lot of code duplication for reading a file by the kernel (eg.
> > > kexec, firmware, kernel modules, ...).   Each place does it just a bit
> > > differently than the other.   There should be a single function for
> > > reading and calculating the file hash at the same time.
> > 
> > If you want to address the duplication for reading file, IMHO you can
> > introduce a general interface to read file in kernel space. But I do not
> > think the reading + calculating only interface is a good way.
> 
> Ok.  As described above, the call would read the buffer into memory and
> then call IMA to calculate the buffer hash.
> 
> (If someone else is interested in getting involved in kernel
> development, cleaning up this code duplication is a good, relatively
> small, self contained project.)

I'm with Dave though, I realize that's work but I can help do it with you.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/7] ima: load policy using path

2015-12-17 Thread Luis R. Rodriguez 
The subject should probably be: ima: add support to load policy from path

Cc'ing Roberts who originally wanted SELinux file policy signing capabilities.
Also Greg, who is reviewing the sysdata file changes I had proposed which would
provide a generic file loading facility for modules, and later a generic file
signing checker. Cc'ing Linus for any feedback on the possible issues he might
foresee if we all go gung-ho on "kernel file loading", as this is where it
seems some folks might be going. I *think* the user hint might help close the
semantic gap observed on using a file loader on firmware_class, but perhaps he
or other might have some other corner cases in mind we should also consider.

Note that the crux between using a generic kernel file loader and the common
"sysdata" file loader will be that sysdata file users (wireless would be one
getting rid of CRDA) and a core kernel file loader (IMA could be one, likewise
perhaps SELinux if it follows suit in design as in this patch) is that the core
kernel file loader would allow arbitrary file paths, and the sysdata users
would have stuff in /lib/firmware/ paths (and therefore also have things in the
linux-firmware tree).

On Tue, Dec 08, 2015 at 01:01:20PM -0500, Mimi Zohar wrote:
> From: Dmitry Kasatkin 
> 
> Currently the IMA policy is loaded by writing the policy rules to
> '/ima/policy'.

> That way the policy cannot be measured or appraised. This patch extends the
> policy loading interface with the possibility to load the policy using a
> pathname. The policy can be loaded like:
> 
> echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy

I don't think this is as clear, you can just say something like:

-
We currently cannot do appraisal or signature vetting of IMA policies
since we currently can only load IMA policies by writing the contents
of the polify directly in, as follows:

cat policy-file > /ima/policy

If we provide the kernel the path to the IMA policy so it can load
the policy itself it'd be able to later appraise or vet for the file
signature if it had one. This adds support to load IMA policies
with a given path as follows:

echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy
-

> 
> Signed-off-by: Dmitry Kasatkin 
> Signed-off-by: Mimi Zohar 
> ---
>  security/integrity/iint.c   |  4 +---
>  security/integrity/ima/ima_fs.c | 39 ++-
>  security/integrity/integrity.h  |  2 +-
>  3 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 2de9c82..54b51a4 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -203,10 +203,8 @@ int integrity_kernel_read(struct file *file, loff_t 
> offset,
>   * This is function opens a file, allocates the buffer of required
>   * size, read entire file content to the buffer and closes the file
>   *
> - * It is used only by init code.
> - *
>   */
> -int __init integrity_read_file(const char *path, char **data)
> +int integrity_read_file(const char *path, char **data)
>  {
>   struct file *file;
>   loff_t size;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index eebb985..f902b6b 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -258,6 +258,40 @@ static const struct file_operations 
> ima_ascii_measurements_ops = {
>   .release = seq_release,
>  };
>  
> +static ssize_t ima_read_policy(char *path)
> +{
> + char *data, *datap;
> + int rc, size, pathlen = strlen(path);
> + char *p;
> +
> + /* remove \n */
> + datap = path;
> + strsep(, "\n");
> +
> + rc = integrity_read_file(path, );
> + if (rc < 0)
> + return rc;
> +
> + size = rc;
> + datap = data;
> +
> + while (size > 0 && (p = strsep(, "\n"))) {
> + pr_debug("rule: %s\n", p);
> + rc = ima_parse_add_rule(p);
> + if (rc < 0)
> + break;
> + size -= rc;
> + }
> +
> + kfree(data);
> + if (rc < 0)
> + return rc;
> + else if (size)
> + return -EINVAL;
> + else
> + return pathlen;
> +}
> +

Please no, instead of adding yet-another kernel file-loading facility which is
likely error prone we should consolidate *all kernel file-loading facilities*
into a *common generic shared one*. So please work to make that happen since you
need yet-another user for it. I've done some initial homework already on a few
prominent common users. I say "initial" homework as my search was rather crude 
on
'git grep' and not done using semantic parsers to see if I "search for file
loaders" through semantic means. This later search may be optional, but it
would help

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

2015-12-17 Thread Luis R. Rodriguez 
On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..dcd902f 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> struct firmware_buf *fw_buf)
>   buf = vmalloc(size);
>   if (!buf)
>   return -ENOMEM;
> - rc = kernel_read(file, 0, buf, size);
> - if (rc != size) {
> - if (rc > 0)
> - rc = -EIO;
> +
> + rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> + if (rc == -EIO)
>   goto fail;
> + else if (rc != -EOPNOTSUPP) {
> + rc = kernel_read(file, 0, buf, size);
> + if (rc != size) {
> + if (rc > 0)
> + rc = -EIO;
> + goto fail;
> + }
>   }
>   rc = security_kernel_fw_from_file(file, buf, size);
>   if (rc)

This is one way, the other way is to generalize the kernel-read from path
routine. I have some changes which help generalize this routine a bit so
help on review there would be appreciated. I'm personally indifferent
as to needing or not *now* a generic kernel read routine that is shared
for this purpose *but* since this patch set *also* seems to be adding
yet-another file reading I'm more inclined to wish for that to be addressed
now instead.

Please let me know if this logic is fair.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 6/7] ima: measure and appraise the IMA policy itself

2015-12-17 Thread Luis R. Rodriguez 
On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 8a45576..4d149c9 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char **data)
>   return rc;
>   }
>  
> + if (!S_ISREG(file_inode(file)->i_mode)) {
> + rc = -EACCES;
> + goto out;
> + }
> +
>   size = i_size_read(file_inode(file));
>   if (size <= 0)
>   goto out;

This hunk seems to be unrelated to this patch? If so can it be split out?

> @@ -232,13 +238,18 @@ int integrity_read_file(const char *path, char **data)
>   goto out;
>   }
>  
> - rc = integrity_kernel_read(file, 0, buf, size);
> + rc = ima_read_and_process_file(file, read_func, buf, size);
> + if (rc == -EOPNOTSUPP) {
> + rc = integrity_kernel_read(file, 0, buf, size);
> + if (rc > 0 && rc != size)
> + rc = -EIO;
> + }
>   if (rc < 0)
>   kfree(buf);
> - else if (rc != size)
> - rc = -EIO;
> - else
> + else {
> + rc = size;
>   *data = buf;
> + }
>  out:
>   fput(file);
>   return rc;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 548b258..40a24c3 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -180,6 +180,7 @@ int ima_policy_show(struct seq_file *m, void *v);
>  #define IMA_APPRAISE_LOG 0x04
>  #define IMA_APPRAISE_MODULES 0x08
>  #define IMA_APPRAISE_FIRMWARE0x10
> +#define IMA_APPRAISE_POLICY  0x20
>  
>  #ifdef CONFIG_IMA_APPRAISE
>  int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> diff --git a/security/integrity/ima/ima_appraise.c 
> b/security/integrity/ima/ima_appraise.c
> index b83049b..1e1a759 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -79,6 +79,7 @@ enum integrity_status ima_get_cache_status(struct 
> integrity_iint_cache *iint,
>   case FIRMWARE_CHECK:
>   case KEXEC_CHECK:
>   case INITRAMFS_CHECK:
> + case POLICY_CHECK:
>   return iint->ima_read_status;
>   case FILE_CHECK:
>   default:

Hrm this uses an int for the func.

> @@ -102,6 +103,7 @@ static void ima_set_cache_status(struct 
> integrity_iint_cache *iint,
>   case FIRMWARE_CHECK:
>   case KEXEC_CHECK:
>   case INITRAMFS_CHECK:
> + case POLICY_CHECK:
>   iint->ima_read_status = status;
>   break;
>   case FILE_CHECK:

This uses an enum.

> @@ -126,6 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache 
> *iint, int func)
>   case FIRMWARE_CHECK:
>   case KEXEC_CHECK:
>   case INITRAMFS_CHECK:
> + case POLICY_CHECK:
>   iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
>   break;
>   case FILE_CHECK:

This uses an enum.

All of these have a common set of funcs that will do similar things, what about
just OR'ing them up in one place? That would make future additions one line
instead of 3.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] firmware: generalize "firmware" as "system data" helpers

2015-12-17 Thread Luis R. Rodriguez 
On Thu, Oct 8, 2015 at 1:16 PM, Josh Boyer <jwbo...@fedoraproject.org> wrote:
> On Tue, Oct 6, 2015 at 5:08 AM, Greg KH <gre...@linuxfoundation.org> wrote:
>> Just responding to one thing at the moment:
>>
>> On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote:
>>>   * we should phase out the usermode helper from firmware_class long term
>>
>> You can "phase out", but you can not delete it as it's a user/kernel api
>> that we have to support for forever, sorry.
>
> Assuming that dell-rbu is the only in-tree legitimate user of the
> userhelper code, I'm curious if the code itself could simply move into
> that driver.  It might help prevent the spread of reliance on an API
> we don't want to see grow in usage.  We'd probably need to evaluate if
> the two new users could migrate off that.

Greg pointed out Daniel might have some uses for this. More on this later.

>> Also, for some devices / use cases, the usermode helper is the solution
>> (think async loading of firmware when the host wants to do it.)
>
> Are any of those use cases in the kernel today, aside from dell-rbu?
> Would Luis' async mode to system data suffice?

We'll have to see based on Daniel's feedback (Daniel, please respond
to the other thread I'll Cc you on).

 Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems loading firmware using built-in drivers with kernels that use initramfs.

2015-12-17 Thread Luis R. Rodriguez 
On Sun, Aug 30, 2015 at 11:11 AM, Linus Torvalds
 wrote:
> On Sun, Aug 30, 2015 at 1:25 AM, Arend van Spriel  wrote:
>> On 08/29/2015 12:38 PM, Ming Lei wrote:
>>
>> Does this mean a built-in driver can not get firmware from initramfs or
>> built in the kernel early. Seems a bit too aggressive.
>
> Yeah, that seems wrong. Loading firmware from initramfs is required
> for some things, like disk drivers. Of course, depending on how it's
> done, it's all after the SYSTEM_BOOTING phase, but ..
>
> What we *might* do is to not allow it for the user-mode helper
> fallback,

FWIW, that's what we did, request_firmware_direct() now skips the
silly usermode helper. I'll note that Greg pointed out to me that
Daniel (was this right?) might have some use cases for the usermode
helper in the future on graphics though. Daniel is that right? Can you
clarify the use case, I'd just like to document this and keep it in
mind for future design changes on firmware_class. Also, it occurs to
me that if you have a need for it, perhaps others might and if we can
avoid *others* from coming up with their own solution that'd be best,
specifically as this is related to file loading -- more on this later
generalized possible use cases in another thread I'll Cc you folks on.

> but I think it's more likely that we'll just deprecate the
> usermode helper fw loader entirely, so adding new error cases for it
> seems pointless.

I was shooting hard to deprecate the usermodehelper, Greg has noted
that we can only phase it out though, so that is what I will be
shooting for: a sysdata API, what will have firmware signing support
later will *not* make use of the usermode helper. The old APIs will
still use it.

[0] http://lkml.kernel.org/r/20151006090821.gb9...@kroah.com

 Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

2015-10-09 Thread Luis R. Rodriguez 
On Fri, Oct 09, 2015 at 08:46:42AM -0400, Josh Boyer wrote:
> On Thu, Oct 8, 2015 at 6:54 PM, Luis R. Rodriguez <mcg...@suse.com> wrote:
> > On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote:
> >> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
> >> <mcg...@do-not-panic.com> wrote:
> >> > From: David Howells <dhowe...@redhat.com>
> >> >
> >> > We'll want to reuse this same code later in order to
> >> > read two separate types of file contents. This generalizes
> >> > fw_read_file() for reading a file rebrands it as fw_read_file().
> >>
> >> Er, maybe that should read "...fw_read_file_contents() for reading a
> >> file and rebrands it as fw_read_file()." ?
> >
> > Thanks, corrected.
> >
> >> > This caller lets us pegs arbitrary data onto the target
> >> > buffer and size if the file is found.
> >>
> >> This sentence is somewhat confusing.  The data isn't arbitrary. It is
> >> what the caller wants you to read from path.  What is arbitrary, at
> >> least in the context of this function, is the path passed to it.
> >> Maybe rewrite this as:
> >>
> >> "The new function allows us to read file contents from arbitrary paths
> >> and return the data and size of the files read."
> >
> > The path is arbitrary but what I meant by arbitrary data is that
> > the data need no longer be firmware, whereas fw_read_file_contents()
> > *did* require passing firmware_class data structures. What this does
> > is it make the possibility of eventually making a more core system
> > data file reader more obvious, so for instance the goal is to later
> > share a reader with:
> >
> > - firmware_class: fw_read_file()
> > - module: kernel_read()
> > - kexec: copy_file_fd()
> >
> > I will clarify this in the commit log and also clarify the path is
> > arbitrary as well as you note.
> >
> >> > While at it this cleans up the exit paths on fw_read_file().
> >> >
> >> > Signed-off-by: David Howells <dhowe...@redhat.com>
> >> > Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
> >>
> >> The code changes themselves look fine.
> >
> > Thank you for the review. Can I peg your Acked-by or Reviewed-by?
> > How about this for a change in the commit log:
> >
> > firmware: generalize reading file contents as a helper
> >
> > We'll want to reuse this same code later in order to read
> > two separate types of file contents. This generalizes
> > fw_read_file_contents() for reading a file and rebrands it
> > as fw_read_file(). This new caller is now generic and that
> > path can be arbitrary, the caller is also agnostic to the
> > firmware_class code now, which begs the possibility of code
> > re-use with other similar callers in the kernel. For instance
> > in the future we may want to share a solution with:
> >
> > - firmware_class: fw_read_file()
> > - module: kernel_read()
> > - kexec: copy_file_fd()
> >
> > While at it this also cleans up the exit paths on fw_read_file().
> 
> That reads much clearer to me.  Thanks.  With that changed:
> 
> Reviewed-by: Josh Boyer <jwbo...@fedoraproject.org>

Thanks, amended.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] firmware: add an extensible system data helpers

2015-10-08 Thread Luis R. Rodriguez 
On Thu, Oct 08, 2015 at 01:59:11PM -0400, Josh Boyer wrote:
> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
> > +static inline int desc_sync_found_call_cb(const struct sysdata_file_desc 
> > *desc,
> > + const struct sysdata_file 
> > *sysdata)
> > +{
> > +   BUG_ON(desc->sync_reqs.mode != SYNCDATA_SYNC);
> 
> ngh...  Why do these inline functions all have BUG_ONs in them?  If it
> is to catch a programming error, why can't you just return EINVAL like
> you do in the async function case?  (Even that WARN_ON seems
> excessive).

Sure, I've replaced the pesky BUG_ON() with returning -EINVAL's.
Let me know if there is anything else.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper

2015-10-08 Thread Luis R. Rodriguez 
On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote:
> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
> <mcg...@do-not-panic.com> wrote:
> > From: David Howells <dhowe...@redhat.com>
> >
> > We'll want to reuse this same code later in order to
> > read two separate types of file contents. This generalizes
> > fw_read_file() for reading a file rebrands it as fw_read_file().
> 
> Er, maybe that should read "...fw_read_file_contents() for reading a
> file and rebrands it as fw_read_file()." ?

Thanks, corrected.

> > This caller lets us pegs arbitrary data onto the target
> > buffer and size if the file is found.
> 
> This sentence is somewhat confusing.  The data isn't arbitrary. It is
> what the caller wants you to read from path.  What is arbitrary, at
> least in the context of this function, is the path passed to it.
> Maybe rewrite this as:
> 
> "The new function allows us to read file contents from arbitrary paths
> and return the data and size of the files read."

The path is arbitrary but what I meant by arbitrary data is that
the data need no longer be firmware, whereas fw_read_file_contents()
*did* require passing firmware_class data structures. What this does
is it make the possibility of eventually making a more core system
data file reader more obvious, so for instance the goal is to later
share a reader with:

- firmware_class: fw_read_file()
- module: kernel_read()
- kexec: copy_file_fd()

I will clarify this in the commit log and also clarify the path is
arbitrary as well as you note.

> > While at it this cleans up the exit paths on fw_read_file().
> >
> > Signed-off-by: David Howells <dhowe...@redhat.com>
> > Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
> 
> The code changes themselves look fine.

Thank you for the review. Can I peg your Acked-by or Reviewed-by?
How about this for a change in the commit log:

firmware: generalize reading file contents as a helper  

We'll want to reuse this same code later in order to read   
two separate types of file contents. This generalizes   
 
fw_read_file_contents() for reading a file and rebrands it  
as fw_read_file(). This new caller is now generic and that  
path can be arbitrary, the caller is also agnostic to the   
firmware_class code now, which begs the possibility of code 
re-use with other similar callers in the kernel. For instance   
in the future we may want to share a solution with: 

- firmware_class: fw_read_file()
- module: kernel_read() 
- kexec: copy_file_fd() 

While at it this also cleans up the exit paths on fw_read_file().   

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] firmware: generalize "firmware" as "system data" helpers]

2015-10-06 Thread Luis R. Rodriguez 
On Tue, Oct 06, 2015 at 10:08:21AM +0100, Greg KH wrote:
> Just responding to one thing at the moment:
> 
> On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote:
> >   * we should phase out the usermode helper from firmware_class long term
> 
> You can "phase out", but you can not delete it as it's a user/kernel api
> that we have to support for forever, sorry.
>
>
> Also, for some devices / use cases, the usermode helper is the solution
> (think async loading of firmware when the host wants to do it.)

Sure, this can still be kept in a dark corner, no need for it to clutter
or get in the way of creating cleaner APIs. That's one of the goals here,
and going through with these changes should help us get there.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] firmware: move completing fw into a helper

2015-10-01 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

This will be re-used later through a new extensible interface.

Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6f5fcda71a60..d8148aa89b01 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,6 +322,15 @@ fail:
return rc;
 }
 
+static void fw_finish_direct_load(struct device *device,
+ struct firmware_buf *buf)
+{
+   mutex_lock(_lock);
+   set_bit(FW_STATUS_DONE, >status);
+   complete_all(>completion);
+   mutex_unlock(_lock);
+}
+
 static int fw_get_filesystem_firmware(struct device *device,
   struct firmware_buf *buf)
 {
@@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device 
*device,
if (!rc) {
dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
-   mutex_lock(_lock);
-   set_bit(FW_STATUS_DONE, >status);
-   complete_all(>completion);
-   mutex_unlock(_lock);
+   fw_finish_direct_load(device, buf);
}
 
return rc;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/5] firmware_class: extensible firmware API

2015-10-01 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

This v2 series pushes together a few simple code shifts me and David
worked on with a new extensible firmware API. I have bundled these changes
together as in my last v1 series [0] it was not clear why the first simple
set of changes were being made, and I had sent out the extensible firmware
API only as an RFC and separately. I've bundled these two together, this should
make it clearer now why we made the first set of changes, the last patch is the
meat of the work to consider and review here. Its important to realize that
this series does not include firmware signing support. The exact architecture
for that is still being discussed -- if we don't all agree on the exact
architecture of firmware signing through mailing list discussions one
possibility is to just discuss it as one of the topics for the upcoming kernel
summit.

Now, regardless of the outcome of the exact architecture and design of firmware
signing -- a few thing are clear though:

1) we keep extending the firmware API as we come up with more needs, when
   we do this it means we have a series of unnecessary collateral evolutions
   on drivers... 

2) the user mode helper code needs to die off and although some distributions
   are now disabling support for it and systemd ripped out support
   for it, Josh Boyer recently noted that drivers other than dell-rbu
   now also select FW_LOADER_USER_HELPER_FALLBACK [1]... we should
   really just compartamentalize such use and phase that out slowly.

The extensible firmware API then is being introduced both as a stepping stone
to expected changes to the firmware API for Linux firmware signing support and
as a way for us to phase out the user mode helper.

My original approach to firmware signing was to support firmware signing
behind the scenes automatically for all drivers when enabled via Kconfig,
with one default key -- similar to how module signing support works. In this
original design it was only a side consideration to support custom keys for
firmware signing, for instance I had envisioned that we could replace CRDA
with a simple firmware request call with our own custom 802.11 key
requirements, which would be orthogonal to what distributions decide for
"general firmware signing". Upon review though those who have expressed
interest in firmware signing have stated interest to be able to specify their
own driver key signing criteria from the start. If we end up requiring key
requirements to *always* be specified we'll essentially be extending the
number of arguments to the firmware API from the start. Instead of having
*all* drivers be modified that use the old firmware API, or adding yet another
new call, an extensible firmware API would enable only changes to be made for
drivers that do want to explicitly opt-in for support for firmware signing,
with an added bonus of also avoiding the user mode helper, and enabling this
new API to be easily extensible in the future without other collateral
evolutions.

[0] 
http://lkml.kernel.org/r/1438725604-22795-1-git-send-email-mcg...@do-not-panic.com
[1] 
http://lkml.kernel.org/r/ca+5pva6togj5ibotgk5casy9vuaqc8sxywkex+ojwc51uev...@mail.gmail.com

David Howells (2):
  firmware: fold successful fw read early
  firmware: generalize reading file contents as a helper

Luis R. Rodriguez (3):
  firmware: generalize "firmware" as "system data" helpers
  firmware: move completing fw into a helper
  firmware: add an extensible system data helpers

 Documentation/firmware_class/system_data.txt |  71 +
 drivers/base/firmware_class.c| 385 ---
 include/linux/sysdata.h  | 208 +++
 3 files changed, 627 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] firmware: generalize "firmware" as "system data" helpers

2015-10-01 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.

Annotate this before we extend firmare_class more,
as this is expected. We want to generalize the code
as much as possible.

Cc: Rusty Russell <ru...@rustcorp.com.au>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: David Howells <dhowe...@redhat.com>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Casey Schaufler <ca...@schaufler-ca.com>
Cc: Ming Lei <ming@canonical.com>
Cc: Takashi Iwai <ti...@suse.de>
Cc: Vojtěch Pavlík <vojt...@suse.cz>
Cc: Kyle McMartin <k...@kernel.org>
Cc: Matthew Garrett <mj...@srcf.ucam.org>
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450e75bd..6f5fcda71a60 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device 
*device,
rc = fw_read_file_contents(file, buf);
fput(file);
if (rc)
-   dev_warn(device, "firmware, attempted to load %s, but 
failed with error %d\n",
-   path, rc);
+   dev_warn(device, "system data, attempted to load %s, 
but failed with error %d\n",
+path, rc);
else
break;
}
__putname(path);
 
if (!rc) {
-   dev_dbg(device, "firmware: direct-loading firmware %s\n",
+   dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
mutex_lock(_lock);
set_bit(FW_STATUS_DONE, >status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, 
const char *name,
}
 
if (fw_get_builtin_firmware(firmware, name)) {
-   dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+   dev_dbg(device, "system data: using built-in system data%s\n", 
name);
return 0; /* assigned */
}
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] firmware: add an extensible system data helpers

2015-10-01 Thread Luis R. Rodriguez 
From: "Luis R. Rodriguez" <mcg...@suse.com>

The firmware API has evolved over the years slowly, as it
grows we extend it by adding new routines or at times we extend
existing routines with more or less arguments. This doesn't scale
well, when new arguments are added to existing routines it means
we need to traverse the kernel with a slew of collateral
evolutions to adjust old driver users. The firmware API is also
now being used for things outside of the scope of what typically
would be considered "firmware", an example here is the p54 driver
enables users to provide a custom EEPROM through this interface.
Another example is optional CPU microcode updates.

There are other subsystems which would like to make use of the
APIs for similar things but have different requirements and
criteria which they'd like to be met for the requested file.
If different requirements are needed it would again mean adding
more arguments and making a slew of collateral evolutions, or
adding yet-another-new-API-call.

Instead of extending the existing firmware API even more this
provides an new extensible API which can be used to supercede the
old firmware API without causing a series of collateral evolutions
as requirements grow. This leaves the old firmware API as-is,
ignores all consideration for usermode-helpers, labels the new API
to reflect its broad use outside of the scope of firmware: system
data helpers, and builds on top of the original firmware core code.

The new extensible "system data" set of helpers accepts that there
really are only two types of requests for accessing system data:

a) synchronous requests
b) asynchronous requests

Both of these requests may have a different set of requirements
which must be met. These requirements can simply be passed as a
descriptors to each type of request. The descriptor can be extended
over time to support different requirements as the kernel evolves.

Using the new system data helpers is only necessary if you have
requirements outside of what the existing old firmware API accepts.
We encourage developers to leave the old API as-is and extend the
new descriptors and system data code to provide support for new
features.

A few simple features added as part of the new set of system data
request APIs, other than making the new API easily extensible for
the future:

 - By default the kernel will free the system data file for you after
   your callbacks are called, you however are allowed to request to that
   you wish to keep the system data file on the descriptor. This is
   dealt with by requiring a consumer callback for the system data file.
 - Allow both asynchronous and synchronous request to specify that system data
   files are optional. With the old APIs we had added one full API call,
   request_firmware_direct() just for this purpose -- although it should be
   noted another of its goal was to also skip the usermode helper.
   The system data request APIs allow for you to annotate that a system
   data file is optional for both synchronous or asynchronous requests
   through the same two basic set of APIs.
 - Usermode helpers are completely ignored, always
 - The system data request APIs currently match the old synchronous firmware
   API calls to refcounted firmware_class module, but it should be easy
   to add support now to enable also refcounting the caller's module
   should it be be needed. Likewise the system data request APIs match the
   old asynchronous firmware API call and refcounts the caller's module.

In order to try to help phase out user mode helpers this makes no use of
the old user mode helper code *at all*, and if we wish to can easily
phase this code out with time then.

Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 Documentation/firmware_class/system_data.txt |  71 +++
 drivers/base/firmware_class.c| 291 +++
 include/linux/sysdata.h  | 208 +++
 3 files changed, 570 insertions(+)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

diff --git a/Documentation/firmware_class/system_data.txt 
b/Documentation/firmware_class/system_data.txt
new file mode 100644
index ..1fe0ecd29324
--- /dev/null
+++ b/Documentation/firmware_class/system_data.txt
@@ -0,0 +1,71 @@
+System data requests API
+
+
+As the kernel evolves we keep extending the firmware_class set of APIs
+with more or less arguments, this creates a slew of collateral evolutions.
+The set of users of firmware request APIs has also grown now to include
+users which are not looking for "firmware" per se, but instead general
+system data files which for one reason or another has been decided to be
+kept oustide of the kernel, and/or to allow dynamic updates. The system data
+request set of APIs addresses rebranding of firmware as generic system data
+files, and provi

[PATCH v2 4/5] firmware: generalize reading file contents as a helper

2015-10-01 Thread Luis R. Rodriguez 
From: David Howells <dhowe...@redhat.com>

We'll want to reuse this same code later in order to
read two separate types of file contents. This generalizes
fw_read_file() for reading a file rebrands it as fw_read_file().
This caller lets us pegs arbitrary data onto the target
buffer and size if the file is found.

While at it this cleans up the exit paths on fw_read_file().

Signed-off-by: David Howells <dhowe...@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcg...@suse.com>
---
 drivers/base/firmware_class.c | 62 +++
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e10a5349aa61..cd51a90ed012 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -291,34 +291,51 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher 
priority than default path");
 
-static int fw_read_file_contents(struct file *file, struct firmware_buf 
*fw_buf)
+/*
+ * Read the contents of a file.
+ */
+static int fw_read_file(const char *path, void **_buf, size_t *_size)
 {
-   int size;
+   struct file *file;
+   size_t size;
char *buf;
int rc;
 
+   file = filp_open(path, O_RDONLY, 0);
+   if (IS_ERR(file))
+   return PTR_ERR(file);
+
+   rc = -EINVAL;
if (!S_ISREG(file_inode(file)->i_mode))
-   return -EINVAL;
+   goto err_file;
size = i_size_read(file_inode(file));
if (size <= 0)
-   return -EINVAL;
+   goto err_file;
+   rc = -ENOMEM;
buf = vmalloc(size);
if (!buf)
-   return -ENOMEM;
+   goto err_file;
+
rc = kernel_read(file, 0, buf, size);
+   if (rc < 0)
+   goto err_buf;
if (rc != size) {
-   if (rc > 0)
-   rc = -EIO;
-   goto fail;
+   rc = -EIO;
+   goto err_buf;
}
+
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
-   goto fail;
-   fw_buf->data = buf;
-   fw_buf->size = size;
+   goto err_buf;
+
+   *_buf = buf;
+   *_size = size;
return 0;
-fail:
+
+err_buf:
vfree(buf);
+err_file:
+   fput(file);
return rc;
 }
 
@@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
 }
 
 static int fw_get_filesystem_firmware(struct device *device,
-  struct firmware_buf *buf)
+ struct firmware_buf *buf)
 {
int i, len;
int rc = -ENOENT;
-   char *path;
+   char *path = NULL;
 
path = __getname();
if (!path)
return -ENOMEM;
 
+   /*
+* Try each possible firmware blob in turn till one doesn't produce
+* ENOENT.
+*/
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-   struct file *file;
-
/* skip the unset customized path */
if (!fw_path[i][0])
continue;
@@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device 
*device,
break;
}
 
-   file = filp_open(path, O_RDONLY, 0);
-   if (IS_ERR(file))
-   continue;
-   rc = fw_read_file_contents(file, buf);
-   fput(file);
+   rc = fw_read_file(path, >data, >size);
if (rc == 0) {
dev_dbg(device, "system data: direct-loading firmware 
%s\n",
buf->fw_id);
fw_finish_direct_load(device, buf);
goto out;
-   } else
+   } else if (rc != -ENOENT) {
dev_warn(device, "system data, attempted to load %s, 
but failed with error %d\n",
 path, rc);
+   goto out;
+   }
}
 out:
__putname(path);
-
return rc;
 }
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux Firmware Signing

2015-09-30 Thread Luis R. Rodriguez 
On Thu, Sep 03, 2015 at 02:14:18PM -0700, Kees Cook wrote:
> [removed bounced email addresses]
> 
> On Wed, Sep 2, 2015 at 2:37 PM, Luis R. Rodriguez <mcg...@suse.com> wrote:
> > On Wed, Sep 02, 2015 at 01:54:43PM -0700, Kees Cook wrote:
> >> On Wed, Sep 2, 2015 at 11:46 AM, Luis R. Rodriguez <mcg...@suse.com> wrote:
> >> > On Tue, Sep 01, 2015 at 11:35:05PM -0400, Mimi Zohar wrote:
> >> >> > OK great, I think that instead of passing the actual routine name we 
> >> >> > should
> >> >> > instead pass an enum type for to the LSM, that'd be easier to parse 
> >> >> > and we'd
> >> >> > then have each case well documented. Each LSM then could add its own
> >> >> > documetnation for this and can switch on it. If we went with a name 
> >> >> > we'd have
> >> >> > to to use something like __func__ and then parse that, its not clear 
> >> >> > if we need
> >> >> > to get that specific.
> >> >>
> >> >> Agreed.  IMA already defines an enumeration.
> >> >>
> >> >> /* IMA policy related functions */
> >> >> enum ima_hooks { FILE_CHECK = 1, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK,
> >> >>  FIRMWARE_CHECK, POLICY_CHECK, POST_SETATTR };
> >> >>
> >> >
> >> > We want something that is not only useful for IMA but any other LSM,
> >> > and FILE_CHECK seems very broad, not sure what BPRM_CHECK is even upon
> >> > inspecting kernel code. Likewise for POST_SETATTR. POLICY_CHECK might
> >> > be broad, perhaps its best we define then a generic set of enums to
> >> > which IMA can map them to then and let it decide. This would ensure
> >> > that the kernel defines each use caes for file inspection carefully,
> >> > documents and defines them and if an LSM wants to bunch a set together
> >> > it can do so easily with a switch statement to map set of generic
> >> > file checks in kernel to a group it already handles.
> >> >
> >> > For instance at least in the short term we'd try to unify:
> >> >
> >> > security_kernel_fw_from_file()
> >> > security_kernel_module_from_file()
> >> >
> >> > to perhaps:
> >> >
> >> > security_kernel_from_file()
> >> >
> >> > As far, as far as I can tell, the only ones we'd be ready to start
> >> > grouping immediately or with small amount of work rather soon:
> >> >
> >> > /**
> >> >  *
> >> >  * enum security_filecheck - known kernel security file checks types
> >> >  *
> >> >  * @__SECURITY_FILECHECK_UNSPEC: attribute 0 reserved
> >> >  * @SECURITY_FILECHECK_MODULE: the file being processed is a Linux 
> >> > kernel module
> >> >  * @SECURITY_FILECHECK_SYSDATA: the file being processed is either a 
> >> > firmware
> >> >  *  file or a system data file read from /lib/firmware/* by 
> >> > firmware_class
> >>
> >> I'd prefer a distinct category for firmware, as it carries an
> >> implication that it is an executable blob of some sort (I know not all
> >> are, though).
> >
> > The ship has sailed in terms of folks using frimrware API for things
> > that are not-firmware per se. The first one I am aware of was the
> > EEPROM override for the p54 driver. The other similar one was CPU
> > microcode, but that's a bit more close to home with "firmware". We
> > could ask users on the new system data request API I am building
> > to describe the type of file being used, as I agree differentiating
> > this for security purposes might be important. So other than just
> > file type we could have sub type category, then we could have,
> >
> > SECURITY_FILECHECK_SYSDATA, and then:
> 
> I object to executable code being called data. :)
> 
> > SECURITY_FILE_SYSDATA_FW
> > SECURITY_FILE_SYSDATA_MICROCODE
> > SECURITY_FILE_SYSDATA_EEPROM
> > SECURITY_FILE_SYSDATA_POLICY (for 802.11 regulatory I suppose)
> 
> The exception to the firmware loading is data, so the primary name
> should be firmware. Regardless, if we want distinct objects, just name
> them:
> 
> SECURITY_FILE_FIRMWARE
> SECURITY_FILE_SYSDATA
> 
> Do we need finer-grain sub types?

These two work for me.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html