Message-ID: <CAGXu5jK0eYoR2c6dAjj5BL2Kc526+Qk1udQqOGJV_+aDjhUjiA@mail.gmail.com>
Date:	Tue, 29 Sep 2015 10:32:16 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] test_printf: test printf family at runtime

On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
> On Tue, Sep 29 2015, Kees Cook <keescook@...omium.org> wrote:
>
>>> +static void __init
>>> +test_string(void)
>>> +{
>>> +       test("", "%s%.0s", "", "123");
>>> +       test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
>>> +       test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
>>> +       /*
>>> +        * POSIX and C99 say that a missing precision should be
>>> +        * treated as a precision of 0. However, the kernel's printf
>>> +        * implementation treats this case as if the . wasn't
>>> +        * present. Let's add a test case documenting the current
>>> +        * behaviour; should anyone ever feel the need to follow the
>>> +        * standards more closely, this can be revisited.
>>> +        */
>>> +       test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
>>> +       test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>>> +}
>>
>> Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
>> change) as well?
>
> I suppose you'd also want checks for the somewhat more important
> vsnprintf size check and unknown specifiers? I guess I could, but do we
> really want to intentionally trigger WARN_ON_ONCEs? Say some distro
> chooses to load this module at boot time, then we'd both spam the kernel
> log with "false positives", and we'd have effectively disabled the
> WARN_ON_ONCEs for the actual kernel code.

Distros don't tend to run the test modules by default. The most common
case is that it's part of a selftests run, in which case the machine
has usually been freshly booted, etc. I think it's more important to
catch regressions.

> Maybe we can hide such things behind some module parameter, so that the
> user explicitly has to ask for them. Also, we can't really probe the
> "success" if these sanity checks from within the module (can we?) - the
> user would have to check dmesg manually anyway.

I think it's best that tests run with as few options as possible.
Surely we can test the behavior? The bstr returns 0, so the string
should be truncated? I haven't looked closely, but it seemed testable.

>
>>> +
>>> +static void __init
>>> +dentry(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_va_format(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_clk(void)
>>> +{
>>> +}
>>
>> For the empty functions, maybe just add a pr_info("TODO: struct_clk")
>> or something?
>
> I think that would be unnecessarily spammy. It should be obvious from
> the code that it is just waiting for someone to fill in the blanks.

Ok.

>
>>> +static int __init
>>> +test_printf_init(void)
>>> +{
>>> +       test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
>>> +       if (!test_buffer)
>>> +               return -ENOMEM;
>>> +
>>> +       test_basic();
>>> +       test_number();
>>> +       test_string();
>>> +       test_pointer();
>>> +
>>> +       kfree(test_buffer);
>>> +
>>> +       if (failed_tests == 0)
>>> +               pr_info("all %u tests passed\n", total_tests);
>>> +       else
>>> +               pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
>>> +
>>> +       return 0;
>>> +}
>>
>> I actually have different feedback on leaving the module loaded: I
>> think it should succeed to load when the tests pass and fail when they
>> don't. This makes it a one-step test to check things ("what is
>> modprobe's return code?"), instead of needed to parse dmesg.
>
> Hm, I guess that makes sense. But, assuming we go with the module param
> suggested above, would it be possible to (unload and) load with a
> different set of parameters?

Sure, you've freed memory already, it should be entirely safe to
unload (which is what the selftests script should do anyway). I still
don't think it should have options, though.

>> I love tests! Thank you. :) One suggestion would be to wire it up to
>> the tools/testing/selftests tree; it should be trivial once you change
>> the test_printf_init return code.
>
> I'll look into that. Not sure I have too much time to work on this this
> side of the merge window, and since these all seem to be things that can
> be incrementally added, I'd prefer seeing something go into 4.4 instead
> of waiting till it's "perfect". So unless I hear otherwise, I'll post a
> v2 with the minor things addressed and ask Andrew to take that through
> -mm.

I'll send the glue patch...

-Kees

-- 
Kees Cook
Chrome OS 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/

Powered by blists - more mailing lists