[PATCH 1/3] compiler.h: define __do_not_initialize

[PATCH 1/3] compiler.h: define __do_not_initialize

[glider]

For CONFIG_INIT_STACK_ALL it's sometimes handy to disable
force-initialization for a local variable, if it is known to be initialized
later on before the first use. This can be done by using the
__do_not_initialize macro.

__do_not_initialize should be applied carefully, as future changes to
the code around the local variable may introduce paths on which the
variable remains uninitialized before the use.

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 include/linux/compiler-clang.h | 10 ++++++++++
 include/linux/compiler_types.h |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 333a6695a918c..9204334d39261 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -24,6 +24,16 @@
 #define __no_sanitize_address
 #endif
 
+/*
+ * Disable initialization of a local variable when building with
+ * CONFIG_INIT_STACK_ALL.
+ */
+#ifdef CONFIG_INIT_STACK_ALL
+#define __do_not_initialize __attribute__((uninitialized))
+#else
+#define __do_not_initialize
+#endif
+
 /*
  * Not all versions of clang implement the the type-generic versions
  * of the builtin overflow checkers. Fortunately, clang implements
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 72393a8c1a6c5..b216beb5586fc 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -180,6 +180,10 @@ struct ftrace_likely_data {
 
 #endif /* __ASSEMBLY__ */
 
+#ifndef __do_not_initialize
+#define __do_not_initialize
+#endif
+
 /*
  * The below symbols may be defined for one or more, but not ALL, of the above
  * compilers. We don't consider that to be an error, so set them to nothing.
-- 
2.25.0.265.gbab2e86ba0-goog

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[glider]

Certain copy_from_user() invocations in binder.c are known to
unconditionally initialize locals before their first use, like e.g. in
the following case:

	struct binder_transaction_data tr;
	if (copy_from_user(&tr, ptr, sizeof(tr)))
		return -EFAULT;

In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
redundant locals initialization that the compiler fails to remove.
To work around this problem till Clang can deal with it, we apply
__do_not_initialize to local Binder structures.

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 drivers/android/binder.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a6b2082c24f8f..3c91d842ac704 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3788,7 +3788,7 @@ static int binder_thread_write(struct binder_proc *proc,
 
 		case BC_TRANSACTION_SG:
 		case BC_REPLY_SG: {
-			struct binder_transaction_data_sg tr;
+			struct binder_transaction_data_sg tr __do_not_initialize;
 
 			if (copy_from_user(&tr, ptr, sizeof(tr)))
 				return -EFAULT;
@@ -3799,7 +3799,7 @@ static int binder_thread_write(struct binder_proc *proc,
 		}
 		case BC_TRANSACTION:
 		case BC_REPLY: {
-			struct binder_transaction_data tr;
+			struct binder_transaction_data tr __do_not_initialize;
 
 			if (copy_from_user(&tr, ptr, sizeof(tr)))
 				return -EFAULT;
@@ -4827,7 +4827,7 @@ static int binder_ioctl_write_read(struct file *filp,
 	struct binder_proc *proc = filp->private_data;
 	unsigned int size = _IOC_SIZE(cmd);
 	void __user *ubuf = (void __user *)arg;
-	struct binder_write_read bwr;
+	struct binder_write_read bwr __do_not_initialize;
 
 	if (size != sizeof(struct binder_write_read)) {
 		ret = -EINVAL;
@@ -5039,7 +5039,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		break;
 	}
 	case BINDER_SET_CONTEXT_MGR_EXT: {
-		struct flat_binder_object fbo;
+		struct flat_binder_object fbo __do_not_initialize;
 
 		if (copy_from_user(&fbo, ubuf, sizeof(fbo))) {
 			ret = -EINVAL;
@@ -5076,7 +5076,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		break;
 	}
 	case BINDER_GET_NODE_INFO_FOR_REF: {
-		struct binder_node_info_for_ref info;
+		struct binder_node_info_for_ref info __do_not_initialize;
 
 		if (copy_from_user(&info, ubuf, sizeof(info))) {
 			ret = -EFAULT;
@@ -5095,7 +5095,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		break;
 	}
 	case BINDER_GET_NODE_DEBUG_INFO: {
-		struct binder_node_debug_info info;
+		struct binder_node_debug_info info __do_not_initialize;
 
 		if (copy_from_user(&info, ubuf, sizeof(info))) {
 			ret = -EFAULT;
-- 
2.25.0.265.gbab2e86ba0-goog

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] sched/wait: avoid double initialization in ___wait_event()

[glider]

With CONFIG_INIT_STACK_ALL enabled, the local __wq_entry is initialized
twice. Because Clang is currently unable to optimize the automatic
initialization away (init_wait_entry() is defined in another translation
unit), remove it with the __do_not_initialize annotation.

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 include/linux/wait.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d021377..03b831ee9b64a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -262,7 +262,8 @@ extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
 #define ___wait_event(wq_head, condition, state, exclusive, ret, cmd)		\
 ({										\
 	__label__ __out;							\
-	struct wait_queue_entry __wq_entry;					\
+	/* Unconditionally initialized by init_wait_entry(). */			\
+	struct wait_queue_entry __wq_entry __do_not_initialize;			\
 	long __ret = ret;	/* explicit shadow */				\
 										\
 	init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE : 0);	\
-- 
2.25.0.265.gbab2e86ba0-goog

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] compiler.h: define __do_not_initialize

[Kees Cook]

On Mon, Feb 24, 2020 at 04:34:59PM +0100, glider@google.com wrote:
> For CONFIG_INIT_STACK_ALL it's sometimes handy to disable
> force-initialization for a local variable, if it is known to be initialized
> later on before the first use. This can be done by using the
> __do_not_initialize macro.

Nit-pick: other things are listed as __no_$feature. What about naming
this __no_initialize (or reuse the attribute name of __uninitialized)?

> __do_not_initialize should be applied carefully, as future changes to
> the code around the local variable may introduce paths on which the
> variable remains uninitialized before the use.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>

Please CC lkml as well in the future.

But yes, this seems like a reasonable work-around until compilers can be
taught which functions are considered initialization sinks. :)

-Kees

> ---
>  include/linux/compiler-clang.h | 10 ++++++++++
>  include/linux/compiler_types.h |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 333a6695a918c..9204334d39261 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -24,6 +24,16 @@
>  #define __no_sanitize_address
>  #endif
>  
> +/*
> + * Disable initialization of a local variable when building with
> + * CONFIG_INIT_STACK_ALL.
> + */
> +#ifdef CONFIG_INIT_STACK_ALL
> +#define __do_not_initialize __attribute__((uninitialized))
> +#else
> +#define __do_not_initialize
> +#endif
> +
>  /*
>   * Not all versions of clang implement the the type-generic versions
>   * of the builtin overflow checkers. Fortunately, clang implements
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 72393a8c1a6c5..b216beb5586fc 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -180,6 +180,10 @@ struct ftrace_likely_data {
>  
>  #endif /* __ASSEMBLY__ */
>  
> +#ifndef __do_not_initialize
> +#define __do_not_initialize
> +#endif
> +
>  /*
>   * The below symbols may be defined for one or more, but not ALL, of the above
>   * compilers. We don't consider that to be an error, so set them to nothing.
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 

-- 
Kees Cook
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Kees Cook]

On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> Certain copy_from_user() invocations in binder.c are known to
> unconditionally initialize locals before their first use, like e.g. in
> the following case:
> 
> 	struct binder_transaction_data tr;
> 	if (copy_from_user(&tr, ptr, sizeof(tr)))
> 		return -EFAULT;
> 
> In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> redundant locals initialization that the compiler fails to remove.
> To work around this problem till Clang can deal with it, we apply
> __do_not_initialize to local Binder structures.

It should be possible to write a Coccinelle script to identify all these
cases. (i.e. a single path from struct declaration to copy_from_user())
and apply the changes automatically. This script could be checked into
scripts/coccinelle/ to help keep these markings in sync...

-Kees

> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  drivers/android/binder.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index a6b2082c24f8f..3c91d842ac704 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3788,7 +3788,7 @@ static int binder_thread_write(struct binder_proc *proc,
>  
>  		case BC_TRANSACTION_SG:
>  		case BC_REPLY_SG: {
> -			struct binder_transaction_data_sg tr;
> +			struct binder_transaction_data_sg tr __do_not_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))
>  				return -EFAULT;
> @@ -3799,7 +3799,7 @@ static int binder_thread_write(struct binder_proc *proc,
>  		}
>  		case BC_TRANSACTION:
>  		case BC_REPLY: {
> -			struct binder_transaction_data tr;
> +			struct binder_transaction_data tr __do_not_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))
>  				return -EFAULT;
> @@ -4827,7 +4827,7 @@ static int binder_ioctl_write_read(struct file *filp,
>  	struct binder_proc *proc = filp->private_data;
>  	unsigned int size = _IOC_SIZE(cmd);
>  	void __user *ubuf = (void __user *)arg;
> -	struct binder_write_read bwr;
> +	struct binder_write_read bwr __do_not_initialize;
>  
>  	if (size != sizeof(struct binder_write_read)) {
>  		ret = -EINVAL;
> @@ -5039,7 +5039,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		break;
>  	}
>  	case BINDER_SET_CONTEXT_MGR_EXT: {
> -		struct flat_binder_object fbo;
> +		struct flat_binder_object fbo __do_not_initialize;
>  
>  		if (copy_from_user(&fbo, ubuf, sizeof(fbo))) {
>  			ret = -EINVAL;
> @@ -5076,7 +5076,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		break;
>  	}
>  	case BINDER_GET_NODE_INFO_FOR_REF: {
> -		struct binder_node_info_for_ref info;
> +		struct binder_node_info_for_ref info __do_not_initialize;
>  
>  		if (copy_from_user(&info, ubuf, sizeof(info))) {
>  			ret = -EFAULT;
> @@ -5095,7 +5095,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		break;
>  	}
>  	case BINDER_GET_NODE_DEBUG_INFO: {
> -		struct binder_node_debug_info info;
> +		struct binder_node_debug_info info __do_not_initialize;
>  
>  		if (copy_from_user(&info, ubuf, sizeof(info))) {
>  			ret = -EFAULT;
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 

-- 
Kees Cook
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Jann Horn]

On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@chromium.org> wrote:
> On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> > Certain copy_from_user() invocations in binder.c are known to
> > unconditionally initialize locals before their first use, like e.g. in
> > the following case:
> >
> >       struct binder_transaction_data tr;
> >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> >               return -EFAULT;
> >
> > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > redundant locals initialization that the compiler fails to remove.
> > To work around this problem till Clang can deal with it, we apply
> > __do_not_initialize to local Binder structures.
>
> It should be possible to write a Coccinelle script to identify all these
> cases. (i.e. a single path from struct declaration to copy_from_user())
> and apply the changes automatically. This script could be checked into
> scripts/coccinelle/ to help keep these markings in sync...

I wonder whether it would instead be reasonable to define a helper
macro for "copy object from userspace to the kernel", and then use
this macro. Something like this:

#define copy_object_from_user(objp, src) ({              \
  __attribute__((uninitialized)) typeof(*(objp)) __buf; \
  copy_from_user(&__buf, (src), sizeof(*(objp)));        \
  *(objp) = __buf;                                       \
})

void blah(unsigned long user_addr) {
  struct foo stackobj;
  copy_object_from_user(&stackobj, user_addr);
  do_stuff(&stackobj);
}

Unfortunately, clang runs a MemCpy optimization pass before the Dead
Store Elimination pass, which makes the copy_from_user() go directly
to `stackobj` instead of __buf before DSE has had a chance to get rid
of the first memcpy(). Grrr...

But looking at the list of passes that LLVM runs, we can see that
between the MemCpy optimization and the DSE pass, we have Bit-Tracking
Dead Store Elimination... okay, fine, so we hack together some code
such that it contains a fake branch that is only resolved between
MemCpy and DSE:

unsigned long blub, blab;
unsigned long get_blub(void) { return blub & 5; }
#define copy_object_from_user(objp, src) ({              \
  __attribute__((uninitialized)) typeof(*(objp)) __buf; \
  *(char*)&__buf = 0;                                   \
  copy_from_user(&__buf, (src), sizeof(*(objp)));        \
  int x = get_blub(); int y = blab & 10;\
  if ((x & y) == 0) { \
    *(objp) = __buf;                                       \
  } \
})

But it still doesn't work, even though the IR looks like DSE ought to work:

*** IR Dump After Value Propagation ***
; Function Attrs: nounwind uwtable
define dso_local void @blah(i64) local_unnamed_addr #1 {
  %2 = alloca %struct.foo, align 4
  %3 = alloca %struct.foo, align 4
  %4 = bitcast %struct.foo* %2 to i8*
  call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %4) #4
  call void @llvm.memset.p0i8.i64(i8* nonnull align 4 %4, i8 -86, i64
1008, i1 false)
  %5 = bitcast %struct.foo* %3 to i8*
  call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %5) #4
  store i8 0, i8* %5, align 4, !tbaa !6
  call void @copy_from_user(i8* nonnull %5, i64 %0, i64 1008) #4
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 4 %4, i8*
nonnull align 4 %5, i64 1008, i1 false), !tbaa.struct !7
  call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %5) #4
  call void @do_stuff(%struct.foo* nonnull %2) #4
  call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %4) #4
  ret void
}
*** IR Dump After Dead Store Elimination ***
; Function Attrs: nounwind uwtable
define dso_local void @blah(i64) local_unnamed_addr #1 {
  %2 = alloca %struct.foo, align 4
  %3 = alloca %struct.foo, align 4
  %4 = bitcast %struct.foo* %2 to i8*
  call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %4) #4
  call void @llvm.memset.p0i8.i64(i8* nonnull align 4 %4, i8 -86, i64
1008, i1 false)
  %5 = bitcast %struct.foo* %3 to i8*
  call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %5) #4
  store i8 0, i8* %5, align 4, !tbaa !6
  call void @copy_from_user(i8* nonnull %5, i64 %0, i64 1008) #4
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 4 %4, i8*
nonnull align 4 %5, i64 1008, i1 false), !tbaa.struct !7
  call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %5) #4
  call void @do_stuff(%struct.foo* nonnull %2) #4
  call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %4) #4
  ret void
}

I guess maybe clang can't do DSE past a function call or something?

We also can't trick the DSE pass using an empty "asm volatile" with an
output-only memory operand, because the DSE pass can't optimize inline
asm.

Is there really no way to somehow hack this together inside a macro? :(
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] compiler.h: define __do_not_initialize

[Alexander Potapenko]

On Tue, Feb 25, 2020 at 5:16 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Feb 24, 2020 at 04:34:59PM +0100, glider@google.com wrote:
> > For CONFIG_INIT_STACK_ALL it's sometimes handy to disable
> > force-initialization for a local variable, if it is known to be initialized
> > later on before the first use. This can be done by using the
> > __do_not_initialize macro.
>
> Nit-pick: other things are listed as __no_$feature. What about naming
> this __no_initialize (or reuse the attribute name of __uninitialized)?
I agree that __no_initialize is more consistent (and also shorter than
__do_not_initialize).
Let's stick with this name.

> Please CC lkml as well in the future.
Ok, will do.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Alexander Potapenko]

On Tue, Feb 25, 2020 at 9:14 AM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@chromium.org> wrote:
> > On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> > > Certain copy_from_user() invocations in binder.c are known to
> > > unconditionally initialize locals before their first use, like e.g. in
> > > the following case:
> > >
> > >       struct binder_transaction_data tr;
> > >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > >               return -EFAULT;
> > >
> > > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > > redundant locals initialization that the compiler fails to remove.
> > > To work around this problem till Clang can deal with it, we apply
> > > __do_not_initialize to local Binder structures.
> >
> > It should be possible to write a Coccinelle script to identify all these
> > cases. (i.e. a single path from struct declaration to copy_from_user())
> > and apply the changes automatically. This script could be checked into
> > scripts/coccinelle/ to help keep these markings in sync...
>
> I wonder whether it would instead be reasonable to define a helper
> macro for "copy object from userspace to the kernel", and then use
> this macro. Something like this:

I really like this idea, but I think hacking something based on the
order of LLVM passes is too fragile and can also lead to suboptimal
code generation with GCC.
Maybe we can pull the variable declaration together with
__attribute__((uninitialized)) inside this macro instead?

> #define copy_object_from_user(objp, src) ({              \
>   __attribute__((uninitialized)) typeof(*(objp)) __buf; \
>   copy_from_user(&__buf, (src), sizeof(*(objp)));        \
>   *(objp) = __buf;                                       \
> })
>
> void blah(unsigned long user_addr) {
>   struct foo stackobj;
>   copy_object_from_user(&stackobj, user_addr);
>   do_stuff(&stackobj);
> }
>
> Unfortunately, clang runs a MemCpy optimization pass before the Dead
> Store Elimination pass, which makes the copy_from_user() go directly
> to `stackobj` instead of __buf before DSE has had a chance to get rid
> of the first memcpy(). Grrr...
>
> But looking at the list of passes that LLVM runs, we can see that
> between the MemCpy optimization and the DSE pass, we have Bit-Tracking
> Dead Store Elimination... okay, fine, so we hack together some code
> such that it contains a fake branch that is only resolved between
> MemCpy and DSE:
>
> unsigned long blub, blab;
> unsigned long get_blub(void) { return blub & 5; }
> #define copy_object_from_user(objp, src) ({              \
>   __attribute__((uninitialized)) typeof(*(objp)) __buf; \
>   *(char*)&__buf = 0;                                   \
>   copy_from_user(&__buf, (src), sizeof(*(objp)));        \
>   int x = get_blub(); int y = blab & 10;\
>   if ((x & y) == 0) { \
>     *(objp) = __buf;                                       \
>   } \
> })
>
> But it still doesn't work, even though the IR looks like DSE ought to work:
>
> *** IR Dump After Value Propagation ***
> ; Function Attrs: nounwind uwtable
> define dso_local void @blah(i64) local_unnamed_addr #1 {
>   %2 = alloca %struct.foo, align 4
>   %3 = alloca %struct.foo, align 4
>   %4 = bitcast %struct.foo* %2 to i8*
>   call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %4) #4
>   call void @llvm.memset.p0i8.i64(i8* nonnull align 4 %4, i8 -86, i64
> 1008, i1 false)
>   %5 = bitcast %struct.foo* %3 to i8*
>   call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %5) #4
>   store i8 0, i8* %5, align 4, !tbaa !6
>   call void @copy_from_user(i8* nonnull %5, i64 %0, i64 1008) #4
>   call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 4 %4, i8*
> nonnull align 4 %5, i64 1008, i1 false), !tbaa.struct !7
>   call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %5) #4
>   call void @do_stuff(%struct.foo* nonnull %2) #4
>   call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %4) #4
>   ret void
> }
> *** IR Dump After Dead Store Elimination ***
> ; Function Attrs: nounwind uwtable
> define dso_local void @blah(i64) local_unnamed_addr #1 {
>   %2 = alloca %struct.foo, align 4
>   %3 = alloca %struct.foo, align 4
>   %4 = bitcast %struct.foo* %2 to i8*
>   call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %4) #4
>   call void @llvm.memset.p0i8.i64(i8* nonnull align 4 %4, i8 -86, i64
> 1008, i1 false)
>   %5 = bitcast %struct.foo* %3 to i8*
>   call void @llvm.lifetime.start.p0i8(i64 1008, i8* nonnull %5) #4
>   store i8 0, i8* %5, align 4, !tbaa !6
>   call void @copy_from_user(i8* nonnull %5, i64 %0, i64 1008) #4
>   call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 4 %4, i8*
> nonnull align 4 %5, i64 1008, i1 false), !tbaa.struct !7
>   call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %5) #4
>   call void @do_stuff(%struct.foo* nonnull %2) #4
>   call void @llvm.lifetime.end.p0i8(i64 1008, i8* nonnull %4) #4
>   ret void
> }
>

> I guess maybe clang can't do DSE past a function call or something?

DSE only works within a basic block, which might actually be a problem
with the real copy_from_user(), which expands to a call to
_copy_from_user() and a branch.


> We also can't trick the DSE pass using an empty "asm volatile" with an
> output-only memory operand, because the DSE pass can't optimize inline
> asm.

This can be handled by something like https://reviews.llvm.org/D74853
(that's a WIP, still breaks the kernel)
Not sure though, how good this is compared to a function attribute.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Alexander Potapenko]

On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> > Certain copy_from_user() invocations in binder.c are known to
> > unconditionally initialize locals before their first use, like e.g. in
> > the following case:
> >
> >       struct binder_transaction_data tr;
> >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> >               return -EFAULT;
> >
> > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > redundant locals initialization that the compiler fails to remove.
> > To work around this problem till Clang can deal with it, we apply
> > __do_not_initialize to local Binder structures.
>
> It should be possible to write a Coccinelle script to identify all these
> cases. (i.e. a single path from struct declaration to copy_from_user())
> and apply the changes automatically. This script could be checked into
> scripts/coccinelle/ to help keep these markings in sync...

The following script:

=================================
@local_inited_by_cfu@
attribute name __no_initialize;
identifier var;
type T;
statement stmt;
@@
T var
+__no_initialize
;
if (copy_from_user(&var,..., sizeof(var))) stmt
=================================

seems to do the job, but this transformation isn't idempotent: it
inserts __no_initialize every time Coccinelle is invoked.
It's hard to work around this problem, as attributes may only be parts
of +-lines (i.e. I cannot write a rule that matches "T var
__no_initialize")
Linux kernel coccinelle scripts don't really contain good examples of
adding attributes, and probably the spatch version used by most
maintainers doesn't support them :(
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Alexander Potapenko]

On Tue, Feb 25, 2020 at 4:24 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> > > Certain copy_from_user() invocations in binder.c are known to
> > > unconditionally initialize locals before their first use, like e.g. in
> > > the following case:
> > >
> > >       struct binder_transaction_data tr;
> > >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > >               return -EFAULT;
> > >
> > > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > > redundant locals initialization that the compiler fails to remove.
> > > To work around this problem till Clang can deal with it, we apply
> > > __do_not_initialize to local Binder structures.
> >
> > It should be possible to write a Coccinelle script to identify all these
> > cases. (i.e. a single path from struct declaration to copy_from_user())
> > and apply the changes automatically. This script could be checked into
> > scripts/coccinelle/ to help keep these markings in sync...
>
> The following script:
>
> =================================
> @local_inited_by_cfu@
> attribute name __no_initialize;
> identifier var;
> type T;
> statement stmt;
> @@
> T var
> +__no_initialize
> ;
> if (copy_from_user(&var,..., sizeof(var))) stmt
> =================================
>
> seems to do the job, but this transformation isn't idempotent: it
> inserts __no_initialize every time Coccinelle is invoked.
> It's hard to work around this problem, as attributes may only be parts
> of +-lines (i.e. I cannot write a rule that matches "T var
> __no_initialize")

This one:

============================
@match@
type T;
identifier var;
statement stmt;
fresh identifier var_noinit = var##" __no_initialize";
@@
-T var;
+T var_noinit;
...
if (copy_from_user(&var,..., sizeof(var))) stmt
============================
is better, because it:
 - uses a "fresh identifier" hack instead of dealing with attributes
(which aren't supported by spatch 1.0.4)
 - seems to be idempotent because of that hack.

I'll regenerate the binder patch using that script and will look into
enhancing it and committing it to scripts/coccinelle.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] binder: do not initialize locals passed to copy_from_user()

[Kees Cook]

On Wed, Feb 26, 2020 at 03:58:41PM +0100, Alexander Potapenko wrote:
> On Tue, Feb 25, 2020 at 4:24 PM Alexander Potapenko <glider@google.com> wrote:
> >
> > On Tue, Feb 25, 2020 at 5:18 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@google.com wrote:
> > > > Certain copy_from_user() invocations in binder.c are known to
> > > > unconditionally initialize locals before their first use, like e.g. in
> > > > the following case:
> > > >
> > > >       struct binder_transaction_data tr;
> > > >       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > >               return -EFAULT;
> > > >
> > > > In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> > > > redundant locals initialization that the compiler fails to remove.
> > > > To work around this problem till Clang can deal with it, we apply
> > > > __do_not_initialize to local Binder structures.
> > >
> > > It should be possible to write a Coccinelle script to identify all these
> > > cases. (i.e. a single path from struct declaration to copy_from_user())
> > > and apply the changes automatically. This script could be checked into
> > > scripts/coccinelle/ to help keep these markings in sync...
> >
> > The following script:
> >
> > =================================
> > @local_inited_by_cfu@
> > attribute name __no_initialize;
> > identifier var;
> > type T;
> > statement stmt;
> > @@
> > T var
> > +__no_initialize
> > ;
> > if (copy_from_user(&var,..., sizeof(var))) stmt
> > =================================
> >
> > seems to do the job, but this transformation isn't idempotent: it
> > inserts __no_initialize every time Coccinelle is invoked.
> > It's hard to work around this problem, as attributes may only be parts
> > of +-lines (i.e. I cannot write a rule that matches "T var
> > __no_initialize")
> 
> This one:
> 
> ============================
> @match@
> type T;
> identifier var;
> statement stmt;
> fresh identifier var_noinit = var##" __no_initialize";
> @@
> -T var;
> +T var_noinit;
> ...
> if (copy_from_user(&var,..., sizeof(var))) stmt
> ============================
> is better, because it:
>  - uses a "fresh identifier" hack instead of dealing with attributes
> (which aren't supported by spatch 1.0.4)
>  - seems to be idempotent because of that hack.
> 
> I'll regenerate the binder patch using that script and will look into
> enhancing it and committing it to scripts/coccinelle.

Cool; sounds good!

-- 
Kees Cook
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel