Messages in this thread | | | Subject | Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading | From | Yves-Alexis Perez <> | Date | Fri, 06 Jan 2017 22:54:38 +0100 |
| |
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[unhandled content-type:application/pgp-signature] | |