Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752891AbbLIVN4 (ORCPT ); Wed, 9 Dec 2015 16:13:56 -0500 Received: from mail-ig0-f182.google.com ([209.85.213.182]:36735 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750902AbbLIVNx convert rfc822-to-8bit (ORCPT ); Wed, 9 Dec 2015 16:13:53 -0500 MIME-Version: 1.0 In-Reply-To: <1449628714-66641-3-git-send-email-computersforpeace@gmail.com> References: <1449628714-66641-1-git-send-email-computersforpeace@gmail.com> <1449628714-66641-3-git-send-email-computersforpeace@gmail.com> Date: Wed, 9 Dec 2015 13:13:52 -0800 X-Google-Sender-Auth: tBoGYe9XsB0v41gPR442Omd3q4s Message-ID: Subject: Re: [PATCH 3/4] firmware: actually return NULL on failed request_firmware_nowait() From: Kees Cook To: Brian Norris Cc: Shuah Khan , Greg Kroah-Hartman , Ming Lei , LKML , Linux API , "Luis R. Rodriguez" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6701 Lines: 121 On Tue, Dec 8, 2015 at 6:38 PM, Brian Norris wrote: > The kerneldoc for request_firmware_nowait() says that it may call the > provided cont() callback with @fw == NULL, if the firmware request > fails. However, this is not the case when called with an empty string > (""). This case is short-circuited by the 'name[0] == '\0'' check > introduced in commit 471b095dfe0d ("firmware_class: make sure fw requests > contain a name"), so _request_firmware() never gets to set the fw to > NULL. > > Noticed while using the new 'trigger_async_request' testing hook: > > # printf '\x00' > /sys/devices/virtual/misc/test_firmware/trigger_async_request > [10553.726178] test_firmware: loading '' > [10553.729859] test_firmware: loaded: 995209091 > # printf '\x00' > /sys/devices/virtual/misc/test_firmware/trigger_async_request > [10733.676184] test_firmware: loading '' > [10733.679855] Unable to handle kernel NULL pointer dereference at virtual address 00000004 > [10733.687951] pgd = ec188000 > [10733.690655] [00000004] *pgd=00000000 > [10733.694240] Internal error: Oops: 5 [#1] SMP ARM > [10733.698847] Modules linked in: btmrvl_sdio btmrvl bluetooth sbs_battery nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables asix usbnet mwifiex_sdio mwifiex cfg80211 jitterentropy_rng drbg joydev snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device ppp_async ppp_generic slhc tun > [10733.725670] CPU: 0 PID: 6600 Comm: bash Not tainted 4.4.0-rc4-00351-g63d0877 #178 > [10733.733137] Hardware name: Rockchip (Device Tree) > [10733.737831] task: ed24f6c0 ti: ee322000 task.ti: ee322000 > [10733.743222] PC is at do_raw_spin_lock+0x18/0x1a0 > [10733.747831] LR is at _raw_spin_lock+0x18/0x1c > [10733.752180] pc : [] lr : [] psr: a00d0013 > [10733.752180] sp : ee323df8 ip : ee323e20 fp : ee323e1c > [10733.763634] r10: 00000051 r9 : b6f18000 r8 : ee323f80 > [10733.768847] r7 : c089cebc r6 : 00000001 r5 : 00000000 r4 : ec0e6000 > [10733.775360] r3 : dead4ead r2 : c06bd140 r1 : eef913b4 r0 : 00000000 > [10733.781874] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > [10733.788995] Control: 10c5387d Table: 2c18806a DAC: 00000051 > [10733.794728] Process bash (pid: 6600, stack limit = 0xee322218) > [10733.800549] Stack: (0xee323df8 to 0xee324000) > [10733.804896] 3de0: ec0e6000 00000000 > [10733.813059] 3e00: 00000001 c089cebc ee323f80 b6f18000 ee323e2c ee323e20 c054c204 c0065394 > [10733.821221] 3e20: ee323e44 ee323e30 c02fec60 c054c1f8 ec0e7ec0 ec3fcfc0 ee323e5c ee323e48 > [10733.829384] 3e40: c02fed08 c02fec48 c07dbf74 eeb05a00 ee323e8c ee323e60 c0253828 c02fecac > [10733.837547] 3e60: 00000001 c0116950 ee323eac ee323e78 00000001 ec3fce00 ed2d9700 ed2d970c > [10733.845710] 3e80: ee323e9c ee323e90 c02e873c c02537d4 ee323eac ee323ea0 c017bd40 c02e8720 > [10733.853873] 3ea0: ee323ee4 ee323eb0 c017b250 c017bd00 00000000 00000000 f3e47a54 ec128b00 > [10733.862035] 3ec0: c017b10c ee323f80 00000001 c000f504 ee322000 00000000 ee323f4c ee323ee8 > [10733.870197] 3ee0: c011b71c c017b118 ee323fb0 c011bc90 becfa8d9 00000001 ec128b00 00000001 > [10733.878359] 3f00: b6f18000 ee323f80 ee323f4c ee323f18 c011bc90 c0063950 ee323f3c ee323f28 > [10733.886522] 3f20: c0063950 c0549138 00000001 ec128b00 00000001 ec128b00 b6f18000 ee323f80 > [10733.894684] 3f40: ee323f7c ee323f50 c011bed8 c011b6ec c0135fb8 c0135f24 ec128b00 ec128b00 > [10733.902847] 3f60: 00000001 b6f18000 c000f504 ee322000 ee323fa4 ee323f80 c011c664 c011be24 > [10733.911009] 3f80: 00000000 00000000 00000001 b6f18000 b6e79be0 00000004 00000000 ee323fa8 > [10733.919172] 3fa0: c000f340 c011c618 00000001 b6f18000 00000001 b6f18000 00000001 00000000 > [10733.927334] 3fc0: 00000001 b6f18000 b6e79be0 00000004 00000001 00000001 8068a3f1 b6e79c84 > [10733.935496] 3fe0: 00000000 becfa7dc b6de194d b6e20246 400d0030 00000001 7a4536e8 49bda390 > [10733.943664] [] (do_raw_spin_lock) from [] (_raw_spin_lock+0x18/0x1c) > [10733.951743] [] (_raw_spin_lock) from [] (fw_free_buf+0x24/0x64) > [10733.959388] [] (fw_free_buf) from [] (release_firmware+0x68/0x74) > [10733.967207] [] (release_firmware) from [] (trigger_async_request_store+0x60/0x124) > [10733.976501] [] (trigger_async_request_store) from [] (dev_attr_store+0x28/0x34) > [10733.985533] [] (dev_attr_store) from [] (sysfs_kf_write+0x4c/0x58) > [10733.993437] [] (sysfs_kf_write) from [] (kernfs_fop_write+0x144/0x1a8) > [10734.001689] [] (kernfs_fop_write) from [] (__vfs_write+0x3c/0xe4) > > After this patch: > > # printf '\x00' > /sys/devices/virtual/misc/test_firmware/trigger_async_request > [ 32.126322] test_firmware: loading '' > [ 32.129995] test_firmware: failed to async load firmware > -bash: printf: write error: No such device > > Fixes: 471b095dfe0d ("firmware_class: make sure fw requests contain a name") > Signed-off-by: Brian Norris Good catch, thanks! Acked-by: Kees Cook -Kees > --- > drivers/base/firmware_class.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 8524450e75bd..b9250e564ebf 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -1118,15 +1118,17 @@ static int > _request_firmware(const struct firmware **firmware_p, const char *name, > struct device *device, unsigned int opt_flags) > { > - struct firmware *fw; > + struct firmware *fw = NULL; > long timeout; > int ret; > > if (!firmware_p) > return -EINVAL; > > - if (!name || name[0] == '\0') > - return -EINVAL; > + if (!name || name[0] == '\0') { > + ret = -EINVAL; > + goto out; > + } > > ret = _request_firmware_prepare(&fw, name, device); > if (ret <= 0) /* error or already assigned */ > -- > 2.6.0.rc2.230.g3dd15c0 > -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/