| From 2e700f8d85975f516ccaad821278c1fe66b2cc98 Mon Sep 17 00:00:00 2001 |
| From: Yves-Alexis Perez <corsac@corsac.net> |
| Date: Fri, 11 Nov 2016 11:28:40 -0800 |
| Subject: firmware: fix usermode helper fallback loading |
| |
| 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; |
| } |
| |