Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180508181247.19431-6-mcgrof@kernel.org>
References: <20180508181247.19431-1-mcgrof@kernel.org> <20180508181247.19431-6-mcgrof@kernel.org>
From:   Kees Cook <keescook@chromium.org>
Date:   Tue, 8 May 2018 15:42:33 -0700
Message-ID: <CAGXu5jJXMxx2Xna_M_g1wsS59XnYKqHAGdxhzDtk3PhnLL-gQQ@mail.gmail.com>
Subject: Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation
 over FW_LOADER
To:     "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Josh Triplett <josh@joshtriplett.org>, maco@android.com,
        Andy Gross <andy.gross@linaro.org>,
        David Brown <david.brown@linaro.org>,
        bjorn.andersson@linaro.org, teg@jklm.no, wagi@monom.org,
        Hans de Goede <hdegoede@redhat.com>,
        Andres Rodriguez <andresx7@gmail.com>,
        Mimi Zohar <zohar@linux.vnet.ibm.com>,
        Jakub Kicinski <kubakici@wp.pl>, Shuah Khan <shuah@kernel.org>,
        Martin Fuzzey <mfuzzey@parkeon.com>,
        David Howells <dhowells@redhat.com>, pali.rohar@gmail.com,
        Takashi Iwai <tiwai@suse.de>,
        Kalle Valo <kvalo@codeaurora.org>,
        Arend van Spriel <arend.vanspriel@broadcom.com>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Nicolas Broeking <nbroeking@me.com>,
        Vikram Mulukutla <markivx@codeaurora.org>,
        Mark Brown <broonie@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        David Woodhouse <dwmw2@infradead.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Abhay_Salunke@dell.com, jewalt@lgsinnovations.com,
        oneukum@suse.com, Bitterblue of Monsea <cantabile.desu@gmail.com>,
        Alexei Starovoitov <ast@fb.com>, hare <hare@suse.com>,
        "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>, khc@pm.waw.pl,
        "David S. Miller" <davem@davemloft.net>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        tkjos@android.com, Jonathan Corbet <corbet@lwn.net>,
        mchehab+samsung@kernel.org, LKML <linux-kernel@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        linux-scsi@vger.kernel.org,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> If you try to read FW_LOADER today it speaks of old riddles and
> unless you have been following development closely you will loose

Typo: lose

> track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
> is a bit fuzzy and how it fits into this big picture.
>
> Give the FW_LOADER kconfig documentation some love with more up to
> date developments and recommendations. While at it, wrap the FW_LOADER
> code into its own menu to compartamentalize and make it clearer which

Typo: compartmentalize

> components really are part of the FW_LOADER. This should also make
> it easier to later move these kconfig entries into the firmware_loader/
> directory later.
>
> This also now recommends using firmwared [0] for folks left needing a uevent
> handler in userspace for the sysfs firmware fallback mechanis given udev's
> uevent firmware mechanism was ripped out a while ago.
>
> [0] https://github.com/teg/firmwared
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  drivers/base/Kconfig | 165 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 131 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 29b0eb452b3a..a4fe86caecca 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -70,39 +70,64 @@ config STANDALONE
>           If unsure, say Y.
>
>  config PREVENT_FIRMWARE_BUILD
> -       bool "Prevent firmware from being built"
> +       bool "Disable drivers features which enable custom firmware building"
>         default y
>         help
> -         Say yes to avoid building firmware. Firmware is usually shipped
> -         with the driver and only when updating the firmware should a
> -         rebuild be made.
> -         If unsure, say Y here.
> +         Say yes to disable driver features which enable building a custom
> +         driver firmwar at kernel build time. These drivers do not use the

Typo: firmware

> +         kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
> +         use their own custom loading mechanism. The required firmware is
> +         usually shipped with the driver, building the driver firmware
> +         should only be needed if you have an updated firmware source.
> +
> +         Firmware should not be being built as part of kernel, these days
> +         you should always prevent this and say Y here. There are only two
> +         old drivers which enable building of its firmware at kernel build
> +         time:
> +
> +           o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
> +           o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
> +
> +menu "Firmware loader"
>
>  config FW_LOADER
> -       tristate "Userspace firmware loading support" if EXPERT
> +       tristate "Firmware loading facility" if EXPERT
>         default y
>         ---help---
> -         This option is provided for the case where none of the in-tree modules
> -         require userspace firmware loading support, but a module built
> -         out-of-tree does.
> +         This enables the firmware loading facility in the kernel. The kernel
> +         will first look for built-in firmware, if it has any. Next, it will
> +         look for the requested firmware in a series of filesystem paths:
> +
> +               o firmware_class path module parameter or kernel boot param
> +               o /lib/firmware/updates/UTS_RELEASE
> +               o /lib/firmware/updates
> +               o /lib/firmware/UTS_RELEASE
> +               o /lib/firmware
> +
> +         Enabling this feature only increases your kernel image by about
> +         828 bytes, enable this option unless you are certain you don't
> +         need firmware.
> +
> +         You typically want this built-in (=y) but you can also enable this
> +         as a module, in which case the firmware_class module will be built.
> +         You also want to be sure to enable this built-in if you are going to
> +         enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
> +
> +if FW_LOADER
>
>  config EXTRA_FIRMWARE
> -       string "External firmware blobs to build into the kernel binary"
> -       depends on FW_LOADER
> +       string "Build these firmware blobs into the kernel binary"

Maybe "Build named firmware blobs ..." "these" took me a while to figure out.

>         help
> -         Various drivers in the kernel source tree may require firmware,
> -         which is generally available in your distribution's linux-firmware
> -         package.
> +         Device drivers which require firmware can typically deal with
> +         having the kernel load firmware from the various supported
> +         /lib/firmware/ paths. This option enables you to build into the
> +         kernel firmware files. Built-in firmware searches are preceeded

Typo: preceded

> +         over firmware lookups using your filesystem over the supported
> +         /lib/firmware paths documented on CONFIG_FW_LOADER.
>
> -         The linux-firmware package should install firmware into
> -         /lib/firmware/ on your system, so they can be loaded by userspace
> -         helpers on request.
> -
> -         This option allows firmware to be built into the kernel for the case
> -         where the user either cannot or doesn't want to provide it from
> -         userspace at runtime (for example, when the firmware in question is
> -         required for accessing the boot device, and the user doesn't want to
> -         use an initrd).
> +         This may be useful for testing or if the firmware is required early on
> +         in boot and cannot rely on the firmware being placed in an initrd or
> +         initramfs.
>
>           This option is a string and takes the (space-separated) names of the
>           firmware files -- the same names that appear in MODULE_FIRMWARE()
> @@ -113,7 +138,7 @@ config EXTRA_FIRMWARE
>           For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
>           the usb8388.bin file into /lib/firmware, and build the kernel. Then
>           any request_firmware("usb8388.bin") will be satisfied internally
> -         without needing to call out to userspace.
> +         inside the kernel without ever looking at your filesystem at runtime.
>
>           WARNING: If you include additional firmware files into your binary
>           kernel image that are not available under the terms of the GPL,
> @@ -130,22 +155,94 @@ config EXTRA_FIRMWARE_DIR
>           looks for the firmware files listed in the EXTRA_FIRMWARE option.
>
>  config FW_LOADER_USER_HELPER
> -       bool
> +       bool "Enable the firmware sysfs fallback mechanism"
> +       help
> +         This option enables a sysfs loading facility to enable firmware
> +         loading to the kernel through userspace as a fallback mechanism
> +         if and only if the kernel's direct filesystem lookup for the
> +         firmware failed using the different /lib/firmware/ paths, or the
> +         path specified in the firmware_class path module parameter, or the
> +         firmware_class path kernel boot parameter if the firmware_class is
> +         built-in. For details on how to work with the sysfs fallback mechanism
> +         refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
> +
> +         The direct filesystem lookup for firwmare is always used first now.

Typo: firmware

> +
> +         If the kernel's direct filesystem lookup for firware fails to find
> +         the requested firmware a sysfs fallback loading facility is made
> +         available and userspace is informed about this through uevents.
> +         The uevent can be supressed if the driver explicitly requested it,
> +         this is known as the driver using the custom fallback mechanism.
> +         If the custom fallback mechanism is used userspace must always
> +         acknowledge failure to find firmware as the timeout for the fallback
> +         mechanism is disabled, and failed requests will linger forever.
> +
> +         This used to be the default firmware loading facility, and udev used
> +         to listen for uvents to load firmware for the kernel. The firmware
> +         loading facility functionality in udev has been removed, as such it
> +         can no longer be relied upon as a fallback mechanism. Linux no longer
> +         relies on or uses a fallback mechanism in userspace. If you need to
> +         rely on one refer to the permissively licensed firmwared:

Typo: firmware

> +
> +         https://github.com/teg/firmwared
> +
> +         Since this was the default firmware loading facility at one point,
> +         old userspace may exist which relies upon it, and as such this
> +         mechanism can never be removed from the kernel.
> +
> +         You should only enable this functionality if you are certain you
> +         require a fallback mechanism and have a userspace mechanism ready to
> +         load firmware in case it is not found. One main reason for this may
> +         be if you have drivers which require firmware built-in and for
> +         whatever reason cannot place the required firmware in initramfs.
> +         Another reason kernels may have this feature enabled is to support a
> +         driver which explicitly relies on this fallback mechanism. Only two
> +         drivers need this today:
> +
> +           o CONFIG_LEDS_LP55XX_COMMON
> +           o CONFIG_DELL_RBU
> +
> +         Outside of supporting the above drivers, another reason for needing
> +         this may be that your firmware resides outside of the paths the kernel
> +         looks for and cannot possibily be specified using the firmware_class
> +         path module parameter or kernel firmware_class path boot parameter
> +         if firmware_class is built-in.
> +
> +         A modern use case may be to temporarily mount a custom partition
> +         during provisioning which is only accessible to userspace, and then
> +         to use it to look for and fetch the required firmware. Such type of
> +         driver functionality may not even ever be desirable upstream by
> +         vendors, and as such is only required to be supported as an interface
> +         for provisioning. Since udev's firmware loading facility has been
> +         removed you can use firmwared or a fork of it to customize how you
> +         want to load firmware based on uevents issued.
> +
> +         Enabling this option will increase your kernel image size by about
> +         13436 bytes.
> +
> +         If you are unsure about this, say N here, unless you are Linux
> +         distribution and need to support the above two drivers, or you are
> +         certain you need to support some really custom firmware loading
> +         facility in userspace.
>
>  config FW_LOADER_USER_HELPER_FALLBACK
> -       bool "Fallback user-helper invocation for firmware loading"
> -       depends on FW_LOADER
> -       select FW_LOADER_USER_HELPER
> +       bool "Force the firmware sysfs fallback mechanism when possible"
> +       depends on FW_LOADER_USER_HELPER
>         help
> -         This option enables / disables the invocation of user-helper
> -         (e.g. udev) for loading firmware files as a fallback after the
> -         direct file loading in kernel fails.  The user-mode helper is
> -         no longer required unless you have a special firmware file that
> -         resides in a non-standard path. Moreover, the udev support has
> -         been deprecated upstream.
> +         Enabling this option forces a sysfs userspace fallback mechanism
> +         to be used for all firmware requests which explicitly do not disable a
> +         a fallback mechanism. Firmware calls which do prohibit a fallback
> +         mechanism is request_firmware_direct(). This option is kept for
> +          backward compatibility purposes given this precise mechanism can also
> +         be enabled by setting the proc sysctl value to true:
> +
> +              /proc/sys/kernel/firmware_config/force_sysfs_fallback
>
>           If you are unsure about this, say N here.
>
> +endif # FW_LOADER
> +endmenu
> +
>  config WANT_DEV_COREDUMP
>         bool
>         help
> --
> 2.17.0
>


-Kees

-- 
Kees Cook
Pixel Security