Re: [PATCH] firmware: fix async/manual firmware loading

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

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

On Sun, 2016-10-30 at 15:50 +0100, Yves-Alexis Perez wrote:
> wait_for_completion_interruptible_timeout() return value is either
> -ERESTARTSYS (in case it was interrupted), 0 (in case the timeout expired)
> or the number of jiffies left until timeout. The return value is stored in
> a long, but in _request_firmware_load() it's silently casted to an int,
> which can overflow and give a negative value, indicating an error.
> 
> Fix this by re-using the timeout variable and only set retval when it's
> safe.

I completely messed-up my git send-email (sorry), so the cover letter only
went to LKML and I assume nobody read it. So just in case, I'm pasting it
below because it gives some more explanation about how I tested this:

when trying to (ab)use the firmware loading interface in a local kernel module
(in order to load the EEPROM content into a PCIE card), I noticed that the
manual firmware loading interface (the one from
/sys/class/firmware/<foo>/{loading,data}) was broken.

After instrumenting the kernel I noticed an issue with the way
wait_for_completion_interruptible_timeout() is called in _request_firmware()
and especially how the return value is handled: it's supposed to be a long, but
here it's silently casted to an int and tested if positive. The initial timeout
seems to be LONG_MAX (since it's a manual firmware loading you're supposed to
have all the time you want to do it), so the return value overflows the int.

Attached patch fixes the problem here, although there might be a better way. I
tested it using lib/test_firmware.c kernel module, with the following patch to
enable manual loading:

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index a3e8ec3..01d333c 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -105,7 +105,7 @@ static ssize_t trigger_async_request_store(struct device *dev,
        mutex_lock(&test_fw_mutex);
        release_firmware(test_firmware);
        test_firmware = NULL;
-       rc = request_firmware_nowait(THIS_MODULE, 1, name, dev, GFP_KERNEL,
+       rc = request_firmware_nowait(THIS_MODULE, 0, name, dev, GFP_KERNEL,
                                     NULL, trigger_async_request_cb);
        if (rc) {
                pr_info("async load of '%s' failed: %d\n", name, rc);

Then load test_firmware and:

# echo -n test > /sys/devices/virtual/misc/test_firmware/trigger_async_request

In another terminal, do:

# echo -n 1 > /sys/class/firmware/test/loading
# echo -n data > /sys/class/firmware/test/data
# echo -n 0 > /sys/class/firmware/test/loading

Without the patch, the loading fails right after the "echo -n 0", with it the loading succeeds with:

[   96.405171] test_firmware: loaded: 4

Regards,
-- 
Yves-Alexis

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