Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754142AbbLIVst (ORCPT ); Wed, 9 Dec 2015 16:48:49 -0500 Received: from mail-pf0-f179.google.com ([209.85.192.179]:35019 "EHLO mail-pf0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753939AbbLIVsq (ORCPT ); Wed, 9 Dec 2015 16:48:46 -0500 Date: Wed, 9 Dec 2015 13:48:43 -0800 From: Brian Norris To: Kees Cook Cc: Shuah Khan , Greg Kroah-Hartman , Ming Lei , LKML , Linux API , "Luis R. Rodriguez" Subject: Re: [PATCH 2/4] test: firmware_class: add asynchronous request trigger Message-ID: <20151209214843.GA51175@google.com> References: <1449628714-66641-1-git-send-email-computersforpeace@gmail.com> <1449628714-66641-2-git-send-email-computersforpeace@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3601 Lines: 124 Hi Kees, On Wed, Dec 09, 2015 at 01:09:02PM -0800, Kees Cook wrote: > On Tue, Dec 8, 2015 at 6:38 PM, Brian Norris > wrote: > > diff --git a/lib/test_firmware.c b/lib/test_firmware.c > > index 841191061816..ba0a12d0301d 100644 > > --- a/lib/test_firmware.c > > +++ b/lib/test_firmware.c > > @@ -12,6 +12,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -81,6 +82,57 @@ out: > > } > > static DEVICE_ATTR_WO(trigger_request); > > > > +static DECLARE_COMPLETION(async_fw_done); > > + > > +static void trigger_async_request_cb(const struct firmware *fw, void *context) > > +{ > > + test_firmware = fw; > > + complete(&async_fw_done); > > +} > > + > > +static ssize_t trigger_async_request_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int rc; > > + char *name; > > + > > + name = kzalloc(count + 1, GFP_KERNEL); > > + if (!name) > > + return -ENOSPC; > > + memcpy(name, buf, count); > > It strikes me that this (and the existing code) should use kstrndup > instead, since the request_firmware* interfaces will ignore \0 bytes > in the name: > > name = kstrndup(buf, count, GFP_KERNEL); > if (!name) > return -ENOSPC; Thought of that at some point, then for some reason I didn't do it. Probably laziness... Will do in a v2, along with the more important fix below. > > + > > + pr_info("loading '%s'\n", name); > > + > > + mutex_lock(&test_fw_mutex); > > + release_firmware(test_firmware); > > + test_firmware = NULL; > > + rc = request_firmware_nowait(THIS_MODULE, 1, name, dev, GFP_KERNEL, > > + NULL, trigger_async_request_cb); > > + /* Free 'name' ASAP, to test for race conditions */ > > + kfree(name); > > + if (rc) { > > + pr_info("async load of '%s' failed: %d\n", name, rc); > > Well, that's a little TOO soon. :) The pr_info uses it still. Haha, yeah... nice catch. I was also thinking, since use-after-free isn't necessarily immediately obvious (this worked fine in my testing), that maybe we could poison the buffer before kfree()'ing? Like: name = ...; len = strlen(name); ... rc = request_firmware_nowait(...); if (rc) { pr_info("..."); kfree(name); goto out; } /* * Clear out the name, to test for race conditions with the * async request */ memset(name, 0, len); kfree(name); > > + goto out; > > + } > > + > > + wait_for_completion(&async_fw_done); > > + > > + if (test_firmware) { > > + pr_info("loaded: %zu\n", test_firmware->size); > > + rc = count; > > + } else { > > + pr_err("failed to async load firmware\n"); > > + rc = -ENODEV; > > + } > > + > > +out: > > + mutex_unlock(&test_fw_mutex); > > + > > + return rc; > > +} > > +static DEVICE_ATTR_WO(trigger_async_request); > > + > > static int __init test_firmware_init(void) > > { > > int rc; ... Brian -- 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/