[PATCH 0/4] firmware_class: few small code shifts

[PATCH 0/4] firmware_class: few small code shifts

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Ming, Greg,

this patch set consists of a few small code shifts which would make
it easier to add extensible firmware API code, and later firmware
signing support. This patch set is being sent out separately as it
does not contain any controversial changes. It should also help
with readibility of the code.

I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
next patch sets would start slowly diving into the topic of firmware signing
and extending documentation, and those patches will depend on this set.

There is a superfluous else branch on patch #3, its not needed because of
the goto statement but we leave that in place to make patch #4 easier to
read.

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

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

 drivers/base/firmware_class.c | 94 ++++++++++++++++++++++++++-----------------
 1 file changed, 57 insertions(+), 37 deletions(-)

-- 
2.3.2.209.gd67f9d5.dirty

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

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@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 <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vojtěch Pavlík <vojtech@suse.cz>
Cc: Kyle McMartin <kyle@kernel.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@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 894bda114224..f97e76cca069 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(&fw_lock);
 		set_bit(FW_STATUS_DONE, &buf->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.3.2.209.gd67f9d5.dirty

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

[Greg KH]

On Tue, Aug 04, 2015 at 03:00:01PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@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.

No, let's leave this as "firmware", as that is what the code does.

If you want to create a "load a resource from the filesystem into the
kernel" subsystem, then let's do that and then port the firmware loader
code over to use that api.

But until then, let's not try to morph the firmware code into something
that it really is not at all at the moment, just because it looks like
this might be a nice thing to do someday in the future.

sorry,

greg k-h

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

[Luis R. Rodriguez]

On Sun, Oct 04, 2015 at 08:16:02PM +0100, Greg KH wrote:
> On Tue, Aug 04, 2015 at 03:00:01PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@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.
> 
> No, let's leave this as "firmware", as that is what the code does.

But its not, p54 uses it for EEPROM overriding, and CPU arch code uses it for
microcode. Then, consider replacing CRDA in the long run, that will not load
firmware at all so we need a solution that covers enabling to replace CRDA but
also acknowledges existing non-firmware uses of firmware_class. We could now
try to set a fine line between wanting to differentiate what "firmware" might
be but that poses some implications of prospects of re-using code for 802.11
for regulatory.bin and other existing non-firmware users, and redistribution /
packaging. We had discussed this a while ago when I was devising the code for
firmware signing, both on IRC and mailing lists, and folks seem to be OK with
the path forward to re-use linux-firmware redistribution mechanisms and paths
for regulatory.bin. More details on all this below.

> If you want to create a "load a resource from the filesystem into the
> kernel" subsystem, then let's do that and then port the firmware loader
> code over to use that api.

That's indeed where this is heading though, but some notes on that front:

I considered doing that from the start but the firmware API happens to be the
only user which will want and use such *robust* API right now. For instance I
consdired a generic sysdata API that would enable users like firmware_class to
then let a subsystem / module declare the prefix paths it would use to lookup
files from. Then it'd use the sysdata helpers. Adding a full framework alone
just for firmware_class seemed overkill right now specially since
firmware_class has requirements right now such as the usermode helper and
user mode locking stuff. I don't really think its a good idea to add a generic
API to enable any of that into anything shared. The only other possible user
short term was for 802.11 to replace CRDA but having the 802.11 subsystem
define something as firmware_class to load just one file seemed like too much
bloat for a simple filesystem loader, specially when it was discussed and
agreed that for 802.11 we'd be happy to just have regulatory.bin be part of and
just ship in linux-firmware repository, rather than devising another special
path for it.

More on all of this below...

> But until then, let's not try to morph the firmware code into something
> that it really is not at all at the moment, just because it looks like
> this might be a nice thing to do someday in the future.

As you note a generic filesystem loader is what this series highlights we
should strive for, I considered it, and here are a few other things to
keep in mind which lead me to implement things as-is in this patch series:

  * we should phase out the usermode helper from firmware_class long term

  * as-is right now only firmware_class would make use of a broad generic
    filesystem loader, and upon discussions with folks in terms of the goal of
    replacing CRDA , we're happy to just let regulatory.bin live within
    linux-firmware repository and re-use its existing distribution mechanisms.
    This is precicely why I went forward with a rebranding side effort here.
    If we want to make use of a separate path for non-firmware things such as
    regulatory.bin and perhaps EEPROM override, CPU microcode, etc, then we should
    have a *much broader* discussion and agreement.

    When I poked folks about this it, it was expressed we didn't want a separate path
    for things like this. We were happy to welcome regulatory.bin within linux-firmware
    and see it beneficial to share the same redistribution path / code for users.
    To be clear folks expressed being fine and it being desirable with sharing the
    /lib/firmware/ path generic "system data files" the kernel might need. p54
    EEPROM override is already one use case, as well as CPU microcode, the next
    obvious non-firmware use case here was regulatory.bin but that first needs
    crypto support, which is why my work postpones that until much later. The
    one thing we *can* do to help here to annotate "system data" is different
    from "firmware" is to have callers annotate this from a security perspective
    but more on this below.

  * we clearly do stand to gain from a small *core* filesystem loader but the code
    I identified as generic is rather *simple and very small*. It turns out
    that its implementation as generic is also orthogonal to the goal of
    re-using the firmware API for general system data files because we decided
    it was OK for us to re-use /lib/firmware for regulatory.bin, for instance.
    The code we *can* generalize consists of a file system loder to share
    between these 3 existing callers:
    
    - firmware_class: fw_read_file()
    - module: kernel_read()
    - kexec: copy_file_fd()
 
    At the last Linux security summit we discussed this as well, likewise
    recently on lkml  as well [0] [1]. We stand to also gain here to
    just define *one* LSM for a general "load from file from filesystem".
    The discussion has lead to agreement we can just then throw the LSM
    the context of type of file being read, we can do this through an enum
    but with just one LSM hook for all. From a security perspective Kees
    Cook has asked that we at least distinguish firmware from "system data"
    as firmware carries an implication that the code being passed to the kernel
    is an executable of some sort, whereas system data is not. We'd also
    distinguish an enum type for kexec-kernel, kexec-initramfs, module, for
    more on this please refer to the ongoing discussion [2].

[0] http://lkml.kernel.org/r/CAGXu5jKF6CBAADoybZHRCzYAepcZYqpbck1gTAeV1p7iuOVAvw@mail.gmail.com
[1] http://lkml.kernel.org/r/1440719673.2118.84.camel@linux.vnet.ibm.com
[2] http://lkml.kernel.org/r/20150930203400.GC14464@wotan.suse.de

  * we are expecting to need an extensible API for the firmware API for
    firmware signing support. Signing support is desirable for both
    firmware and "system data" so it makes sense to re-use the existing
    extensible API for both.

Bundling up the LSMs and code for the 3 different code paths I identified above
then is something archticturally different than looking to re-use /lib/firmware
path for "system data". Sharing code here is indeed desirable but the code
changes for that work ends up to be orthogonal to re-using the firmware API for
general system data files, unless of course we decide on separting the distribution
paths for "firmware" and "system data" and decide on also needing two different
code paths for both them. I don't think that's needed.

There are two levels of code generalization prospects here then:

  1) system data loader / firmware loader
  2) general core filesystem loader with shared LSM hook

2) is something I intend on addressing more long term, I've been working on
this but it is slightly orthogonal to the item 1). It does not need to be done
in order to move forward with item 1). I am working towards both of these
though, in parallel.

I've identified that we could *perhaps* generalize the firmware loader code
even more but IMHO in order to do that in any sensible way we first should
phase out use of the user mode helper by enabling use of that path only to the
few drivers that really need it. Once that is done we can split out the
usermode helper code out from the firmware stuff, the firmware code could at that
point could be shoved under kernel/sysdata.c for instance as a more generic
filesystem loader. Without phasing the user mode helper out first there is
quite a bit of complexities that tie us into the usermode helper that don't
make it easy to do.

Long term then we'd have "2) a generic core filesystem loader" being used by
modules, kexec, and the sysdata API, and we'd have "1) system data loader" 
which enables kernel components to load system data as either firmware or system
data from /lib/firmware/, which one it loads can be specified by an enum by
the caller.

Please let me know if you have a preferred alternative strategy you recommend
to follow, but please take into consideration all of the above.

  Luis

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

[Greg KH]

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

thanks,

greg k-h

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

[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

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

[Josh Boyer]

On Tue, Oct 6, 2015 at 5:08 AM, Greg KH <gregkh@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.

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

josh

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

[Luis R. Rodriguez]

On Thu, Oct 8, 2015 at 1:16 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> On Tue, Oct 6, 2015 at 5:08 AM, Greg KH <gregkh@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

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

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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

Signed-off-by: Luis R. Rodriguez <mcgrof@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 f97e76cca069..9ee334c1b872 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(&fw_lock);
+	set_bit(FW_STATUS_DONE, &buf->status);
+	complete_all(&buf->completion);
+	mutex_unlock(&fw_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(&fw_lock);
-		set_bit(FW_STATUS_DONE, &buf->status);
-		complete_all(&buf->completion);
-		mutex_unlock(&fw_lock);
+		fw_finish_direct_load(device, buf);
 	}
 
 	return rc;
-- 
2.3.2.209.gd67f9d5.dirty

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

[Luis R. Rodriguez]

From: David Howells <dhowells@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 <dhowells@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@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 9ee334c1b872..736fb952b75b 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.3.2.209.gd67f9d5.dirty

Re: [PATCH 3/4] firmware: fold successful fw read early

[Ming Lei]

On Tue, Aug 4, 2015 at 6:00 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: David Howells <dhowells@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 <dhowells@redhat.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@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 9ee334c1b872..736fb952b75b 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;

'break' should be enough, and the following 'out' label can be saved too.

> +               } 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.3.2.209.gd67f9d5.dirty
>

Re: [PATCH 3/4] firmware: fold successful fw read early

[Luis R. Rodriguez]

On Sun, Aug 9, 2015 at 6:29 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, Aug 4, 2015 at 6:00 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> From: David Howells <dhowells@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 <dhowells@redhat.com>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@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 9ee334c1b872..736fb952b75b 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;
>
> 'break' should be enough, and the following 'out' label can be saved too.

This is true but I left it as it makes the next patch easier to read and follow.

>> +               } else
>>                         dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
>>                                  path, rc);
>> -               else
>> -                       break;
>>         }
>> +out:
>>         __putname(path);

  Luis

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

[Luis R. Rodriguez]

From: David Howells <dhowells@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 <dhowells@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@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 736fb952b75b..dcc7036b4ad2 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, &buf->data, &buf->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.3.2.209.gd67f9d5.dirty

Re: [PATCH 0/4] firmware_class: few small code shifts

[Luis R. Rodriguez]

On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> Ming, Greg,
> 
> this patch set consists of a few small code shifts which would make
> it easier to add extensible firmware API code, and later firmware
> signing support. This patch set is being sent out separately as it
> does not contain any controversial changes. It should also help
> with readibility of the code.
> 
> I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> next patch sets would start slowly diving into the topic of firmware signing
> and extending documentation, and those patches will depend on this set.
> 
> There is a superfluous else branch on patch #3, its not needed because of
> the goto statement but we leave that in place to make patch #4 easier to
> read.
> 
> David Howells (2):
>   firmware: fold successful fw read early
>   firmware: generalize reading file contents as a helper
> 
> Luis R. Rodriguez (2):
>   firmware: generalize "firmware" as "system data" helpers
>   firmware: move completing fw into a helper

Ming, Greg,

Please let me know if there are any issues with this series.

  Luis

Re: [PATCH 0/4] firmware_class: few small code shifts

[Greg KH]

On Fri, Aug 21, 2015 at 11:23:03PM +0200, Luis R. Rodriguez wrote:
> On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > Ming, Greg,
> > 
> > this patch set consists of a few small code shifts which would make
> > it easier to add extensible firmware API code, and later firmware
> > signing support. This patch set is being sent out separately as it
> > does not contain any controversial changes. It should also help
> > with readibility of the code.
> > 
> > I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> > next patch sets would start slowly diving into the topic of firmware signing
> > and extending documentation, and those patches will depend on this set.
> > 
> > There is a superfluous else branch on patch #3, its not needed because of
> > the goto statement but we leave that in place to make patch #4 easier to
> > read.
> > 
> > David Howells (2):
> >   firmware: fold successful fw read early
> >   firmware: generalize reading file contents as a helper
> > 
> > Luis R. Rodriguez (2):
> >   firmware: generalize "firmware" as "system data" helpers
> >   firmware: move completing fw into a helper
> 
> Ming, Greg,
> 
> Please let me know if there are any issues with this series.

It's too late for 4.3 at the moment, and my first impulse is you are
just painting the bikeshed by changing some printk names, so I don't
like that, but I'll review it in full after 4.3-rc1 is out, can't do
anything until then.

thanks,

greg k-h

Re: [PATCH 0/4] firmware_class: few small code shifts

[Luis R. Rodriguez]

On Fri, Aug 21, 2015 at 10:38:24PM -0700, Greg KH wrote:
> On Fri, Aug 21, 2015 at 11:23:03PM +0200, Luis R. Rodriguez wrote:
> > On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > 
> > > Ming, Greg,
> > > 
> > > this patch set consists of a few small code shifts which would make
> > > it easier to add extensible firmware API code, and later firmware
> > > signing support. This patch set is being sent out separately as it
> > > does not contain any controversial changes. It should also help
> > > with readibility of the code.
> > > 
> > > I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> > > next patch sets would start slowly diving into the topic of firmware signing
> > > and extending documentation, and those patches will depend on this set.
> > > 
> > > There is a superfluous else branch on patch #3, its not needed because of
> > > the goto statement but we leave that in place to make patch #4 easier to
> > > read.
> > > 
> > > David Howells (2):
> > >   firmware: fold successful fw read early
> > >   firmware: generalize reading file contents as a helper
> > > 
> > > Luis R. Rodriguez (2):
> > >   firmware: generalize "firmware" as "system data" helpers
> > >   firmware: move completing fw into a helper
> > 
> > Ming, Greg,
> > 
> > Please let me know if there are any issues with this series.
> 
> It's too late for 4.3 at the moment, and my first impulse is you are
> just painting the bikeshed by changing some printk names, so I don't
> like that,

This series doesn't address the actual sysdata helper changes which make the
firmware API easly extensible, it just paves the path for it, so because of
that what you describe is correct but only in lack of sight of the other RFC
patch I posted [0].

[0] http://1438730036-22913-1-git-send-email-mcgrof@do-not-panic.com

> but I'll review it in full after 4.3-rc1 is out, can't do
> anything until then.

Sure.

 Luis