linux-kernel - Re: [PATCH 2/4] test: firmware_class: add asynchronous request trigger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jLGp315GdOWQyamD8awzrdZDFoNVWQozTFdjQAjrtRN0w@mail.gmail.com>
Date:	Wed, 9 Dec 2015 14:05:17 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	Shuah Khan <shuahkh@....samsung.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ming Lei <ming.lei@...onical.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux API <linux-api@...r.kernel.org>,
	"Luis R. Rodriguez" <mcgrof@...e.com>
Subject: Re: [PATCH 2/4] test: firmware_class: add asynchronous request trigger

On Wed, Dec 9, 2015 at 1:48 PM, Brian Norris
<computersforpeace@...il.com> 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
>> <computersforpeace@...il.com> 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 <linux/init.h>
>> >  #include <linux/module.h>
>> >  #include <linux/printk.h>
>> > +#include <linux/completion.h>
>> >  #include <linux/firmware.h>
>> >  #include <linux/device.h>
>> >  #include <linux/fs.h>
>> > @@ -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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/