Re: [RFC][PATCH v3 4/5] Add new file in LKDTM to test fortified strscpy.

[Kees Cook <keescook@chromium.org>]

On Wed, Oct 21, 2020 at 05:06:07PM +0200, laniel_francis@privacyrequired.com wrote:
> From: Francis Laniel <laniel_francis@privacyrequired.com>
> 
> This new test generates a crash at runtime because there is a write overflow in
> destination string.
> 
> Signed-off-by: Francis Laniel <laniel_francis@privacyrequired.com>
> ---
>  drivers/misc/lkdtm/Makefile             |  1 +
>  drivers/misc/lkdtm/core.c               |  1 +
>  drivers/misc/lkdtm/fortify.c            | 47 +++++++++++++++++++++++++
>  drivers/misc/lkdtm/lkdtm.h              |  3 ++
>  tools/testing/selftests/lkdtm/tests.txt |  1 +
>  5 files changed, 53 insertions(+)
>  create mode 100644 drivers/misc/lkdtm/fortify.c
> 
> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> index c70b3822013f..d898f7b22045 100644
> --- a/drivers/misc/lkdtm/Makefile
> +++ b/drivers/misc/lkdtm/Makefile
> @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM)		+= rodata_objcopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>  lkdtm-$(CONFIG_LKDTM)		+= cfi.o
> +lkdtm-$(CONFIG_LKDTM)		+= fortify.o
>  
>  KASAN_SANITIZE_stackleak.o	:= n
>  KCOV_INSTRUMENT_rodata.o	:= n
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index a002f39a5964..4326e2d09870 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -180,6 +180,7 @@ static const struct crashtype crashtypes[] = {
>  #ifdef CONFIG_X86_32
>  	CRASHTYPE(DOUBLE_FAULT),
>  #endif
> +	CRASHTYPE(FORTIFIED_STRSCPY),
>  };
>  
>  
> diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
> new file mode 100644
> index 000000000000..cecdfbb8ba55
> --- /dev/null
> +++ b/drivers/misc/lkdtm/fortify.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Francis Laniel <laniel_francis@privacyrequired.com>
> + *
> + * Add tests related to fortified functions in this file.
> + */
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include "lkdtm.h"
> +
> +
> +/*
> + * Calls fortified strscpy to test that it returns the same result as vanilla
> + * strscpy and generate a panic because there is a write overflow (i.e. src
> + * length is greater than dst length).
> + */
> +void lkdtm_FORTIFIED_STRSCPY(void)
> +{
> +#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)

I would drop the #if: just let it run and freak out on non-fortified
kernels.

> +	char *src;
> +	char dst[3];
> +
> +	src = kstrdup("foobar", GFP_KERNEL);
> +
> +	if (src == NULL)
> +		return;
> +
> +	/* Vanilla strscpy returns -E2BIG if size is 0. */
> +	WARN_ON(strscpy(dst, src, 0) != -E2BIG);

For LKDTM, we have different reporting that "normal", in the sense that
usually the WARN/BUG outcomes are _desirable_ (i.e. "freak out on stack
overflow"). So, I would write this as:

	if (strscpy(dst, src, 0) != -E2BIG)
		pr_warn("FAIL: strscpy() of 0 length did not return -E2BIG\n");

> +
> +	/* Vanilla strscpy returns -E2BIG if src is truncated. */
> +	WARN_ON(strscpy(dst, src, sizeof(dst)) != -E2BIG);

Same.

> +
> +	/* After above call, dst must contain "fo" because src was truncated. */
> +	WARN_ON(strncmp(dst, "fo", sizeof(dst)) != 0);

Same.

> +
> +	/*
> +	 * Use strlen here so size cannot be known at compile time and there is
> +	 * a runtime overflow.
> +	 */
> +	strscpy(dst, src, strlen(src));

I think we'll need a couple more corner cases, and any that need to Oops
separately can be separate functions. Here's a corner case to test to
strnlen():

struct {
	union {
	        char big[10];
	        char src[5];
	};
} weird = { .big = "hello!" };
char dst[sizeof(weird.src) + 1];

strscpy(dst, weird.src, sizeof(dst));

if (strcmp(dst, "hello") != 0)
	pr_warn("FAIL ...

But each of the cases being tested in the fortified strscpy() should be
exercised.

> +
> +	pr_info("Fail: No overflow in above strscpy call!\n");

pr_warn("FAIL: ...

> +
> +	kfree(src);
> +#endif
> +}
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index 70c8b7c9460f..29c12dcdeab1 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -106,4 +106,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>  /* cfi.c */
>  void lkdtm_CFI_FORWARD_PROTO(void);
>  
> +/* fortify.c */
> +void lkdtm_FORTIFIED_STRSCPY(void);
> +
>  #endif
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 9d266e79c6a2..4234109579eb 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -70,3 +70,4 @@ USERCOPY_KERNEL
>  USERCOPY_KERNEL_DS
>  STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>  CFI_FORWARD_PROTO
> +FORTIFIED_STRSCPY
> \ No newline at end of file
> -- 
> 2.20.1
> 

-- 
Kees Cook