Re: [RFC 3/3] lib: Convert test_user_copy to KUnit test
On Wed, 2020-07-15 at 19:34 -0700, Kees Cook wrote:
> On Wed, Jul 15, 2020 at 12:11:20AM -0300, Vitor Massaru Iha wrote:
> > This adds the conversion of the runtime tests of test_user_copy
> > fuctions,
> > from `lib/test_user_copy.c`to KUnit tests.
> >
> > Signed-off-by: Vitor Massaru Iha
> > [...]
> > @@ -16,6 +16,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Several 32-bit architectures support 64-bit {get,put}_user()
> > calls.
> > @@ -31,26 +32,16 @@
> > # define TEST_U64
> > #endif
> >
> > -#define test(condition, msg, ...)
> > \
> > -({
> > \
> > - int cond = (condition);
> > \
> > - if (cond) \
> > - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> > - cond;
> > \
> > -})
> > -
> > static bool is_zeroed(void *from, size_t size)
> > {
> > return memchr_inv(from, 0x0, size) == NULL;
> > }
> >
> > -static int test_check_nonzero_user(char *kmem, char __user *umem,
> > size_t size)
> > +static void test_check_nonzero_user(struct kunit *test, char
> > *kmem, char __user *umem, size_t size)
> > {
> > - int ret = 0;
> > size_t start, end, i, zero_start, zero_end;
> >
> > - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> > - return -EINVAL;
> > + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too
> > small");
>
> I think this could be a much smaller diff if you just replaced the
> "test" macro:
>
> #define test(condition, msg, ...)
> \
> ({
> \
> int cond = !!(condition); \
> KUNIT_EXPECT_FALSE_MSG(kunit_context, cond, msg,
> ##__VA_ARGS__);\
> cond;
> \
> })
>
Sure, I'll do it.
Thanks.
Re: [RFC 3/3] lib: Convert test_user_copy to KUnit test
On Wed, 2020-07-15 at 17:40 -0700, Brendan Higgins wrote:
> On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha
> wrote:
> > This adds the conversion of the runtime tests of test_user_copy
> > fuctions,
> > from `lib/test_user_copy.c`to KUnit tests.
> >
> > Signed-off-by: Vitor Massaru Iha
> > ---
> > lib/Kconfig.debug | 17 ++
> > lib/Makefile| 2 +-
> > lib/{test_user_copy.c => user_copy_kunit.c} | 196 +---
> >
> > 3 files changed, 102 insertions(+), 113 deletions(-)
> > rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9ad9210d70a1..29558674c011 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2154,6 +2154,23 @@ config SYSCTL_KUNIT_TEST
> >
> > If unsure, say N.
> >
> > +config USER_COPY_KUNIT
> > + tristate "KUnit Test for user/kernel boundary protections"
> > + depends on KUNIT
> > + depends on m
> > + help
> > + This builds the "test_user_copy" module that runs sanity
> > checks
> > + on the copy_to/from_user infrastructure, making sure
> > basic
> > + user/kernel boundary testing is working. If it fails to
> > load,
> > + a regression has been detected in the user/kernel memory
> > boundary
> > + protections.
> > +
> > + For more information on KUnit and unit tests in general
> > please refer
> > + to the KUnit documentation in Documentation/dev-
> > tools/kunit/.
> > +
> > + If unsure, say N.
>
> Where do you delete the entry for CONFIG_TEST_USER_COPY? I don't see
> it here.
I didn't. Thanks I'll delete it.
> > config LIST_KUNIT_TEST
> > tristate "KUnit Test for Kernel Linked-list structures" if
> > !KUNIT_ALL_TESTS
> > depends on KUNIT
> > diff --git a/lib/Makefile b/lib/Makefile
> > index b1c42c10073b..8c145f85accc 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> > obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> > obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> > obj-$(CONFIG_TEST_SORT) += test_sort.o
> > -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> > obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> > obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> > obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> > @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> > # KUnit tests
> > obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> > obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> > +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> > diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> > similarity index 55%
> > rename from lib/test_user_copy.c
> > rename to lib/user_copy_kunit.c
> > index 5ff04d8fe971..c15bb1e997d6 100644
> > --- a/lib/test_user_copy.c
> > +++ b/lib/user_copy_kunit.c
> > @@ -16,6 +16,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Several 32-bit architectures support 64-bit {get,put}_user()
> > calls.
> > @@ -31,26 +32,16 @@
> > # define TEST_U64
> > #endif
> >
> > -#define test(condition, msg,
> > ...) \
> > -({
> > \
> > - int cond =
> > (condition); \
> > - if
> > (cond) \
> > - pr_warn("[%d] " msg "\n", __LINE__,
> > ##__VA_ARGS__); \
> > - cond;
> > \
> > -})
> > -
> > static bool is_zeroed(void *from, size_t size)
> > {
> > return memchr_inv(from, 0x0, size) == NULL;
> > }
> >
> > -static int test_check_nonzero_user(char *kmem, char __user *umem,
> > size_t size)
> > +static void test_check_nonzero_user(struct kunit *test, char
> > *kmem, char __user *umem, size_t size)
> > {
> > - int ret = 0;
> > size_t start, end, i, zero_start, zero_end;
> >
> > - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> > - return -EINVAL;
> > + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer
> > too small");
> >
> > /*
> > * We want to cross a page boundary to exercise the code
> > more
> > @@ -84,7 +75,7 @@ static int test_check_nonzero_user(char *kmem,
> > char __user *umem, size_t size)
> > for (i = zero_end; i < size; i += 2)
> > kmem[i] = 0xff;
> >
> > - ret |= test(copy_to_user(umem, kmem, size),
> > + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, kmem,
> > size),
> > "legitimate copy_to_user failed");
> >
> > for (start = 0; start <= size; start++) {
> > @@ -93,35 +84,31 @@ static int test_check_nonzero_user(char *kmem,
> > char __user *umem, size_t size)
> > int
Re: [RFC 3/3] lib: Convert test_user_copy to KUnit test
On Wed, Jul 15, 2020 at 12:11:20AM -0300, Vitor Massaru Iha wrote:
> This adds the conversion of the runtime tests of test_user_copy fuctions,
> from `lib/test_user_copy.c`to KUnit tests.
>
> Signed-off-by: Vitor Massaru Iha
> [...]
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> @@ -31,26 +32,16 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg, ...)\
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> - cond; \
> -})
> -
> static bool is_zeroed(void *from, size_t size)
> {
> return memchr_inv(from, 0x0, size) == NULL;
> }
>
> -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t
> size)
> +static void test_check_nonzero_user(struct kunit *test, char *kmem, char
> __user *umem, size_t size)
> {
> - int ret = 0;
> size_t start, end, i, zero_start, zero_end;
>
> - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> - return -EINVAL;
> + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too small");
I think this could be a much smaller diff if you just replaced the
"test" macro:
#define test(condition, msg, ...) \
({ \
int cond = !!(condition); \
KUNIT_EXPECT_FALSE_MSG(kunit_context, cond, msg, ##__VA_ARGS__);\
cond; \
})
--
Kees Cook
Re: [RFC 3/3] lib: Convert test_user_copy to KUnit test
On Tue, Jul 14, 2020 at 8:11 PM Vitor Massaru Iha wrote:
>
> This adds the conversion of the runtime tests of test_user_copy fuctions,
> from `lib/test_user_copy.c`to KUnit tests.
>
> Signed-off-by: Vitor Massaru Iha
> ---
> lib/Kconfig.debug | 17 ++
> lib/Makefile| 2 +-
> lib/{test_user_copy.c => user_copy_kunit.c} | 196 +---
> 3 files changed, 102 insertions(+), 113 deletions(-)
> rename lib/{test_user_copy.c => user_copy_kunit.c} (55%)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ad9210d70a1..29558674c011 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2154,6 +2154,23 @@ config SYSCTL_KUNIT_TEST
>
> If unsure, say N.
>
> +config USER_COPY_KUNIT
> + tristate "KUnit Test for user/kernel boundary protections"
> + depends on KUNIT
> + depends on m
> + help
> + This builds the "test_user_copy" module that runs sanity checks
> + on the copy_to/from_user infrastructure, making sure basic
> + user/kernel boundary testing is working. If it fails to load,
> + a regression has been detected in the user/kernel memory boundary
> + protections.
> +
> + For more information on KUnit and unit tests in general please
> refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
Where do you delete the entry for CONFIG_TEST_USER_COPY? I don't see it here.
> config LIST_KUNIT_TEST
> tristate "KUnit Test for Kernel Linked-list structures" if
> !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index b1c42c10073b..8c145f85accc 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,7 +78,6 @@ obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> obj-$(CONFIG_TEST_SORT) += test_sort.o
> -obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> # KUnit tests
> obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> +obj-$(CONFIG_USER_COPY_KUNIT) += user_copy_kunit.o
> diff --git a/lib/test_user_copy.c b/lib/user_copy_kunit.c
> similarity index 55%
> rename from lib/test_user_copy.c
> rename to lib/user_copy_kunit.c
> index 5ff04d8fe971..c15bb1e997d6 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/user_copy_kunit.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Several 32-bit architectures support 64-bit {get,put}_user() calls.
> @@ -31,26 +32,16 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg, ...) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> - cond; \
> -})
> -
> static bool is_zeroed(void *from, size_t size)
> {
> return memchr_inv(from, 0x0, size) == NULL;
> }
>
> -static int test_check_nonzero_user(char *kmem, char __user *umem, size_t
> size)
> +static void test_check_nonzero_user(struct kunit *test, char *kmem, char
> __user *umem, size_t size)
> {
> - int ret = 0;
> size_t start, end, i, zero_start, zero_end;
>
> - if (test(size < 2 * PAGE_SIZE, "buffer too small"))
> - return -EINVAL;
> + KUNIT_EXPECT_FALSE_MSG(test, size < 2 * PAGE_SIZE, "buffer too
> small");
>
> /*
> * We want to cross a page boundary to exercise the code more
> @@ -84,7 +75,7 @@ static int test_check_nonzero_user(char *kmem, char __user
> *umem, size_t size)
> for (i = zero_end; i < size; i += 2)
> kmem[i] = 0xff;
>
> - ret |= test(copy_to_user(umem, kmem, size),
> + KUNIT_EXPECT_FALSE_MSG(test, copy_to_user(umem, kmem, size),
> "legitimate copy_to_user failed");
>
> for (start = 0; start <= size; start++) {
> @@ -93,35 +84,31 @@ static int test_check_nonzero_user(char *kmem, char
> __user *umem, size_t size)
> int retval = check_zeroed_user(umem + start, len);
> int expected = is_zeroed(kmem + start, len);
>
> - ret |= test(retval != expected,
> - "check_nonzero_user(=%d) !=
> memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
> - retval, expected, start, end);
> +
