Messages in this thread | | | Date | Tue, 29 Sep 2015 10:32:16 -0700 | Subject | Re: [PATCH 4/4] test_printf: test printf family at runtime | From | Kees Cook <> |
| |
On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On Tue, Sep 29 2015, Kees Cook <keescook@chromium.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
| |