Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757137AbcJ3R3I (ORCPT ); Sun, 30 Oct 2016 13:29:08 -0400 Received: from pic75-3-78-194-244-226.fbxo.proxad.net ([78.194.244.226]:42734 "EHLO mail.corsac.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754090AbcJ3R3H (ORCPT ); Sun, 30 Oct 2016 13:29:07 -0400 X-Greylist: delayed 9487 seconds by postgrey-1.27 at vger.kernel.org; Sun, 30 Oct 2016 13:29:06 EDT Message-ID: <1477848538.3456.1.camel@corsac.net> Subject: Re: [PATCH] firmware: fix async/manual firmware loading From: Yves-Alexis Perez To: linux-kernel@vger.kernel.org Cc: Ming Lei , "Luis R. Rodriguez" , Greg Kroah-Hartman , stable@vger.kernel.org Date: Sun, 30 Oct 2016 18:28:58 +0100 In-Reply-To: <20161030145048.6291-2-corsac@corsac.net> References: <20161030145048.6291-1-corsac@corsac.net> <20161030145048.6291-2-corsac@corsac.net> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-r8nw/TvknXUr+iyQqX2H" X-Mailer: Evolution 3.22.1-2 Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4016 Lines: 102 --=-r8nw/TvknXUr+iyQqX2H Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 i= n > 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. >=20 > 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 mod= ule (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//{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 tim= eout 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 devic= e *dev, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mutex_lock(&test_fw_mutex); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0release_firmware(test_firmw= are); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0test_firmware =3D NULL; -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D request_firmware_nowait(T= HIS_MODULE, 1, name, dev, GFP_KERNEL, +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D request_firmware_nowait(T= HIS_MODULE, 0, name, dev, GFP_KERNEL, =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0NUL= L, trigger_async_request_cb); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (rc) { =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0pr_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_requ= est 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 t= he loading succeeds with: [=C2=A0=C2=A0=C2=A096.405171] test_firmware: loaded: 4 Regards, --=20 Yves-Alexis --=-r8nw/TvknXUr+iyQqX2H Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYFi3aAAoJEG3bU/KmdcCljaIH/i5PWCZgNUZKke1IyoUUCI8B 4lx6xn7yPFdHxwJqn9KM7VYYzV5lRGrGTEGw2ywoSNEfSZwdqyPqqVYIT2Q7/EzO xFM7l59cRdRUnVvoGBhclwCapz7H//fNvWz8SdodZzV8em5tsug2dBkWdUv2t59w 5cOucdSPbaIJeLpB47cfBRNTURUed6GSaXE5RVZjqiBEHAKwYxpRpOL/E+5y0GLt s/q6Th5rzA/339KqoZntpYpTs04osz3G2eSZvGImFIQhtfqeB8OyHXnHlZXloda8 j+thS3BhZqhYimGdvYMwwz4Lzynz0hqIiQcYEpUvO3whP4IKhsfLpCr+HGs6CII= =rcbE -----END PGP SIGNATURE----- --=-r8nw/TvknXUr+iyQqX2H--