16 Oct, 2019

1 commit

  • On a machine with a 64K PAGE_SIZE, the nested for loops in
    test_check_nonzero_user() can lead to soft lockups, eg:

    watchdog: BUG: soft lockup - CPU#4 stuck for 22s! [modprobe:611]
    Modules linked in: test_user_copy(+) vmx_crypto gf128mul crc32c_vpmsum virtio_balloon ip_tables x_tables autofs4
    CPU: 4 PID: 611 Comm: modprobe Tainted: G L 5.4.0-rc1-gcc-8.2.0-00001-gf5a1a536fa14-dirty #1151
    ...
    NIP __might_sleep+0x20/0xc0
    LR __might_fault+0x40/0x60
    Call Trace:
    check_zeroed_user+0x12c/0x200
    test_user_copy_init+0x67c/0x1210 [test_user_copy]
    do_one_initcall+0x60/0x340
    do_init_module+0x7c/0x2f0
    load_module+0x2d94/0x30e0
    __do_sys_finit_module+0xc8/0x150
    system_call+0x5c/0x68

    Even with a 4K PAGE_SIZE the test takes multiple seconds. Instead
    tweak it to only scan a 1024 byte region, but make it cross the
    page boundary.

    Fixes: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
    Suggested-by: Aleksa Sarai
    Signed-off-by: Michael Ellerman
    Reviewed-by: Aleksa Sarai
    Acked-by: Christian Brauner
    Link: https://lore.kernel.org/r/20191016122732.13467-1-mpe@ellerman.id.au
    Signed-off-by: Christian Brauner

    Michael Ellerman
     

07 Oct, 2019

1 commit

  • While writing the tests for copy_struct_from_user(), I used a construct
    that Linus doesn't appear to be too fond of:

    On 2019-10-04, Linus Torvalds wrote:
    > Hmm. That code is ugly, both before and after the fix.
    >
    > This just doesn't make sense for so many reasons:
    >
    > if ((ret |= test(umem_src == NULL, "kmalloc failed")))
    >
    > where the insanity comes from
    >
    > - why "|=" when you know that "ret" was zero before (and it had to
    > be, for the test to make sense)
    >
    > - why do this as a single line anyway?
    >
    > - don't do the stupid "double parenthesis" to hide a warning. Make it
    > use an actual comparison if you add a layer of parentheses.

    So instead, use a bog-standard check that isn't nearly as ugly.

    Fixes: 341115822f88 ("usercopy: Add parentheses around assignment in test_copy_struct_from_user")
    Fixes: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
    Signed-off-by: Aleksa Sarai
    Reviewed-by: Nathan Chancellor
    Reviewed-by: Christian Brauner
    Link: https://lore.kernel.org/r/20191005233028.18566-1-cyphar@cyphar.com
    Signed-off-by: Christian Brauner

    Aleksa Sarai
     

04 Oct, 2019

1 commit

  • Clang warns:

    lib/test_user_copy.c:96:10: warning: using the result of an assignment
    as a condition without parentheses [-Wparentheses]
    if (ret |= test(umem_src == NULL, "kmalloc failed"))
    ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    lib/test_user_copy.c:96:10: note: place parentheses around the
    assignment to silence this warning
    if (ret |= test(umem_src == NULL, "kmalloc failed"))
    ^
    ( )
    lib/test_user_copy.c:96:10: note: use '!=' to turn this compound
    assignment into an inequality comparison
    if (ret |= test(umem_src == NULL, "kmalloc failed"))
    ^~
    !=

    Add the parentheses as it suggests because this is intentional.

    Fixes: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
    Link: https://github.com/ClangBuiltLinux/linux/issues/731
    Signed-off-by: Nathan Chancellor
    Acked-by: Aleksa Sarai
    Acked-by: Christian Brauner
    Link: https://lore.kernel.org/r/20191003171121.2723619-1-natechancellor@gmail.com
    Signed-off-by: Christian Brauner

    Nathan Chancellor
     

01 Oct, 2019

1 commit

  • A common pattern for syscall extensions is increasing the size of a
    struct passed from userspace, such that the zero-value of the new fields
    result in the old kernel behaviour (allowing for a mix of userspace and
    kernel vintages to operate on one another in most cases).

    While this interface exists for communication in both directions, only
    one interface is straightforward to have reasonable semantics for
    (userspace passing a struct to the kernel). For kernel returns to
    userspace, what the correct semantics are (whether there should be an
    error if userspace is unaware of a new extension) is very
    syscall-dependent and thus probably cannot be unified between syscalls
    (a good example of this problem is [1]).

    Previously there was no common lib/ function that implemented
    the necessary extension-checking semantics (and different syscalls
    implemented them slightly differently or incompletely[2]). Future
    patches replace common uses of this pattern to make use of
    copy_struct_from_user().

    Some in-kernel selftests that insure that the handling of alignment and
    various byte patterns are all handled identically to memchr_inv() usage.

    [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
    robustify sched_read_attr() ABI logic and code")

    [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
    similar checks to copy_struct_from_user() while rt_sigprocmask(2)
    always rejects differently-sized struct arguments.

    Suggested-by: Rasmus Villemoes
    Signed-off-by: Aleksa Sarai
    Reviewed-by: Kees Cook
    Reviewed-by: Christian Brauner
    Link: https://lore.kernel.org/r/20191001011055.19283-2-cyphar@cyphar.com
    Signed-off-by: Christian Brauner

    Aleksa Sarai
     

05 Jun, 2019

1 commit

  • Based on 1 normalized pattern(s):

    this software is licensed under the terms of the gnu general public
    license version 2 as published by the free software foundation and
    may be copied distributed and modified under those terms this
    program is distributed in the hope that it will be useful but
    without any warranty without even the implied warranty of
    merchantability or fitness for a particular purpose see the gnu
    general public license for more details

    extracted by the scancode license scanner the SPDX license identifier

    GPL-2.0-only

    has been chosen to replace the boilerplate/reference in 285 file(s).

    Signed-off-by: Thomas Gleixner
    Reviewed-by: Alexios Zavras
    Reviewed-by: Allison Randal
    Cc: linux-spdx@vger.kernel.org
    Link: https://lkml.kernel.org/r/20190529141900.642774971@linutronix.de
    Signed-off-by: Greg Kroah-Hartman

    Thomas Gleixner
     

26 Mar, 2018

1 commit


10 Mar, 2018

1 commit


01 May, 2017

1 commit


23 Feb, 2017

1 commit

  • On a NOMMU ARM kernel, we get this link error:

    ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined!

    The problem is that the extended get_user/put_user definitions
    were only added for the normal (MMU based) case.

    We could add it for NOMMU as well, but it seems easier to just not
    call it, since no other code needs it.

    Fixes: 4c5d7bc63775 ("usercopy: Add tests for all get_user() sizes")
    Signed-off-by: Arnd Bergmann
    Signed-off-by: Kees Cook

    Arnd Bergmann
     

22 Feb, 2017

1 commit

  • The existing test was only exercising native unsigned long size
    get_user(). For completeness, we should check all sizes. But we
    must skip some 32-bit architectures that don't implement a 64-bit
    get_user().

    These new tests actually uncovered a bug in ARM's 64-bit get_user()
    zeroing.

    Signed-off-by: Kees Cook

    Kees Cook
     

17 Feb, 2017

2 commits

  • Under SMAP/PAN/etc, we cannot write directly to userspace memory, so
    this rearranges the test bytes to get written through copy_to_user().
    Additionally drops the bad copy_from_user() test that would trigger a
    memcpy() against userspace on failure.

    Signed-off-by: Kees Cook

    Kees Cook
     
  • During usercopy the destination buffer will be zeroed if copy_from_user()
    or get_user() fails. This patch adds testcases for it. The destination
    buffer is set with non-zero value before illegal copy_from_user() or
    get_user() is executed and the buffer is compared to zero after usercopy
    is done.

    Signed-off-by: Hoeun Ryu
    [kees: clarified commit log, dropped second kmalloc]
    Signed-off-by: Kees Cook

    Hoeun Ryu
     

24 Jan, 2014

1 commit

  • To help avoid an architecture failing to correctly check kernel/user
    boundaries when handling copy_to_user, copy_from_user, put_user, or
    get_user, perform some simple tests and fail to load if any of them
    behave unexpectedly.

    Specifically, this is to make sure there is a way to notice if things
    like what was fixed in commit 8404663f81d2 ("ARM: 7527/1: uaccess:
    explicitly check __user pointer when !CPU_USE_DOMAINS") ever regresses
    again, for any architecture.

    Additionally, adds new "user" selftest target, which loads this module.

    Signed-off-by: Kees Cook
    Cc: Rusty Russell
    Cc: Joe Perches
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Kees Cook