From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <1483739678.4969.7.camel@corsac.net> Subject: Re: [PATCH 4.8 50/96] firmware: fix usermode helper fallback loading From: Yves-Alexis Perez To: Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Ben Gamari Cc: stable@vger.kernel.org, "Luis R. Rodriguez" , Ming Lei , Bjorn Andersson Date: Fri, 06 Jan 2017 22:54:38 +0100 In-Reply-To: <20170106214229.739771341@linuxfoundation.org> References: <20170106214227.601120243@linuxfoundation.org> <20170106214229.739771341@linuxfoundation.org> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-S3+xK45UX842gwiTdebc" Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: --=-S3+xK45UX842gwiTdebc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-01-06 at 22:43 +0100, Greg Kroah-Hartman wrote: > 4.8-stable review patch.=C2=A0=C2=A0If 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, >=20 > ------------------ >=20 > From: Yves-Alexis Perez >=20 > commit 2e700f8d85975f516ccaad821278c1fe66b2cc98 upstream. >=20 > When you use the firmware usermode helper fallback with a timeout value s= et > 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 kerne= l > v4.0. >=20 > 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: >=20 > =C2=A0 a) You've enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK (not many > distros): >=20 > =C2=A0 Then all of these callers will rely on the fallback mechanism in c= ase > =C2=A0 the firmware is not found through an initial direct filesystem loo= kup: >=20 > =C2=A0 o request_firmware() > =C2=A0 o request_firmware_into_buf() > =C2=A0 o request_firmware_nowait() >=20 > =C2=A0 b) If you've only enabled CONFIG_FW_LOADER_USER_HELPER (most distr= os): >=20 > =C2=A0 Then only callers using request_firmware_nowait() with the second > =C2=A0 argument set to false, this explicitly is requesting the UMH firmw= are > =C2=A0 fallback to be relied on in case the first filesystem lookup fails= . >=20 > =C2=A0 Using Coccinelle SmPL grammar we have identified only two drivers > =C2=A0 explicitly requesting the UMH firmware fallback mechanism: >=20 > =C2=A0 - drivers/firmware/dell_rbu.c > =C2=A0 - drivers/leds/leds-lp55xx-common.c >=20 > 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. >=20 > 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_OFFSE= T > value was also used if the timeout was configured by a user to 0. >=20 > The following works: >=20 > echo 2147483647 > /sys/class/firmware/timeout >=20 > But both of the following set the timeout to MAX_JIFFY_OFFSET even if > we display 0 back to userspace: >=20 > echo 2147483648 > /sys/class/firmware/timeout > cat /sys/class/firmware/timeout > 0 >=20 > echo 0> /sys/class/firmware/timeout > cat /sys/class/firmware/timeout > 0 >=20 > A max value of INT_MAX (2147483647) seconds is therefore implicit due to = the > another cast with simple_strtol(). >=20 > This fixes the secondary cast (the first one is simple_strtol() but its a= n > issue only by forcing an implicit limit) by re-using the timeout variable > and > only setting retval in appropriate cases. >=20 > 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 v21= 7. >=20 > Signed-off-by: Yves-Alexis Perez > Fixes: 68ff2a00dbf5 "firmware_loader: handle timeout via > wait_for_completion_interruptible_timeout()" > Cc: Luis R. Rodriguez > Cc: Ming Lei > Cc: Bjorn Andersson > Cc: Greg Kroah-Hartman > Acked-by: Luis R. Rodriguez > Reviewed-by: Bjorn Andersson > [mcgrof@kernel.org: gave commit log a whole lot of love] > Signed-off-by: Luis R. Rodriguez > Signed-off-by: Greg Kroah-Hartman > Signed-off-by: Greg Kroah-Hartman >=20 > --- > =C2=A0drivers/base/firmware_class.c |=C2=A0=C2=A0=C2=A0=C2=A07 ++++--- > =C2=A01 file changed, 4 insertions(+), 3 deletions(-) >=20 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -955,13 +955,14 @@ static int _request_firmware_load(struct > =C2=A0 timeout =3D MAX_JIFFY_OFFSET; > =C2=A0 } > =C2=A0 > - retval =3D wait_for_completion_interruptible_timeout(&buf- > >completion, > + timeout =3D wait_for_completion_interruptible_timeout(&buf- > >completion, > =C2=A0 timeout); > - if (retval =3D=3D -ERESTARTSYS || !retval) { > + if (timeout =3D=3D -ERESTARTSYS || !timeout) { > + retval =3D timeout; > =C2=A0 mutex_lock(&fw_lock); > =C2=A0 fw_load_abort(fw_priv); > =C2=A0 mutex_unlock(&fw_lock); > - } else if (retval > 0) { > + } else if (timeout > 0) { > =C2=A0 retval =3D 0; > =C2=A0 } > =C2=A0 >=20 >=20 --=20 Yves-Alexis --=-S3+xK45UX842gwiTdebc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEl0WwInMjgf6efq/1bdtT8qZ1wKUFAlhwEh4ACgkQbdtT8qZ1 wKW1vQf/d08l6xkD+XFksJaiFJQczcvi/IqJ8LWvTTlXxsKenZ4slJgARPrAICt2 7VVvzjDFgAhIHZM6X06hnj4ab4z/5XtRMlbPaB3VdUjSlEoR1VI8oUHZ0LBIM8LW Skij9UaDziPftMPAIsbJ8e9RFWXlS/p8cu0cX6O0k3KFhjaT+yN39cQ3/rByJlRZ EtCNz4pYdzzeeQVHcCSQBcLjuWMvMpI121HQA3KrkfnXnHx+ipZopPhc30Q7fKvg 9Oxq7v1C4NVuTCg4x8O2/Q/xAnz1IWQwDRf9ge6NhE/CjtUTeHoXw3/Ia/Jy6Uas ocxhMD6uoLL+tYcrzxWeLo6zaCMTIA== =pIYb -----END PGP SIGNATURE----- --=-S3+xK45UX842gwiTdebc--