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

[Francis Laniel <laniel_francis@privacyrequired.com>]

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) {