From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752130AbbJEVW3 (ORCPT ); Mon, 5 Oct 2015 17:22:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:58934 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751639AbbJEVW1 (ORCPT ); Mon, 5 Oct 2015 17:22:27 -0400 Date: Mon, 5 Oct 2015 23:22:22 +0200 From: "Luis R. Rodriguez" To: Greg KH Cc: "Luis R. Rodriguez" , ming.lei@canonical.com, corbet@lwn.net, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, dwmw2@infradead.org, dhowells@redhat.com, seth.forshee@canonical.com, rusty@rustcorp.com.au, mmarek@suse.cz, mjg59@srcf.ucam.org, kyle@kernel.org, linux-security-module@vger.kernel.org, keyrings@linux-nfs.org, Andrew Morton , Kees Cook , Casey Schaufler , Takashi Iwai , =?utf-8?Q?Vojt=C4=9Bch_Pavl=C3=ADk?= , mcgrof@suse.com Subject: Re: [PATCH 1/4] firmware: generalize "firmware" as "system data" helpers Message-ID: <20151005212222.GO14464@wotan.suse.de> References: <1438725604-22795-1-git-send-email-mcgrof@do-not-panic.com> <1438725604-22795-2-git-send-email-mcgrof@do-not-panic.com> <20151004191602.GC9649@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151004191602.GC9649@kroah.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" > > > > 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