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@xxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxx
[1] http://lkml.kernel.org/r/1440719673.2118.84.camel@xxxxxxxxxxxxxxxxxx
[2] http://lkml.kernel.org/r/20150930203400.GC14464@xxxxxxxxxxxxx

  * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/