From: Francis Laniel <[email protected]>
This new test ensures that fortified strscpy has the same behavior than vanilla
strscpy (e.g. returning -E2BIG when src content is truncated).
Finally, it generates a crash at runtime because there is a write overflow in
destination string.
Signed-off-by: Francis Laniel <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
---
drivers/misc/lkdtm/Makefile | 1 +
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/fortify.c | 82 +++++++++++++++++++++++++
drivers/misc/lkdtm/lkdtm.h | 3 +
tools/testing/selftests/lkdtm/tests.txt | 1 +
5 files changed, 88 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 b8c51a633fcc..3c0a67f072c0 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -175,6 +175,7 @@ static const struct crashtype crashtypes[] = {
CRASHTYPE(USERCOPY_KERNEL),
CRASHTYPE(STACKLEAK_ERASING),
CRASHTYPE(CFI_FORWARD_PROTO),
+ CRASHTYPE(FORTIFIED_STRSCPY),
#ifdef CONFIG_X86_32
CRASHTYPE(DOUBLE_FAULT),
#endif
diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
new file mode 100644
index 000000000000..790d46591bf5
--- /dev/null
+++ b/drivers/misc/lkdtm/fortify.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Francis Laniel <[email protected]>
+ *
+ * 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)
+{
+ char *src;
+ char dst[5];
+
+ struct {
+ union {
+ char big[10];
+ char src[5];
+ };
+ } weird = { .big = "hello!" };
+ char weird_dst[sizeof(weird.src) + 1];
+
+ src = kstrdup("foobar", GFP_KERNEL);
+
+ if (src == NULL)
+ return;
+
+ /* Vanilla strscpy returns -E2BIG if size is 0. */
+ 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. */
+ if (strscpy(dst, src, sizeof(dst)) != -E2BIG)
+ pr_warn("FAIL: strscpy() did not return -E2BIG while src is truncated\n");
+
+ /* After above call, dst must contain "foob" because src was truncated. */
+ if (strncmp(dst, "foob", sizeof(dst)) != 0)
+ pr_warn("FAIL: after strscpy() dst does not contain \"foob\" but \"%s\"\n",
+ dst);
+
+ /* Shrink src so the strscpy() below succeeds. */
+ src[3] = '\0';
+
+ /*
+ * Vanilla strscpy returns number of character copied if everything goes
+ * well.
+ */
+ if (strscpy(dst, src, sizeof(dst)) != 3)
+ pr_warn("FAIL: strscpy() did not return 3 while src was copied entirely truncated\n");
+
+ /* After above call, dst must contain "foo" because src was copied. */
+ if (strncmp(dst, "foo", sizeof(dst)) != 0)
+ pr_warn("FAIL: after strscpy() dst does not contain \"foo\" but \"%s\"\n",
+ dst);
+
+ /* Test when src is embedded inside a union. */
+ strscpy(weird_dst, weird.src, sizeof(weird_dst));
+
+ if (strcmp(weird_dst, "hello") != 0)
+ pr_warn("FAIL: after strscpy() weird_dst does not contain \"hello\" but \"%s\"\n",
+ weird_dst);
+
+ /* Restore src to its initial value. */
+ src[3] = 'b';
+
+ /*
+ * Use strlen here so size cannot be known at compile time and there is
+ * a runtime write overflow.
+ */
+ strscpy(dst, src, strlen(src));
+
+ pr_warn("FAIL: No overflow in above strscpy()\n");
+
+ kfree(src);
+}
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index 49e6b945feb7..138f06254b61 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -104,4 +104,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 74a8d329a72c..92ba4cc41314 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -68,3 +68,4 @@ USERCOPY_STACK_BEYOND
USERCOPY_KERNEL
STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
CFI_FORWARD_PROTO
+FORTIFIED_STRSCPY
--
2.20.1
On Wed, Nov 18, 2020 at 12:07:30PM +0100, [email protected] wrote:
> From: Francis Laniel <[email protected]>
>
> This new test ensures that fortified strscpy has the same behavior than vanilla
> strscpy (e.g. returning -E2BIG when src content is truncated).
> Finally, it generates a crash at runtime because there is a write overflow in
> destination string.
>
> Signed-off-by: Francis Laniel <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> ---
> drivers/misc/lkdtm/Makefile | 1 +
> drivers/misc/lkdtm/core.c | 1 +
> drivers/misc/lkdtm/fortify.c | 82 +++++++++++++++++++++++++
> drivers/misc/lkdtm/lkdtm.h | 3 +
> tools/testing/selftests/lkdtm/tests.txt | 1 +
> 5 files changed, 88 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 b8c51a633fcc..3c0a67f072c0 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -175,6 +175,7 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(USERCOPY_KERNEL),
> CRASHTYPE(STACKLEAK_ERASING),
> CRASHTYPE(CFI_FORWARD_PROTO),
> + CRASHTYPE(FORTIFIED_STRSCPY),
> #ifdef CONFIG_X86_32
> CRASHTYPE(DOUBLE_FAULT),
> #endif
> diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
> new file mode 100644
> index 000000000000..790d46591bf5
> --- /dev/null
> +++ b/drivers/misc/lkdtm/fortify.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Francis Laniel <[email protected]>
> + *
> + * Add tests related to fortified functions in this file.
> + */
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include "lkdtm.h"
Ah, I just noticed one small nit here during build testing: lkdtm.h
needs to be included first to get the correct pr_fmt to avoid a warning:
In file included from drivers/misc/lkdtm/fortify.c:9:
drivers/misc/lkdtm/lkdtm.h:5: warning: "pr_fmt" redefined
5 | #define pr_fmt(fmt) "lkdtm: " fmt
-Kees
--
Kees Cook
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on kselftest/next linus/master hnaz-linux-mm/master v5.10-rc4 next-20201118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/laniel_francis-privacyrequired-com/Fortify-strscpy/20201118-190826
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 93c69b2d17372463ae33b79b3002c22a208945b3
config: s390-randconfig-r025-20201118 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project b2613fb2f0f53691dd0211895afbb9413457fca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/b8ce9dd880f1853b99ef29a47088fe8f9c2bf885
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review laniel_francis-privacyrequired-com/Fortify-strscpy/20201118-190826
git checkout b8ce9dd880f1853b99ef29a47088fe8f9c2bf885
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from drivers/misc/lkdtm/fortify.c:9:
>> drivers/misc/lkdtm/lkdtm.h:5:9: warning: 'pr_fmt' macro redefined [-Wmacro-redefined]
#define pr_fmt(fmt) "lkdtm: " fmt
^
include/linux/printk.h:301:9: note: previous definition is here
#define pr_fmt(fmt) fmt
^
1 warning generated.
vim +/pr_fmt +5 drivers/misc/lkdtm/lkdtm.h
9a49a528dcf3c20 drivers/misc/lkdtm.h Kees Cook 2016-02-22 4
6d2e91a662256fd drivers/misc/lkdtm.h Kees Cook 2016-07-15 @5 #define pr_fmt(fmt) "lkdtm: " fmt
6d2e91a662256fd drivers/misc/lkdtm.h Kees Cook 2016-07-15 6
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on kselftest/next linus/master hnaz-linux-mm/master v5.10-rc4 next-20201118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/laniel_francis-privacyrequired-com/Fortify-strscpy/20201118-190826
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 93c69b2d17372463ae33b79b3002c22a208945b3
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b8ce9dd880f1853b99ef29a47088fe8f9c2bf885
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review laniel_francis-privacyrequired-com/Fortify-strscpy/20201118-190826
git checkout b8ce9dd880f1853b99ef29a47088fe8f9c2bf885
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from drivers/misc/lkdtm/fortify.c:9:
>> drivers/misc/lkdtm/lkdtm.h:5: warning: "pr_fmt" redefined
5 | #define pr_fmt(fmt) "lkdtm: " fmt
|
In file included from include/linux/kernel.h:16,
from include/asm-generic/bug.h:20,
from ./arch/xtensa/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from include/linux/slab.h:15,
from drivers/misc/lkdtm/fortify.c:8:
include/linux/printk.h:301: note: this is the location of the previous definition
301 | #define pr_fmt(fmt) fmt
|
vim +/pr_fmt +5 drivers/misc/lkdtm/lkdtm.h
9a49a528dcf3c20 drivers/misc/lkdtm.h Kees Cook 2016-02-22 4
6d2e91a662256fd drivers/misc/lkdtm.h Kees Cook 2016-07-15 @5 #define pr_fmt(fmt) "lkdtm: " fmt
6d2e91a662256fd drivers/misc/lkdtm.h Kees Cook 2016-07-15 6
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Le mercredi 18 novembre 2020, 21:02:32 CET Kees Cook a ?crit :
> On Wed, Nov 18, 2020 at 12:07:30PM +0100, [email protected]
wrote:
> > From: Francis Laniel <[email protected]>
> >
> > This new test ensures that fortified strscpy has the same behavior than
> > vanilla strscpy (e.g. returning -E2BIG when src content is truncated).
> > Finally, it generates a crash at runtime because there is a write overflow
> > in destination string.
> >
> > Signed-off-by: Francis Laniel <[email protected]>
> > Reviewed-by: Kees Cook <[email protected]>
> > ---
> >
> > drivers/misc/lkdtm/Makefile | 1 +
> > drivers/misc/lkdtm/core.c | 1 +
> > drivers/misc/lkdtm/fortify.c | 82 +++++++++++++++++++++++++
> > drivers/misc/lkdtm/lkdtm.h | 3 +
> > tools/testing/selftests/lkdtm/tests.txt | 1 +
> > 5 files changed, 88 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 b8c51a633fcc..3c0a67f072c0 100644
> > --- a/drivers/misc/lkdtm/core.c
> > +++ b/drivers/misc/lkdtm/core.c
> > @@ -175,6 +175,7 @@ static const struct crashtype crashtypes[] = {
> >
> > CRASHTYPE(USERCOPY_KERNEL),
> > CRASHTYPE(STACKLEAK_ERASING),
> > CRASHTYPE(CFI_FORWARD_PROTO),
> >
> > + CRASHTYPE(FORTIFIED_STRSCPY),
> >
> > #ifdef CONFIG_X86_32
> >
> > CRASHTYPE(DOUBLE_FAULT),
> >
> > #endif
> >
> > diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c
> > new file mode 100644
> > index 000000000000..790d46591bf5
> > --- /dev/null
> > +++ b/drivers/misc/lkdtm/fortify.c
> > @@ -0,0 +1,82 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 Francis Laniel <[email protected]>
> > + *
> > + * Add tests related to fortified functions in this file.
> > + */
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include "lkdtm.h"
>
> Ah, I just noticed one small nit here during build testing: lkdtm.h
> needs to be included first to get the correct pr_fmt to avoid a warning:
>
> In file included from drivers/misc/lkdtm/fortify.c:9:
> drivers/misc/lkdtm/lkdtm.h:5: warning: "pr_fmt" redefined
> 5 | #define pr_fmt(fmt) "lkdtm: " fmt
>
> -Kees
This my bad, I noticed this warning but though it was "normal" with LKDTM.
I should have asked about it!
I will send the v6 soon!