Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading

[Yves-Alexis Perez <corsac@corsac.net>]

[-- Attachment #1: Type: text/plain, Size: 4957 bytes --]

On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote:
> 4.8-stable review patch.  If anyone has any objections, please let me know.

Hi Greg,

Ben Gamari think there was a regression in that patch so I'm adding him to
recipients so he can voice concerns if needed.

Regards,
> 
> ------------------
> 
> From: Yves-Alexis Perez <corsac@corsac.net>
> 
> commit 2e700f8d85975f516ccaad821278c1fe66b2cc98 upstream.
> 
> When you use the firmware usermode helper fallback with a timeout value set
> to a
> value greater than INT_MAX (2147483647) a cast overflow issue causes the
> timeout value to go negative and breaks all usermode helper loading. This
> regression was introduced through commit 68ff2a00dbf5 ("firmware_loader:
> handle timeout via wait_for_completion_interruptible_timeout()") on kernel
> v4.0.
> 
> The firmware_class drivers relies on the firmware usermode helper
> fallback as a mechanism to look for firmware if the direct filesystem
> search failed only if:
> 
>   a) You've enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK (not many
> distros):
> 
>   Then all of these callers will rely on the fallback mechanism in case
>   the firmware is not found through an initial direct filesystem lookup:
> 
>   o request_firmware()
>   o request_firmware_into_buf()
>   o request_firmware_nowait()
> 
>   b) If you've only enabled CONFIG_FW_LOADER_USER_HELPER (most distros):
> 
>   Then only callers using request_firmware_nowait() with the second
>   argument set to false, this explicitly is requesting the UMH firmware
>   fallback to be relied on in case the first filesystem lookup fails.
> 
>   Using Coccinelle SmPL grammar we have identified only two drivers
>   explicitly requesting the UMH firmware fallback mechanism:
> 
>   - drivers/firmware/dell_rbu.c
>   - drivers/leds/leds-lp55xx-common.c
> 
> Since most distributions only enable CONFIG_FW_LOADER_USER_HELPER the
> biggest impact of this regression are users of the dell_rbu and
> leds-lp55xx-common device driver which required the UMH to find their
> respective needed firmwares.
> 
> The default timeout for the UMH is set to 60 seconds always, as of
> commit 68ff2a00dbf5 ("firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()") the timeout was bumped
> to MAX_JIFFY_OFFSET ((LONG_MAX >> 1)-1). Additionally the MAX_JIFFY_OFFSET
> value was also used if the timeout was configured by a user to 0.
> 
> The following works:
> 
> echo 2147483647 > /sys/class/firmware/timeout
> 
> But both of the following set the timeout to MAX_JIFFY_OFFSET even if
> we display 0 back to userspace:
> 
> echo 2147483648 > /sys/class/firmware/timeout
> cat /sys/class/firmware/timeout
> 0
> 
> echo 0> /sys/class/firmware/timeout
> cat /sys/class/firmware/timeout
> 0
> 
> A max value of INT_MAX (2147483647) seconds is therefore implicit due to the
> another cast with simple_strtol().
> 
> This fixes the secondary cast (the first one is simple_strtol() but its an
> issue only by forcing an implicit limit) by re-using the timeout variable
> and
> only setting retval in appropriate cases.
> 
> Lastly worth noting systemd had ripped out the UMH firmware fallback
> mechanism from udev since udev 2014 via commit be2ea723b1d023b3d
> ("udev: remove userspace firmware loading support"), so as of systemd v217.
> 
> Signed-off-by: Yves-Alexis Perez <corsac@corsac.net>
> Fixes: 68ff2a00dbf5 "firmware_loader: handle timeout via
> wait_for_completion_interruptible_timeout()"
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> [mcgrof@kernel.org: gave commit log a whole lot of love]
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
>  drivers/base/firmware_class.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -955,13 +955,14 @@ static int _request_firmware_load(struct
>  		timeout = MAX_JIFFY_OFFSET;
>  	}
>  
> -	retval = wait_for_completion_interruptible_timeout(&buf-
> >completion,
> +	timeout = wait_for_completion_interruptible_timeout(&buf-
> >completion,
>  			timeout);
> -	if (retval == -ERESTARTSYS || !retval) {
> +	if (timeout == -ERESTARTSYS || !timeout) {
> +		retval = timeout;
>  		mutex_lock(&fw_lock);
>  		fw_load_abort(fw_priv);
>  		mutex_unlock(&fw_lock);
> -	} else if (retval > 0) {
> +	} else if (timeout > 0) {
>  		retval = 0;
>  	}
>  
> 
> 
-- 
Yves-Alexis

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]