[RFC][PATCH v1] Fortify string function strscpy.

[RFC][PATCH v1] Fortify string function strscpy.

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

Hi.


First, I do hope you are all fine and the same for your relatives.

This patch is related to this issue:
https://github.com/KSPP/linux/issues/96

In this patch, I fortified strscpy so the function detects write overflows to
dest.
I did not deal with read overflows since strscpy will exit when '\0' is met in
src.

Unfortunately, I did not test this modification at run time because I did not
find enough documentation about LKDTM to make it work.
Also, when I booted the modified kernel inside a VM, I had oom reaper called to
kill systemd related processes when I used len as third argument of
__real_strscpy...
For all these reasons I marked this patch as RFC.


Best regards.

Signed-off-by: Francis Laniel <laniel_francis@privacyrequired.com>
---
 include/linux/string.h | 45 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index b1f3894a0a3e..b661863619e0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -6,6 +6,8 @@
 #include <linux/compiler.h>	/* for inline */
 #include <linux/types.h>	/* for size_t */
 #include <linux/stddef.h>	/* for NULL */
+#include <linux/bug.h>		/* for WARN_ON_ONCE */
+#include <linux/errno.h>	/* for E2BIG */
 #include <stdarg.h>
 #include <uapi/linux/string.h>
 
@@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
 	return ret;
 }
 
+/* defined after fortified strlen to reuse it */
+extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
+__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t count)
+{
+	size_t len;
+	size_t p_size = __builtin_object_size(p, 0);
+	size_t q_size = __builtin_object_size(q, 0);
+	/*
+	 * If p_size and q_size cannot be known at compile time we just had to
+	 * trust this function caller.
+	 */
+	if (p_size == (size_t)-1 && q_size == (size_t)-1)
+		return __real_strscpy(p, q, count);
+	len = strlen(q);
+	if (count) {
+		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
+		if (WARN_ON_ONCE(count > INT_MAX))
+			return -E2BIG;
+		/*
+		 * strscpy handles read overflows by stop reading q when '\0' is
+		 * met.
+		 * We stick to this behavior here.
+		 */
+		len = (len >= count) ? count : len;
+		/*
+		 * If len can be known at compile time and is greater than
+		 * p_size, generate a compile time write overflow error.
+		 */
+		if (__builtin_constant_p(len) && len > p_size)
+			__write_overflow();
+		/* Otherwise generate a runtime write overflow error. */
+		if (len > p_size)
+			fortify_panic(__func__);
+		/*
+		 * Still use count as third argument to correctly compute max
+		 * inside strscpy.
+		 */
+		return __real_strscpy(p, q, count);
+	}
+	/* If count is 0, strscpy return -E2BIG. */
+	return -E2BIG;
+}
+
 /* defined after fortified strlen and strnlen to reuse them */
 __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
 {
-- 
2.20.1

[RFC][PATCH v2] Fortify string function strscpy.

[laniel_francis]

From: Francis Laniel <laniel_francis@privacyrequired.com>

Thanks to kees advices (see:
https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote a LKDTM
test for the fortified version of strscpy I added in the v1 of this patch.
The test panics due to write overflow.

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 | 37 +++++++++++++++++++++++++++++
 drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
 include/linux/string.h       | 45 ++++++++++++++++++++++++++++++++++++
 5 files changed, 94 insertions(+), 7 deletions(-)
 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 a5e344df9166..979f9e3feefd 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,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..0397d2def66d
--- /dev/null
+++ b/drivers/misc/lkdtm/fortify.c
@@ -0,0 +1,37 @@
+// 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 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)
+	char *src;
+	char dst[3];
+
+	src = kmalloc(7, GFP_KERNEL);
+	src[0] = 'f';
+	src[1] = 'o';
+	src[2] = 'o';
+	src[3] = 'b';
+	src[4] = 'a';
+	src[5] = 'r';
+	src[6] = '\0';
+
+	strscpy(dst, src, 1000);
+
+	kfree(dst);
+
+	pr_info("Fail: No overflow in above strscpy call!\n");
+#endif
+}
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index 8878538b2c13..8e5e90eb0e00 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -6,7 +6,7 @@
 
 #include <linux/kernel.h>
 
-/* lkdtm_bugs.c */
+/* bugs.c */
 void __init lkdtm_bugs_init(int *recur_param);
 void lkdtm_PANIC(void);
 void lkdtm_BUG(void);
@@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void);
 void lkdtm_DOUBLE_FAULT(void);
 void lkdtm_CORRUPT_PAC(void);
 
-/* lkdtm_heap.c */
+/* heap.c */
 void __init lkdtm_heap_init(void);
 void __exit lkdtm_heap_exit(void);
 void lkdtm_OVERWRITE_ALLOCATION(void);
@@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
 void lkdtm_SLAB_FREE_CROSS(void);
 void lkdtm_SLAB_FREE_PAGE(void);
 
-/* lkdtm_perms.c */
+/* perms.c */
 void __init lkdtm_perms_init(void);
 void lkdtm_WRITE_RO(void);
 void lkdtm_WRITE_RO_AFTER_INIT(void);
@@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void);
 void lkdtm_ACCESS_USERSPACE(void);
 void lkdtm_ACCESS_NULL(void);
 
-/* lkdtm_refcount.c */
+/* refcount.c */
 void lkdtm_REFCOUNT_INC_OVERFLOW(void);
 void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
 void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
@@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
 void lkdtm_REFCOUNT_TIMING(void);
 void lkdtm_ATOMIC_TIMING(void);
 
-/* lkdtm_rodata.c */
+/* rodata.c */
 void lkdtm_rodata_do_nothing(void);
 
-/* lkdtm_usercopy.c */
+/* usercopy.c */
 void __init lkdtm_usercopy_init(void);
 void __exit lkdtm_usercopy_exit(void);
 void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
@@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void);
 void lkdtm_USERCOPY_KERNEL(void);
 void lkdtm_USERCOPY_KERNEL_DS(void);
 
-/* lkdtm_stackleak.c */
+/* stackleak.c */
 void lkdtm_STACKLEAK_ERASING(void);
 
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
 
+/* fortify.c */
+void lkdtm_FORTIFIED_STRSCPY(void);
+
 #endif
diff --git a/include/linux/string.h b/include/linux/string.h
index b1f3894a0a3e..b661863619e0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -6,6 +6,8 @@
 #include <linux/compiler.h>	/* for inline */
 #include <linux/types.h>	/* for size_t */
 #include <linux/stddef.h>	/* for NULL */
+#include <linux/bug.h>		/* for WARN_ON_ONCE */
+#include <linux/errno.h>	/* for E2BIG */
 #include <stdarg.h>
 #include <uapi/linux/string.h>
 
@@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
 	return ret;
 }
 
+/* defined after fortified strlen to reuse it */
+extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
+__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t count)
+{
+	size_t len;
+	size_t p_size = __builtin_object_size(p, 0);
+	size_t q_size = __builtin_object_size(q, 0);
+	/*
+	 * If p_size and q_size cannot be known at compile time we just had to
+	 * trust this function caller.
+	 */
+	if (p_size == (size_t)-1 && q_size == (size_t)-1)
+		return __real_strscpy(p, q, count);
+	len = strlen(q);
+	if (count) {
+		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
+		if (WARN_ON_ONCE(count > INT_MAX))
+			return -E2BIG;
+		/*
+		 * strscpy handles read overflows by stop reading q when '\0' is
+		 * met.
+		 * We stick to this behavior here.
+		 */
+		len = (len >= count) ? count : len;
+		/*
+		 * If len can be known at compile time and is greater than
+		 * p_size, generate a compile time write overflow error.
+		 */
+		if (__builtin_constant_p(len) && len > p_size)
+			__write_overflow();
+		/* Otherwise generate a runtime write overflow error. */
+		if (len > p_size)
+			fortify_panic(__func__);
+		/*
+		 * Still use count as third argument to correctly compute max
+		 * inside strscpy.
+		 */
+		return __real_strscpy(p, q, count);
+	}
+	/* If count is 0, strscpy return -E2BIG. */
+	return -E2BIG;
+}
+
 /* defined after fortified strlen and strnlen to reuse them */
 __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
 {
-- 
2.20.1

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Kees Cook]

On Fri, Oct 16, 2020 at 02:38:09PM +0200, laniel_francis@privacyrequired.com wrote:
> From: Francis Laniel <laniel_francis@privacyrequired.com>
> 
> Thanks to kees advices (see:
> https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote a LKDTM
> test for the fortified version of strscpy I added in the v1 of this patch.
> The test panics due to write overflow.

Ah nice, thanks! I am reminded about this series as well:
https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
I think we can likely do this all at the same time, merge the
complementary pieces, etc.

Notes below...

> 
> 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 | 37 +++++++++++++++++++++++++++++
>  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------

Yay tests! These should, however, be a separate patch.

>  include/linux/string.h       | 45 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 94 insertions(+), 7 deletions(-)
>  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 a5e344df9166..979f9e3feefd 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,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..0397d2def66d
> --- /dev/null
> +++ b/drivers/misc/lkdtm/fortify.c
> @@ -0,0 +1,37 @@
> +// 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 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)
> +	char *src;
> +	char dst[3];
> +
> +	src = kmalloc(7, GFP_KERNEL);
> +	src[0] = 'f';
> +	src[1] = 'o';
> +	src[2] = 'o';
> +	src[3] = 'b';
> +	src[4] = 'a';
> +	src[5] = 'r';
> +	src[6] = '\0';

Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
using __underlying_strcpy() might be easier.

> +
> +	strscpy(dst, src, 1000);
> +
> +	kfree(dst);
> +
> +	pr_info("Fail: No overflow in above strscpy call!\n");
> +#endif
> +}

One thing I'd love to see is a _compile-time_ test too: but it needs to
be a negative failure case, which Makefiles are not well suited to
dealing with. e.g. something like:

good.o: nop.c bad.c
	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c nop.c ; fi

I'm not sure how to do it.

> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index 8878538b2c13..8e5e90eb0e00 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -6,7 +6,7 @@
>  
>  #include <linux/kernel.h>
>  
> -/* lkdtm_bugs.c */
> +/* bugs.c */

oops, yes. Can you split change from the others, since it's an unrelated
clean-up.

>  void __init lkdtm_bugs_init(int *recur_param);
>  void lkdtm_PANIC(void);
>  void lkdtm_BUG(void);
> @@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void);
>  void lkdtm_DOUBLE_FAULT(void);
>  void lkdtm_CORRUPT_PAC(void);
>  
> -/* lkdtm_heap.c */
> +/* heap.c */
>  void __init lkdtm_heap_init(void);
>  void __exit lkdtm_heap_exit(void);
>  void lkdtm_OVERWRITE_ALLOCATION(void);
> @@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
>  void lkdtm_SLAB_FREE_CROSS(void);
>  void lkdtm_SLAB_FREE_PAGE(void);
>  
> -/* lkdtm_perms.c */
> +/* perms.c */
>  void __init lkdtm_perms_init(void);
>  void lkdtm_WRITE_RO(void);
>  void lkdtm_WRITE_RO_AFTER_INIT(void);
> @@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void);
>  void lkdtm_ACCESS_USERSPACE(void);
>  void lkdtm_ACCESS_NULL(void);
>  
> -/* lkdtm_refcount.c */
> +/* refcount.c */
>  void lkdtm_REFCOUNT_INC_OVERFLOW(void);
>  void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
>  void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
> @@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
>  void lkdtm_REFCOUNT_TIMING(void);
>  void lkdtm_ATOMIC_TIMING(void);
>  
> -/* lkdtm_rodata.c */
> +/* rodata.c */
>  void lkdtm_rodata_do_nothing(void);
>  
> -/* lkdtm_usercopy.c */
> +/* usercopy.c */
>  void __init lkdtm_usercopy_init(void);
>  void __exit lkdtm_usercopy_exit(void);
>  void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
> @@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void);
>  void lkdtm_USERCOPY_KERNEL(void);
>  void lkdtm_USERCOPY_KERNEL_DS(void);
>  
> -/* lkdtm_stackleak.c */
> +/* stackleak.c */
>  void lkdtm_STACKLEAK_ERASING(void);
>  
>  /* cfi.c */
>  void lkdtm_CFI_FORWARD_PROTO(void);
>  
> +/* fortify.c */
> +void lkdtm_FORTIFIED_STRSCPY(void);
> +
>  #endif
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b1f3894a0a3e..b661863619e0 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -6,6 +6,8 @@
>  #include <linux/compiler.h>	/* for inline */
>  #include <linux/types.h>	/* for size_t */
>  #include <linux/stddef.h>	/* for NULL */
> +#include <linux/bug.h>		/* for WARN_ON_ONCE */
> +#include <linux/errno.h>	/* for E2BIG */
>  #include <stdarg.h>
>  #include <uapi/linux/string.h>
>  
> @@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
>  	return ret;
>  }
>  
> +/* defined after fortified strlen to reuse it */
> +extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
> +__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t count)

I would name "count" as "size" to match the other helpers.

> +{
> +	size_t len;
> +	size_t p_size = __builtin_object_size(p, 0);
> +	size_t q_size = __builtin_object_size(q, 0);

These can be using ", 1" instead of ", 0". And I'll grab the related
changes from the mentioned series above.

> +	/*
> +	 * If p_size and q_size cannot be known at compile time we just had to
> +	 * trust this function caller.
> +	 */
> +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
> +		return __real_strscpy(p, q, count);
> +	len = strlen(q);
> +	if (count) {

This test isn't needed; it'll work itself out correctly. :P

> +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
> +		if (WARN_ON_ONCE(count > INT_MAX))
> +			return -E2BIG;

This is already handled in strscpy, I'd drop this here.

> +		/*
> +		 * strscpy handles read overflows by stop reading q when '\0' is
> +		 * met.
> +		 * We stick to this behavior here.
> +		 */
> +		len = (len >= count) ? count : len;
> +		/*
> +		 * If len can be known at compile time and is greater than
> +		 * p_size, generate a compile time write overflow error.
> +		 */
> +		if (__builtin_constant_p(len) && len > p_size)

This won't work (len wasn't an argument and got assigned); you need:

		if (__builtin_constant_p(size) && p_size < size)


> +			__write_overflow();
> +		/* Otherwise generate a runtime write overflow error. */
> +		if (len > p_size)
> +			fortify_panic(__func__);

I think this just needs to be:

		if (p_size < size)
			fortify_panic(__func__);


> +		/*
> +		 * Still use count as third argument to correctly compute max
> +		 * inside strscpy.
> +		 */
> +		return __real_strscpy(p, q, count);
> +	}
> +	/* If count is 0, strscpy return -E2BIG. */
> +	return -E2BIG;

I'd let __real_strscpy() handle this.

> +}
> +
>  /* defined after fortified strlen and strnlen to reuse them */
>  __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
>  {
> -- 
> 2.20.1
> 

-- 
Kees Cook

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Francis Laniel]

Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit :
> On Fri, Oct 16, 2020 at 02:38:09PM +0200, laniel_francis@privacyrequired.com 
wrote:
> > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > 
> > Thanks to kees advices (see:
> > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote a
> > LKDTM test for the fortified version of strscpy I added in the v1 of this
> > patch. The test panics due to write overflow.
> 
> Ah nice, thanks! I am reminded about this series as well:
> https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
> I think we can likely do this all at the same time, merge the
> complementary pieces, etc.

You are welcome!
Just to be sure I understand correctly: you want me to add work of Daniel 
Axtens to my local version, then add my modifications on top of his work and 
republish the whole patch set?

> 
> Notes below...
> 
> > 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 | 37 +++++++++++++++++++++++++++++
> >  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
> 
> Yay tests! These should, however, be a separate patch.

Ok, I will separate it.
If I understand correctly: one semantic modification = one commit.

> 
> >  include/linux/string.h       | 45 ++++++++++++++++++++++++++++++++++++
> >  5 files changed, 94 insertions(+), 7 deletions(-)
> >  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 a5e344df9166..979f9e3feefd 100644
> > --- a/drivers/misc/lkdtm/core.c
> > +++ b/drivers/misc/lkdtm/core.c
> > @@ -178,6 +178,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..0397d2def66d
> > --- /dev/null
> > +++ b/drivers/misc/lkdtm/fortify.c
> > @@ -0,0 +1,37 @@
> > +// 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 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) +	char *src;
> > +	char dst[3];
> > +
> > +	src = kmalloc(7, GFP_KERNEL);
> > +	src[0] = 'f';
> > +	src[1] = 'o';
> > +	src[2] = 'o';
> > +	src[3] = 'b';
> > +	src[4] = 'a';
> > +	src[5] = 'r';
> > +	src[6] = '\0';
> 
> Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
> using __underlying_strcpy() might be easier.

I am sorry but I did not understand.
If we use here __underlying_strcpy() the function this will not profit from the 
protection added in fortified version of strscpy()?

> 
> > +
> > +	strscpy(dst, src, 1000);
> > +
> > +	kfree(dst);
> > +
> > +	pr_info("Fail: No overflow in above strscpy call!\n");
> > +#endif
> > +}
> 
> One thing I'd love to see is a _compile-time_ test too: but it needs to
> be a negative failure case, which Makefiles are not well suited to
> dealing with. e.g. something like:
> 
> good.o: nop.c bad.c
> 	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c
> nop.c ; fi
> 
> I'm not sure how to do it.
> 

This is a good idea, I though to it but I did not see an easy way to deal with 
it.
I will investigate one it, but I cannot guarantee the next version will come 
with this feature.

> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> > index 8878538b2c13..8e5e90eb0e00 100644
> > --- a/drivers/misc/lkdtm/lkdtm.h
> > +++ b/drivers/misc/lkdtm/lkdtm.h
> > @@ -6,7 +6,7 @@
> > 
> >  #include <linux/kernel.h>
> > 
> > -/* lkdtm_bugs.c */
> > +/* bugs.c */
> 
> oops, yes. Can you split change from the others, since it's an unrelated
> clean-up.

Understand, it will be done for next version!

> 
> >  void __init lkdtm_bugs_init(int *recur_param);
> >  void lkdtm_PANIC(void);
> >  void lkdtm_BUG(void);
> > 
> > @@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void);
> > 
> >  void lkdtm_DOUBLE_FAULT(void);
> >  void lkdtm_CORRUPT_PAC(void);
> > 
> > -/* lkdtm_heap.c */
> > +/* heap.c */
> > 
> >  void __init lkdtm_heap_init(void);
> >  void __exit lkdtm_heap_exit(void);
> >  void lkdtm_OVERWRITE_ALLOCATION(void);
> > 
> > @@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
> > 
> >  void lkdtm_SLAB_FREE_CROSS(void);
> >  void lkdtm_SLAB_FREE_PAGE(void);
> > 
> > -/* lkdtm_perms.c */
> > +/* perms.c */
> > 
> >  void __init lkdtm_perms_init(void);
> >  void lkdtm_WRITE_RO(void);
> >  void lkdtm_WRITE_RO_AFTER_INIT(void);
> > 
> > @@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void);
> > 
> >  void lkdtm_ACCESS_USERSPACE(void);
> >  void lkdtm_ACCESS_NULL(void);
> > 
> > -/* lkdtm_refcount.c */
> > +/* refcount.c */
> > 
> >  void lkdtm_REFCOUNT_INC_OVERFLOW(void);
> >  void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
> >  void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
> > 
> > @@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
> > 
> >  void lkdtm_REFCOUNT_TIMING(void);
> >  void lkdtm_ATOMIC_TIMING(void);
> > 
> > -/* lkdtm_rodata.c */
> > +/* rodata.c */
> > 
> >  void lkdtm_rodata_do_nothing(void);
> > 
> > -/* lkdtm_usercopy.c */
> > +/* usercopy.c */
> > 
> >  void __init lkdtm_usercopy_init(void);
> >  void __exit lkdtm_usercopy_exit(void);
> >  void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
> > 
> > @@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void);
> > 
> >  void lkdtm_USERCOPY_KERNEL(void);
> >  void lkdtm_USERCOPY_KERNEL_DS(void);
> > 
> > -/* lkdtm_stackleak.c */
> > +/* stackleak.c */
> > 
> >  void lkdtm_STACKLEAK_ERASING(void);
> >  
> >  /* cfi.c */
> >  void lkdtm_CFI_FORWARD_PROTO(void);
> > 
> > +/* fortify.c */
> > +void lkdtm_FORTIFIED_STRSCPY(void);
> > +
> > 
> >  #endif
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index b1f3894a0a3e..b661863619e0 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -6,6 +6,8 @@
> > 
> >  #include <linux/compiler.h>	/* for inline */
> >  #include <linux/types.h>	/* for size_t */
> >  #include <linux/stddef.h>	/* for NULL */
> > 
> > +#include <linux/bug.h>		/* for WARN_ON_ONCE */
> > +#include <linux/errno.h>	/* for E2BIG */
> > 
> >  #include <stdarg.h>
> >  #include <uapi/linux/string.h>
> > 
> > @@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char
> > *q, size_t size)> 
> >  	return ret;
> >  
> >  }
> > 
> > +/* defined after fortified strlen to reuse it */
> > +extern ssize_t __real_strscpy(char *, const char *, size_t)
> > __RENAME(strscpy); +__FORTIFY_INLINE ssize_t strscpy(char *p, const char
> > *q, size_t count)
> I would name "count" as "size" to match the other helpers.
> 

I decided to keep count because it is the argument name in unfortified version 
of strscpy
I will change the name for next version to stick with all the fortified 
functions arguments.

> > +{
> > +	size_t len;
> > +	size_t p_size = __builtin_object_size(p, 0);
> > +	size_t q_size = __builtin_object_size(q, 0);
> 
> These can be using ", 1" instead of ", 0". And I'll grab the related
> changes from the mentioned series above.
> 

I looked Daniel Axtens patch and understood why it is better to use 1 instead 
of 0 so I will add it for the next version.

> > +	/*
> > +	 * If p_size and q_size cannot be known at compile time we just had to
> > +	 * trust this function caller.
> > +	 */
> > +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
> > +		return __real_strscpy(p, q, count);
> > +	len = strlen(q);
> > +	if (count) {
> 
> This test isn't needed; it'll work itself out correctly. :P
> 

Indeed, if this condition is met, __real_strscpy will be called later.

> > +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
> > +		if (WARN_ON_ONCE(count > INT_MAX))
> > +			return -E2BIG;
> 
> This is already handled in strscpy, I'd drop this here.

I though of it at first, but since the patch modify count/size before giving it 
to __real_strscpy(), real one will never return -E2BIG due to that.
So removing this modification will lead to difference between returned value of 
fortified strscpy() and __real_strscpy().

> 
> > +		/*
> > +		 * strscpy handles read overflows by stop reading q when '\0' is
> > +		 * met.
> > +		 * We stick to this behavior here.
> > +		 */
> > +		len = (len >= count) ? count : len;
> > +		/*
> > +		 * If len can be known at compile time and is greater than
> > +		 * p_size, generate a compile time write overflow error.
> > +		 */
> > +		if (__builtin_constant_p(len) && len > p_size)
> 
> This won't work (len wasn't an argument and got assigned); you need:
> 
> 		if (__builtin_constant_p(size) && p_size < size)
> 

You are right, len is unknown at compile time... So, I will correct it for 
next version!

> > +			__write_overflow();
> > +		/* Otherwise generate a runtime write overflow error. */
> > +		if (len > p_size)
> > +			fortify_panic(__func__);
> 
> I think this just needs to be:
> 
> 		if (p_size < size)
> 			fortify_panic(__func__);
> 

I am not really sure.
If p_size is 4, size is 1000 and q is "foo\0", then what you suggested will 
panic but there is not need to panic since __real_strscpy will truncate size 
and copy just 4 bytes into p (because of '\0' in q).
Am I correct?

> > +		/*
> > +		 * Still use count as third argument to correctly compute max
> > +		 * inside strscpy.
> > +		 */
> > +		return __real_strscpy(p, q, count);
> > +	}
> > +	/* If count is 0, strscpy return -E2BIG. */
> > +	return -E2BIG;
> 
> I'd let __real_strscpy() handle this.
> 

See my three times above comment.
__real_strscpy is called only if count > 0, so it will never return -E2BIG due 
to this.
So it will lead to difference in returned value between fortified strscpy() and 
__real_strscpy().

> > +}
> > +
> > 
> >  /* defined after fortified strlen and strnlen to reuse them */
> >  __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t
> >  count) {

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Daniel Axtens]

Francis Laniel <laniel_francis@privacyrequired.com> writes:

> Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit :
>> On Fri, Oct 16, 2020 at 02:38:09PM +0200, laniel_francis@privacyrequired.com 
> wrote:
>> > From: Francis Laniel <laniel_francis@privacyrequired.com>
>> > 
>> > Thanks to kees advices (see:
>> > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote a
>> > LKDTM test for the fortified version of strscpy I added in the v1 of this
>> > patch. The test panics due to write overflow.
>> 
>> Ah nice, thanks! I am reminded about this series as well:
>> https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
>> I think we can likely do this all at the same time, merge the
>> complementary pieces, etc.
>
> You are welcome!
> Just to be sure I understand correctly: you want me to add work of Daniel 
> Axtens to my local version, then add my modifications on top of his work and 
> republish the whole patch set?

That would make sense to me: apply my patches from the list and include
them in your next patch series. If you need to modify my patches, just
add your Signed-off-by: after mine.

I don't know if my patches still apply to a current tree: let me know if
you need any help getting them up to date.

Kind regards,
Daniel

>
>> 
>> Notes below...
>> 
>> > 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 | 37 +++++++++++++++++++++++++++++
>> >  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
>> 
>> Yay tests! These should, however, be a separate patch.
>
> Ok, I will separate it.
> If I understand correctly: one semantic modification = one commit.
>
>> 
>> >  include/linux/string.h       | 45 ++++++++++++++++++++++++++++++++++++
>> >  5 files changed, 94 insertions(+), 7 deletions(-)
>> >  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 a5e344df9166..979f9e3feefd 100644
>> > --- a/drivers/misc/lkdtm/core.c
>> > +++ b/drivers/misc/lkdtm/core.c
>> > @@ -178,6 +178,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..0397d2def66d
>> > --- /dev/null
>> > +++ b/drivers/misc/lkdtm/fortify.c
>> > @@ -0,0 +1,37 @@
>> > +// 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 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) +	char *src;
>> > +	char dst[3];
>> > +
>> > +	src = kmalloc(7, GFP_KERNEL);
>> > +	src[0] = 'f';
>> > +	src[1] = 'o';
>> > +	src[2] = 'o';
>> > +	src[3] = 'b';
>> > +	src[4] = 'a';
>> > +	src[5] = 'r';
>> > +	src[6] = '\0';
>> 
>> Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
>> using __underlying_strcpy() might be easier.
>
> I am sorry but I did not understand.
> If we use here __underlying_strcpy() the function this will not profit from the 
> protection added in fortified version of strscpy()?
>
>> 
>> > +
>> > +	strscpy(dst, src, 1000);
>> > +
>> > +	kfree(dst);
>> > +
>> > +	pr_info("Fail: No overflow in above strscpy call!\n");
>> > +#endif
>> > +}
>> 
>> One thing I'd love to see is a _compile-time_ test too: but it needs to
>> be a negative failure case, which Makefiles are not well suited to
>> dealing with. e.g. something like:
>> 
>> good.o: nop.c bad.c
>> 	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c
>> nop.c ; fi
>> 
>> I'm not sure how to do it.
>> 
>
> This is a good idea, I though to it but I did not see an easy way to deal with 
> it.
> I will investigate one it, but I cannot guarantee the next version will come 
> with this feature.
>
>> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> > index 8878538b2c13..8e5e90eb0e00 100644
>> > --- a/drivers/misc/lkdtm/lkdtm.h
>> > +++ b/drivers/misc/lkdtm/lkdtm.h
>> > @@ -6,7 +6,7 @@
>> > 
>> >  #include <linux/kernel.h>
>> > 
>> > -/* lkdtm_bugs.c */
>> > +/* bugs.c */
>> 
>> oops, yes. Can you split change from the others, since it's an unrelated
>> clean-up.
>
> Understand, it will be done for next version!
>
>> 
>> >  void __init lkdtm_bugs_init(int *recur_param);
>> >  void lkdtm_PANIC(void);
>> >  void lkdtm_BUG(void);
>> > 
>> > @@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void);
>> > 
>> >  void lkdtm_DOUBLE_FAULT(void);
>> >  void lkdtm_CORRUPT_PAC(void);
>> > 
>> > -/* lkdtm_heap.c */
>> > +/* heap.c */
>> > 
>> >  void __init lkdtm_heap_init(void);
>> >  void __exit lkdtm_heap_exit(void);
>> >  void lkdtm_OVERWRITE_ALLOCATION(void);
>> > 
>> > @@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
>> > 
>> >  void lkdtm_SLAB_FREE_CROSS(void);
>> >  void lkdtm_SLAB_FREE_PAGE(void);
>> > 
>> > -/* lkdtm_perms.c */
>> > +/* perms.c */
>> > 
>> >  void __init lkdtm_perms_init(void);
>> >  void lkdtm_WRITE_RO(void);
>> >  void lkdtm_WRITE_RO_AFTER_INIT(void);
>> > 
>> > @@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void);
>> > 
>> >  void lkdtm_ACCESS_USERSPACE(void);
>> >  void lkdtm_ACCESS_NULL(void);
>> > 
>> > -/* lkdtm_refcount.c */
>> > +/* refcount.c */
>> > 
>> >  void lkdtm_REFCOUNT_INC_OVERFLOW(void);
>> >  void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
>> >  void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
>> > 
>> > @@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
>> > 
>> >  void lkdtm_REFCOUNT_TIMING(void);
>> >  void lkdtm_ATOMIC_TIMING(void);
>> > 
>> > -/* lkdtm_rodata.c */
>> > +/* rodata.c */
>> > 
>> >  void lkdtm_rodata_do_nothing(void);
>> > 
>> > -/* lkdtm_usercopy.c */
>> > +/* usercopy.c */
>> > 
>> >  void __init lkdtm_usercopy_init(void);
>> >  void __exit lkdtm_usercopy_exit(void);
>> >  void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
>> > 
>> > @@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void);
>> > 
>> >  void lkdtm_USERCOPY_KERNEL(void);
>> >  void lkdtm_USERCOPY_KERNEL_DS(void);
>> > 
>> > -/* lkdtm_stackleak.c */
>> > +/* stackleak.c */
>> > 
>> >  void lkdtm_STACKLEAK_ERASING(void);
>> >  
>> >  /* cfi.c */
>> >  void lkdtm_CFI_FORWARD_PROTO(void);
>> > 
>> > +/* fortify.c */
>> > +void lkdtm_FORTIFIED_STRSCPY(void);
>> > +
>> > 
>> >  #endif
>> > 
>> > diff --git a/include/linux/string.h b/include/linux/string.h
>> > index b1f3894a0a3e..b661863619e0 100644
>> > --- a/include/linux/string.h
>> > +++ b/include/linux/string.h
>> > @@ -6,6 +6,8 @@
>> > 
>> >  #include <linux/compiler.h>	/* for inline */
>> >  #include <linux/types.h>	/* for size_t */
>> >  #include <linux/stddef.h>	/* for NULL */
>> > 
>> > +#include <linux/bug.h>		/* for WARN_ON_ONCE */
>> > +#include <linux/errno.h>	/* for E2BIG */
>> > 
>> >  #include <stdarg.h>
>> >  #include <uapi/linux/string.h>
>> > 
>> > @@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char
>> > *q, size_t size)> 
>> >  	return ret;
>> >  
>> >  }
>> > 
>> > +/* defined after fortified strlen to reuse it */
>> > +extern ssize_t __real_strscpy(char *, const char *, size_t)
>> > __RENAME(strscpy); +__FORTIFY_INLINE ssize_t strscpy(char *p, const char
>> > *q, size_t count)
>> I would name "count" as "size" to match the other helpers.
>> 
>
> I decided to keep count because it is the argument name in unfortified version 
> of strscpy
> I will change the name for next version to stick with all the fortified 
> functions arguments.
>
>> > +{
>> > +	size_t len;
>> > +	size_t p_size = __builtin_object_size(p, 0);
>> > +	size_t q_size = __builtin_object_size(q, 0);
>> 
>> These can be using ", 1" instead of ", 0". And I'll grab the related
>> changes from the mentioned series above.
>> 
>
> I looked Daniel Axtens patch and understood why it is better to use 1 instead 
> of 0 so I will add it for the next version.
>
>> > +	/*
>> > +	 * If p_size and q_size cannot be known at compile time we just had to
>> > +	 * trust this function caller.
>> > +	 */
>> > +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
>> > +		return __real_strscpy(p, q, count);
>> > +	len = strlen(q);
>> > +	if (count) {
>> 
>> This test isn't needed; it'll work itself out correctly. :P
>> 
>
> Indeed, if this condition is met, __real_strscpy will be called later.
>
>> > +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
>> > +		if (WARN_ON_ONCE(count > INT_MAX))
>> > +			return -E2BIG;
>> 
>> This is already handled in strscpy, I'd drop this here.
>
> I though of it at first, but since the patch modify count/size before giving it 
> to __real_strscpy(), real one will never return -E2BIG due to that.
> So removing this modification will lead to difference between returned value of 
> fortified strscpy() and __real_strscpy().
>
>> 
>> > +		/*
>> > +		 * strscpy handles read overflows by stop reading q when '\0' is
>> > +		 * met.
>> > +		 * We stick to this behavior here.
>> > +		 */
>> > +		len = (len >= count) ? count : len;
>> > +		/*
>> > +		 * If len can be known at compile time and is greater than
>> > +		 * p_size, generate a compile time write overflow error.
>> > +		 */
>> > +		if (__builtin_constant_p(len) && len > p_size)
>> 
>> This won't work (len wasn't an argument and got assigned); you need:
>> 
>> 		if (__builtin_constant_p(size) && p_size < size)
>> 
>
> You are right, len is unknown at compile time... So, I will correct it for 
> next version!
>
>> > +			__write_overflow();
>> > +		/* Otherwise generate a runtime write overflow error. */
>> > +		if (len > p_size)
>> > +			fortify_panic(__func__);
>> 
>> I think this just needs to be:
>> 
>> 		if (p_size < size)
>> 			fortify_panic(__func__);
>> 
>
> I am not really sure.
> If p_size is 4, size is 1000 and q is "foo\0", then what you suggested will 
> panic but there is not need to panic since __real_strscpy will truncate size 
> and copy just 4 bytes into p (because of '\0' in q).
> Am I correct?
>
>> > +		/*
>> > +		 * Still use count as third argument to correctly compute max
>> > +		 * inside strscpy.
>> > +		 */
>> > +		return __real_strscpy(p, q, count);
>> > +	}
>> > +	/* If count is 0, strscpy return -E2BIG. */
>> > +	return -E2BIG;
>> 
>> I'd let __real_strscpy() handle this.
>> 
>
> See my three times above comment.
> __real_strscpy is called only if count > 0, so it will never return -E2BIG due 
> to this.
> So it will lead to difference in returned value between fortified strscpy() and 
> __real_strscpy().
>
>> > +}
>> > +
>> > 
>> >  /* defined after fortified strlen and strnlen to reuse them */
>> >  __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t
>> >  count) {

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Kees Cook]

On Sat, Oct 17, 2020 at 11:22:04AM +0200, Francis Laniel wrote:
> Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit :
> > On Fri, Oct 16, 2020 at 02:38:09PM +0200, laniel_francis@privacyrequired.com 
> wrote:
> > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > 
> > > Thanks to kees advices (see:
> > > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote a
> > > LKDTM test for the fortified version of strscpy I added in the v1 of this
> > > patch. The test panics due to write overflow.
> > 
> > Ah nice, thanks! I am reminded about this series as well:
> > https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
> > I think we can likely do this all at the same time, merge the
> > complementary pieces, etc.
> 
> You are welcome!
> Just to be sure I understand correctly: you want me to add work of Daniel 
> Axtens to my local version, then add my modifications on top of his work and 
> republish the whole patch set?

Yup; I would rebase his, and then have your patches follow.

> 
> > 
> > Notes below...
> > 
> > > 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 | 37 +++++++++++++++++++++++++++++
> > >  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
> > 
> > Yay tests! These should, however, be a separate patch.
> 
> Ok, I will separate it.
> If I understand correctly: one semantic modification = one commit.

Right -- I'd like to see the tests be separate. Also, probably the new
test should get added to tools/testing/selftests/lkdtm/tests.txt. I
forgot to suggest that last time!

> > > +/*
> > > + * Calls fortified strscpy to 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) +	char *src;
> > > +	char dst[3];
> > > +
> > > +	src = kmalloc(7, GFP_KERNEL);
> > > +	src[0] = 'f';
> > > +	src[1] = 'o';
> > > +	src[2] = 'o';
> > > +	src[3] = 'b';
> > > +	src[4] = 'a';
> > > +	src[5] = 'r';
> > > +	src[6] = '\0';
> > 
> > Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
> > using __underlying_strcpy() might be easier.
> 
> I am sorry but I did not understand.
> If we use here __underlying_strcpy() the function this will not profit from the 
> protection added in fortified version of strscpy()?

Sorry, I meant that instead open-coding the assignment, you could use
__underlying_strcpy(). Better yet, now that I look at it, would be:

	src = strdup("foobar", GFP_KERNEL);

Instead of the kmalloc and src[0], src[1] = ...

> > > +
> > > +	strscpy(dst, src, 1000);
> > > +
> > > +	kfree(dst);
> > > +
> > > +	pr_info("Fail: No overflow in above strscpy call!\n");
> > > +#endif
> > > +}
> > 
> > One thing I'd love to see is a _compile-time_ test too: but it needs to
> > be a negative failure case, which Makefiles are not well suited to
> > dealing with. e.g. something like:
> > 
> > good.o: nop.c bad.c
> > 	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c
> > nop.c ; fi
> > 
> > I'm not sure how to do it.
> > 
> 
> This is a good idea, I though to it but I did not see an easy way to deal with 
> it.
> I will investigate one it, but I cannot guarantee the next version will come 
> with this feature.

Yeah, this isn't required for the series; it's just me thinking out
loud. It'd be really nice to validate the fortification on the compile
side. Though, in following Linus's guidelines, it may need to be a
warning, not a hard failure of the build. Hmm.

> > > +extern ssize_t __real_strscpy(char *, const char *, size_t)
> > > __RENAME(strscpy); +__FORTIFY_INLINE ssize_t strscpy(char *p, const char
> > > *q, size_t count)
> > > +{
> > > +	size_t len;
> > > +	size_t p_size = __builtin_object_size(p, 0);
> > > +	size_t q_size = __builtin_object_size(q, 0);
> > > +	/*
> > > +	 * If p_size and q_size cannot be known at compile time we just had to
> > > +	 * trust this function caller.
> > > +	 */
> > > +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
> > > +		return __real_strscpy(p, q, count);
> > > +	len = strlen(q);

I realize again that this needs to be strnlen(q, size), I think?
Otherwise we're just repeating the flaws of strlcpy().

> > > +	if (count) {
> > 
> > This test isn't needed; it'll work itself out correctly. :P
> 
> Indeed, if this condition is met, __real_strscpy will be called later.
> 
> > > +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
> > > +		if (WARN_ON_ONCE(count > INT_MAX))
> > > +			return -E2BIG;
> > 
> > This is already handled in strscpy, I'd drop this here.
> 
> I though of it at first, but since the patch modify count/size before giving it 
> to __real_strscpy(), real one will never return -E2BIG due to that.
> So removing this modification will lead to difference between returned value of 
> fortified strscpy() and __real_strscpy().

I think if you avoid modifying size, it'll work out correctly.

> > 
> > > +		/*
> > > +		 * strscpy handles read overflows by stop reading q when '\0' is
> > > +		 * met.
> > > +		 * We stick to this behavior here.
> > > +		 */
> > > +		len = (len >= count) ? count : len;
> > > +		/*
> > > +		 * If len can be known at compile time and is greater than
> > > +		 * p_size, generate a compile time write overflow error.
> > > +		 */
> > > +		if (__builtin_constant_p(len) && len > p_size)
> > 
> > This won't work (len wasn't an argument and got assigned); you need:
> > 
> > 		if (__builtin_constant_p(size) && p_size < size)
> > 
> 
> You are right, len is unknown at compile time... So, I will correct it for 
> next version!
> 
> > > +			__write_overflow();
> > > +		/* Otherwise generate a runtime write overflow error. */
> > > +		if (len > p_size)
> > > +			fortify_panic(__func__);
> > 
> > I think this just needs to be:
> > 
> > 		if (p_size < size)
> > 			fortify_panic(__func__);
> > 
> 
> I am not really sure.
> If p_size is 4, size is 1000 and q is "foo\0", then what you suggested will 
> panic but there is not need to panic since __real_strscpy will truncate size 
> and copy just 4 bytes into p (because of '\0' in q).
> Am I correct?

Hm, so, if p_size is 4 and size is __builtin_constant_p(), and p_size
< size, it should fail to compile. (The wrong max size is proposed.)
But yes, you're right, for the runtime case, if len < p_size, we shouldn't
panic. But that means the test needs to be "len >= p_size"

> 
> > > +		/*
> > > +		 * Still use count as third argument to correctly compute max
> > > +		 * inside strscpy.
> > > +		 */
> > > +		return __real_strscpy(p, q, count);
> > > +	}
> > > +	/* If count is 0, strscpy return -E2BIG. */
> > > +	return -E2BIG;
> > 
> > I'd let __real_strscpy() handle this.
> > 
> 
> See my three times above comment.
> __real_strscpy is called only if count > 0, so it will never return -E2BIG due 
> to this.
> So it will lead to difference in returned value between fortified strscpy() and 
> __real_strscpy().

There are enough changes pending here that I'll wait for the next
version to look at this part again. Regardless, we should at least have
the tests described even if the compile-time ones can't be tested yet.

-- 
Kees Cook

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Francis Laniel]

Le lundi 19 octobre 2020, 13:51:26 CEST Daniel Axtens a écrit :
> Francis Laniel <laniel_francis@privacyrequired.com> writes:
> > Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit :
> >> On Fri, Oct 16, 2020 at 02:38:09PM +0200,
> >> laniel_francis@privacyrequired.com> 
> > wrote:
> >> > From: Francis Laniel <laniel_francis@privacyrequired.com>
> >> > 
> >> > Thanks to kees advices (see:
> >> > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote
> >> > a
> >> > LKDTM test for the fortified version of strscpy I added in the v1 of
> >> > this
> >> > patch. The test panics due to write overflow.
> >> 
> >> Ah nice, thanks! I am reminded about this series as well:
> >> https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
> >> I think we can likely do this all at the same time, merge the
> >> complementary pieces, etc.
> > 
> > You are welcome!
> > Just to be sure I understand correctly: you want me to add work of Daniel
> > Axtens to my local version, then add my modifications on top of his work
> > and republish the whole patch set?
> 
> That would make sense to me: apply my patches from the list and include
> them in your next patch series. If you need to modify my patches, just
> add your Signed-off-by: after mine.
> 
> I don't know if my patches still apply to a current tree: let me know if
> you need any help getting them up to date.
> 
> Kind regards,
> Daniel
> 

I succeeded to apply your patches but I do not think the result is correct in 
terms of semantic (your second patch contains your changes plus 
lkdtm_CORRUPT_PAC which was on my HEAD).
I will see this for the reviews of the next version.

The important is that I can work on top of your modifications.

> >> Notes below...
> >> 
> >> > 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 | 37 +++++++++++++++++++++++++++++
> >> >  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
> >> 
> >> Yay tests! These should, however, be a separate patch.
> > 
> > Ok, I will separate it.
> > If I understand correctly: one semantic modification = one commit.
> > 
> >> >  include/linux/string.h       | 45 ++++++++++++++++++++++++++++++++++++
> >> >  5 files changed, 94 insertions(+), 7 deletions(-)
> >> >  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 a5e344df9166..979f9e3feefd 100644
> >> > --- a/drivers/misc/lkdtm/core.c
> >> > +++ b/drivers/misc/lkdtm/core.c
> >> > @@ -178,6 +178,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..0397d2def66d
> >> > --- /dev/null
> >> > +++ b/drivers/misc/lkdtm/fortify.c
> >> > @@ -0,0 +1,37 @@
> >> > +// 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 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) +	char *src;
> >> > +	char dst[3];
> >> > +
> >> > +	src = kmalloc(7, GFP_KERNEL);
> >> > +	src[0] = 'f';
> >> > +	src[1] = 'o';
> >> > +	src[2] = 'o';
> >> > +	src[3] = 'b';
> >> > +	src[4] = 'a';
> >> > +	src[5] = 'r';
> >> > +	src[6] = '\0';
> >> 
> >> Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
> >> using __underlying_strcpy() might be easier.
> > 
> > I am sorry but I did not understand.
> > If we use here __underlying_strcpy() the function this will not profit
> > from the protection added in fortified version of strscpy()?
> > 
> >> > +
> >> > +	strscpy(dst, src, 1000);
> >> > +
> >> > +	kfree(dst);
> >> > +
> >> > +	pr_info("Fail: No overflow in above strscpy call!\n");
> >> > +#endif
> >> > +}
> >> 
> >> One thing I'd love to see is a _compile-time_ test too: but it needs to
> >> be a negative failure case, which Makefiles are not well suited to
> >> dealing with. e.g. something like:
> >> 
> >> good.o: nop.c bad.c
> >> 
> >> 	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c
> >> 
> >> nop.c ; fi
> >> 
> >> I'm not sure how to do it.
> > 
> > This is a good idea, I though to it but I did not see an easy way to deal
> > with it.
> > I will investigate one it, but I cannot guarantee the next version will
> > come with this feature.
> > 
> >> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> >> > index 8878538b2c13..8e5e90eb0e00 100644
> >> > --- a/drivers/misc/lkdtm/lkdtm.h
> >> > +++ b/drivers/misc/lkdtm/lkdtm.h
> >> > @@ -6,7 +6,7 @@
> >> > 
> >> >  #include <linux/kernel.h>
> >> > 
> >> > -/* lkdtm_bugs.c */
> >> > +/* bugs.c */
> >> 
> >> oops, yes. Can you split change from the others, since it's an unrelated
> >> clean-up.
> > 
> > Understand, it will be done for next version!
> > 
> >> >  void __init lkdtm_bugs_init(int *recur_param);
> >> >  void lkdtm_PANIC(void);
> >> >  void lkdtm_BUG(void);
> >> > 
> >> > @@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void);
> >> > 
> >> >  void lkdtm_DOUBLE_FAULT(void);
> >> >  void lkdtm_CORRUPT_PAC(void);
> >> > 
> >> > -/* lkdtm_heap.c */
> >> > +/* heap.c */
> >> > 
> >> >  void __init lkdtm_heap_init(void);
> >> >  void __exit lkdtm_heap_exit(void);
> >> >  void lkdtm_OVERWRITE_ALLOCATION(void);
> >> > 
> >> > @@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void);
> >> > 
> >> >  void lkdtm_SLAB_FREE_CROSS(void);
> >> >  void lkdtm_SLAB_FREE_PAGE(void);
> >> > 
> >> > -/* lkdtm_perms.c */
> >> > +/* perms.c */
> >> > 
> >> >  void __init lkdtm_perms_init(void);
> >> >  void lkdtm_WRITE_RO(void);
> >> >  void lkdtm_WRITE_RO_AFTER_INIT(void);
> >> > 
> >> > @@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void);
> >> > 
> >> >  void lkdtm_ACCESS_USERSPACE(void);
> >> >  void lkdtm_ACCESS_NULL(void);
> >> > 
> >> > -/* lkdtm_refcount.c */
> >> > +/* refcount.c */
> >> > 
> >> >  void lkdtm_REFCOUNT_INC_OVERFLOW(void);
> >> >  void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
> >> >  void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void);
> >> > 
> >> > @@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
> >> > 
> >> >  void lkdtm_REFCOUNT_TIMING(void);
> >> >  void lkdtm_ATOMIC_TIMING(void);
> >> > 
> >> > -/* lkdtm_rodata.c */
> >> > +/* rodata.c */
> >> > 
> >> >  void lkdtm_rodata_do_nothing(void);
> >> > 
> >> > -/* lkdtm_usercopy.c */
> >> > +/* usercopy.c */
> >> > 
> >> >  void __init lkdtm_usercopy_init(void);
> >> >  void __exit lkdtm_usercopy_exit(void);
> >> >  void lkdtm_USERCOPY_HEAP_SIZE_TO(void);
> >> > 
> >> > @@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void);
> >> > 
> >> >  void lkdtm_USERCOPY_KERNEL(void);
> >> >  void lkdtm_USERCOPY_KERNEL_DS(void);
> >> > 
> >> > -/* lkdtm_stackleak.c */
> >> > +/* stackleak.c */
> >> > 
> >> >  void lkdtm_STACKLEAK_ERASING(void);
> >> >  
> >> >  /* cfi.c */
> >> >  void lkdtm_CFI_FORWARD_PROTO(void);
> >> > 
> >> > +/* fortify.c */
> >> > +void lkdtm_FORTIFIED_STRSCPY(void);
> >> > +
> >> > 
> >> >  #endif
> >> > 
> >> > diff --git a/include/linux/string.h b/include/linux/string.h
> >> > index b1f3894a0a3e..b661863619e0 100644
> >> > --- a/include/linux/string.h
> >> > +++ b/include/linux/string.h
> >> > @@ -6,6 +6,8 @@
> >> > 
> >> >  #include <linux/compiler.h>	/* for inline */
> >> >  #include <linux/types.h>	/* for size_t */
> >> >  #include <linux/stddef.h>	/* for NULL */
> >> > 
> >> > +#include <linux/bug.h>		/* for WARN_ON_ONCE */
> >> > +#include <linux/errno.h>	/* for E2BIG */
> >> > 
> >> >  #include <stdarg.h>
> >> >  #include <uapi/linux/string.h>
> >> > 
> >> > @@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const
> >> > char
> >> > *q, size_t size)>
> >> > 
> >> >  	return ret;
> >> >  
> >> >  }
> >> > 
> >> > +/* defined after fortified strlen to reuse it */
> >> > +extern ssize_t __real_strscpy(char *, const char *, size_t)
> >> > __RENAME(strscpy); +__FORTIFY_INLINE ssize_t strscpy(char *p, const
> >> > char
> >> > *q, size_t count)
> >> 
> >> I would name "count" as "size" to match the other helpers.
> > 
> > I decided to keep count because it is the argument name in unfortified
> > version of strscpy
> > I will change the name for next version to stick with all the fortified
> > functions arguments.
> > 
> >> > +{
> >> > +	size_t len;
> >> > +	size_t p_size = __builtin_object_size(p, 0);
> >> > +	size_t q_size = __builtin_object_size(q, 0);
> >> 
> >> These can be using ", 1" instead of ", 0". And I'll grab the related
> >> changes from the mentioned series above.
> > 
> > I looked Daniel Axtens patch and understood why it is better to use 1
> > instead of 0 so I will add it for the next version.
> > 
> >> > +	/*
> >> > +	 * If p_size and q_size cannot be known at compile time we just had
> >> > to
> >> > +	 * trust this function caller.
> >> > +	 */
> >> > +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
> >> > +		return __real_strscpy(p, q, count);
> >> > +	len = strlen(q);
> >> > +	if (count) {
> >> 
> >> This test isn't needed; it'll work itself out correctly. :P
> > 
> > Indeed, if this condition is met, __real_strscpy will be called later.
> > 
> >> > +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
> >> > +		if (WARN_ON_ONCE(count > INT_MAX))
> >> > +			return -E2BIG;
> >> 
> >> This is already handled in strscpy, I'd drop this here.
> > 
> > I though of it at first, but since the patch modify count/size before
> > giving it to __real_strscpy(), real one will never return -E2BIG due to
> > that. So removing this modification will lead to difference between
> > returned value of fortified strscpy() and __real_strscpy().
> > 
> >> > +		/*
> >> > +		 * strscpy handles read overflows by stop reading q when '\0' is
> >> > +		 * met.
> >> > +		 * We stick to this behavior here.
> >> > +		 */
> >> > +		len = (len >= count) ? count : len;
> >> > +		/*
> >> > +		 * If len can be known at compile time and is greater than
> >> > +		 * p_size, generate a compile time write overflow error.
> >> > +		 */
> >> > +		if (__builtin_constant_p(len) && len > p_size)
> >> 
> >> This won't work (len wasn't an argument and got assigned); you need:
> >> 		if (__builtin_constant_p(size) && p_size < size)
> > 
> > You are right, len is unknown at compile time... So, I will correct it for
> > next version!
> > 
> >> > +			__write_overflow();
> >> > +		/* Otherwise generate a runtime write overflow error. */
> >> > +		if (len > p_size)
> >> > +			fortify_panic(__func__);
> >> 
> >> I think this just needs to be:
> >> 		if (p_size < size)
> >> 		
> >> 			fortify_panic(__func__);
> > 
> > I am not really sure.
> > If p_size is 4, size is 1000 and q is "foo\0", then what you suggested
> > will
> > panic but there is not need to panic since __real_strscpy will truncate
> > size and copy just 4 bytes into p (because of '\0' in q).
> > Am I correct?
> > 
> >> > +		/*
> >> > +		 * Still use count as third argument to correctly compute max
> >> > +		 * inside strscpy.
> >> > +		 */
> >> > +		return __real_strscpy(p, q, count);
> >> > +	}
> >> > +	/* If count is 0, strscpy return -E2BIG. */
> >> > +	return -E2BIG;
> >> 
> >> I'd let __real_strscpy() handle this.
> > 
> > See my three times above comment.
> > __real_strscpy is called only if count > 0, so it will never return -E2BIG
> > due to this.
> > So it will lead to difference in returned value between fortified
> > strscpy() and __real_strscpy().
> > 
> >> > +}
> >> > +
> >> > 
> >> >  /* defined after fortified strlen and strnlen to reuse them */
> >> >  __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t
> >> >  count) {

Re: [RFC][PATCH v2] Fortify string function strscpy.

[Francis Laniel]

Le mardi 20 octobre 2020, 01:19:43 CEST Kees Cook a écrit :
> On Sat, Oct 17, 2020 at 11:22:04AM +0200, Francis Laniel wrote:
> > Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit :
> > > On Fri, Oct 16, 2020 at 02:38:09PM +0200,
> > > laniel_francis@privacyrequired.com> 
> > wrote:
> > > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > > 
> > > > Thanks to kees advices (see:
> > > > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I
> > > > wrote a
> > > > LKDTM test for the fortified version of strscpy I added in the v1 of
> > > > this
> > > > patch. The test panics due to write overflow.
> > > 
> > > Ah nice, thanks! I am reminded about this series as well:
> > > https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net
> > > I think we can likely do this all at the same time, merge the
> > > complementary pieces, etc.
> > 
> > You are welcome!
> > Just to be sure I understand correctly: you want me to add work of Daniel
> > Axtens to my local version, then add my modifications on top of his work
> > and republish the whole patch set?
> 
> Yup; I would rebase his, and then have your patches follow.
> 
> > > Notes below...
> > > 
> > > > 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 | 37 +++++++++++++++++++++++++++++
> > > >  drivers/misc/lkdtm/lkdtm.h   | 17 ++++++++------
> > > 
> > > Yay tests! These should, however, be a separate patch.
> > 
> > Ok, I will separate it.
> > If I understand correctly: one semantic modification = one commit.
> 
> Right -- I'd like to see the tests be separate. Also, probably the new
> test should get added to tools/testing/selftests/lkdtm/tests.txt. I
> forgot to suggest that last time!

I separated my commit in three different for the v3.

> 
> > > > +/*
> > > > + * Calls fortified strscpy to 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) +	char *src;
> > > > +	char dst[3];
> > > > +
> > > > +	src = kmalloc(7, GFP_KERNEL);
> > > > +	src[0] = 'f';
> > > > +	src[1] = 'o';
> > > > +	src[2] = 'o';
> > > > +	src[3] = 'b';
> > > > +	src[4] = 'a';
> > > > +	src[5] = 'r';
> > > > +	src[6] = '\0';
> > > 
> > > Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if
> > > using __underlying_strcpy() might be easier.
> > 
> > I am sorry but I did not understand.
> > If we use here __underlying_strcpy() the function this will not profit
> > from the protection added in fortified version of strscpy()?
> 
> Sorry, I meant that instead open-coding the assignment, you could use
> __underlying_strcpy(). Better yet, now that I look at it, would be:
> 
> 	src = strdup("foobar", GFP_KERNEL);
> 
> Instead of the kmalloc and src[0], src[1] = ...

I did not think about strdup... It is now used in v3.

> > > > +
> > > > +	strscpy(dst, src, 1000);
> > > > +
> > > > +	kfree(dst);
> > > > +
> > > > +	pr_info("Fail: No overflow in above strscpy call!\n");
> > > > +#endif
> > > > +}
> > > 
> > > One thing I'd love to see is a _compile-time_ test too: but it needs to
> > > be a negative failure case, which Makefiles are not well suited to
> > > dealing with. e.g. something like:
> > > 
> > > good.o: nop.c bad.c
> > > 
> > > 	if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c
> > > 
> > > nop.c ; fi
> > > 
> > > I'm not sure how to do it.
> > 
> > This is a good idea, I though to it but I did not see an easy way to deal
> > with it.
> > I will investigate one it, but I cannot guarantee the next version will
> > come with this feature.
> 
> Yeah, this isn't required for the series; it's just me thinking out
> loud. It'd be really nice to validate the fortification on the compile
> side. Though, in following Linus's guidelines, it may need to be a
> warning, not a hard failure of the build. Hmm.

Should we redeclare __write_overflow() as a warning instead of an error?

> > > > +extern ssize_t __real_strscpy(char *, const char *, size_t)
> > > > __RENAME(strscpy); +__FORTIFY_INLINE ssize_t strscpy(char *p, const
> > > > char
> > > > *q, size_t count)
> > > > +{
> > > > +	size_t len;
> > > > +	size_t p_size = __builtin_object_size(p, 0);
> > > > +	size_t q_size = __builtin_object_size(q, 0);
> > > > +	/*
> > > > +	 * If p_size and q_size cannot be known at compile time we just had
> > > > to
> > > > +	 * trust this function caller.
> > > > +	 */
> > > > +	if (p_size == (size_t)-1 && q_size == (size_t)-1)
> > > > +		return __real_strscpy(p, q, count);
> > > > +	len = strlen(q);
> 
> I realize again that this needs to be strnlen(q, size), I think?
> Otherwise we're just repeating the flaws of strlcpy().

Using strnlen would save us when src does not contain '\0' so I replaced 
strlen by strnlen for v3.

> > > > +	if (count) {
> > > 
> > > This test isn't needed; it'll work itself out correctly. :P
> > 
> > Indeed, if this condition is met, __real_strscpy will be called later.
> > 
> > > > +		/* If count is bigger than INT_MAX, strscpy returns -E2BIG. */
> > > > +		if (WARN_ON_ONCE(count > INT_MAX))
> > > > +			return -E2BIG;
> > > 
> > > This is already handled in strscpy, I'd drop this here.
> > 
> > I though of it at first, but since the patch modify count/size before
> > giving it to __real_strscpy(), real one will never return -E2BIG due to
> > that. So removing this modification will lead to difference between
> > returned value of fortified strscpy() and __real_strscpy().
> 
> I think if you avoid modifying size, it'll work out correctly.

I removed all about modifying size for v3.
When I implemented that I though "you should recompute size here because 
strscpy can save you because it exits when '\0' is met".
Then I though about it and changed my mind because we need safety inside 
fortified version of strscpy and the maximum safety.

> > > > +		/*
> > > > +		 * strscpy handles read overflows by stop reading q when '\0' is
> > > > +		 * met.
> > > > +		 * We stick to this behavior here.
> > > > +		 */
> > > > +		len = (len >= count) ? count : len;
> > > > +		/*
> > > > +		 * If len can be known at compile time and is greater than
> > > > +		 * p_size, generate a compile time write overflow error.
> > > > +		 */
> > > > +		if (__builtin_constant_p(len) && len > p_size)
> > > 
> > > This won't work (len wasn't an argument and got assigned); you need:
> > > 		if (__builtin_constant_p(size) && p_size < size)
> > 
> > You are right, len is unknown at compile time... So, I will correct it for
> > next version!
> > 
> > > > +			__write_overflow();
> > > > +		/* Otherwise generate a runtime write overflow error. */
> > > > +		if (len > p_size)
> > > > +			fortify_panic(__func__);
> > > 
> > > I think this just needs to be:
> > > 		if (p_size < size)
> > > 		
> > > 			fortify_panic(__func__);
> > 
> > I am not really sure.
> > If p_size is 4, size is 1000 and q is "foo\0", then what you suggested
> > will
> > panic but there is not need to panic since __real_strscpy will truncate
> > size and copy just 4 bytes into p (because of '\0' in q).
> > Am I correct?
> 
> Hm, so, if p_size is 4 and size is __builtin_constant_p(), and p_size
> < size, it should fail to compile. (The wrong max size is proposed.)
> But yes, you're right, for the runtime case, if len < p_size, we shouldn't
> panic. But that means the test needs to be "len >= p_size"

I do not agree on the fact that the condition should be "len >= p_size".
Indeed, there is not write overflow when len equals p_size, src can just be 
truncated when written to dst.
When len is strictly greater than p_size, there is, for sure, a write overflow.

> > > > +		/*
> > > > +		 * Still use count as third argument to correctly compute max
> > > > +		 * inside strscpy.
> > > > +		 */
> > > > +		return __real_strscpy(p, q, count);
> > > > +	}
> > > > +	/* If count is 0, strscpy return -E2BIG. */
> > > > +	return -E2BIG;
> > > 
> > > I'd let __real_strscpy() handle this.
> > 
> > See my three times above comment.
> > __real_strscpy is called only if count > 0, so it will never return -E2BIG
> > due to this.
> > So it will lead to difference in returned value between fortified
> > strscpy() and __real_strscpy().
> 
> There are enough changes pending here that I'll wait for the next
> version to look at this part again. Regardless, we should at least have
> the tests described even if the compile-time ones can't be tested yet.

Normally the review for the next version should be easier.