Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752898AbbLIWFX (ORCPT ); Wed, 9 Dec 2015 17:05:23 -0500 Received: from mail-ig0-f179.google.com ([209.85.213.179]:33244 "EHLO mail-ig0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752293AbbLIWFS (ORCPT ); Wed, 9 Dec 2015 17:05:18 -0500 MIME-Version: 1.0 In-Reply-To: <20151209214843.GA51175@google.com> References: <1449628714-66641-1-git-send-email-computersforpeace@gmail.com> <1449628714-66641-2-git-send-email-computersforpeace@gmail.com> <20151209214843.GA51175@google.com> Date: Wed, 9 Dec 2015 14:05:17 -0800 X-Google-Sender-Auth: 0n38hQjM4pGk2xrO9NFZlrtrqWc Message-ID: Subject: Re: [PATCH 2/4] test: firmware_class: add asynchronous request trigger 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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4168 Lines: 139 On Wed, Dec 9, 2015 at 1:48 PM, Brian Norris wrote: > 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); Hrm, well, I'm not against it, but I think running under KASan is probably the right way to find these things. But, might as well, just to notice any regressions. -Kees > >> > + 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 -- 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/