[PATCH] firmware: fix usermode helper fallback loading

[Luis R. Rodriguez]

From: Yves-Alexis Perez <corsac@xxxxxxxxxx>

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@xxxxxxxxxx>
Fixes: 68ff2a00dbf5 "firmware_loader: handle timeout via wait_for_completion_interruptible_timeout()"
Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Acked-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
[mcgrof@xxxxxxxxxx: gave commit log a whole lot of love]
Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
---

Summary:

o the UMH fallback has been broken since Linux v4.0 !
o systemd removed the UMH firmware loader since systemd v217 in 2014 !

And no one had kicked or screamed...

 drivers/base/firmware_class.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760a4278..a95e1e572697 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -955,13 +955,14 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		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;
 	}
 
-- 
2.10.1