[PATCH v2 1/3] compiler.h: define __no_initialize

[PATCH v2 1/3] compiler.h: define __no_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
__no_initialize macro.

__no_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>

---

v2:
 - changed __do_not_initialize to __no_initialize as requested by Kees
   Cook
---
 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..27f774b27b061 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 __no_initialize __attribute__((uninitialized))
+#else
+#define __no_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..0208699c855af 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -216,6 +216,10 @@ struct ftrace_likely_data {
 # define __no_fgcse
 #endif
 
+#ifndef __no_initialize
+#define __no_initialize
+#endif
+
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 1/3] compiler.h: define __no_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
__no_initialize macro.

__no_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>

---

v2:
 - changed __do_not_initialize to __no_initialize as requested by Kees
   Cook
---
 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..27f774b27b061 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 __no_initialize __attribute__((uninitialized))
+#else
+#define __no_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..0208699c855af 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -216,6 +216,10 @@ struct ftrace_likely_data {
 # define __no_fgcse
 #endif
 
+#ifndef __no_initialize
+#define __no_initialize
+#endif
+
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
-- 
2.25.0.265.gbab2e86ba0-goog

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

[PATCH v2 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
__no_initialize to local Binder structures.

This patch was generated using the following Coccinelle script:

  @match@
  type T;
  identifier var;
  position p0, p1;
  @@
  T var@p0;
  ...
  copy_from_user(&var,..., sizeof(var))@p1

  @escapes depends on match@
  type match.T;
  identifier match.var;
  position match.p0,match.p1;
  @@
  T var@p0;
  ... var ...
  copy_from_user(&var,..., sizeof(var))@p1

  @local_inited_by_cfu depends on !escapes@
  type T;
  identifier var;
  position match.p0,match.p1;
  fresh identifier var_noinit = var##" __no_initialize";
  @@
  -T var@p0;
  +T var_noinit;
  ...
  copy_from_user(&var,..., sizeof(var))@p1

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

---
 v2:
  - changed __do_not_initialize to __no_initialize as requested by Kees
    Cook
  - wrote a Coccinelle script to generate the patch
---
 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..a59871532ff6b 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 __no_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 __no_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 __no_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 __no_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 __no_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 __no_initialize;
 
 		if (copy_from_user(&info, ubuf, sizeof(info))) {
 			ret = -EFAULT;
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 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
__no_initialize to local Binder structures.

This patch was generated using the following Coccinelle script:

  @match@
  type T;
  identifier var;
  position p0, p1;
  @@
  T var@p0;
  ...
  copy_from_user(&var,..., sizeof(var))@p1

  @escapes depends on match@
  type match.T;
  identifier match.var;
  position match.p0,match.p1;
  @@
  T var@p0;
  ... var ...
  copy_from_user(&var,..., sizeof(var))@p1

  @local_inited_by_cfu depends on !escapes@
  type T;
  identifier var;
  position match.p0,match.p1;
  fresh identifier var_noinit = var##" __no_initialize";
  @@
  -T var@p0;
  +T var_noinit;
  ...
  copy_from_user(&var,..., sizeof(var))@p1

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

---
 v2:
  - changed __do_not_initialize to __no_initialize as requested by Kees
    Cook
  - wrote a Coccinelle script to generate the patch
---
 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..a59871532ff6b 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 __no_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 __no_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 __no_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 __no_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 __no_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 __no_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 v2 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 __no_initialize annotation.

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

---
 v2:
  - changed __do_not_initialize to __no_initialize as requested by Kees
    Cook
---
 drivers/android/binder.c | 4 ++--
 include/linux/wait.h     | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a59871532ff6b..66984e7c33094 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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 __no_initialize;
+	struct binder_write_read bwr;
 
 	if (size != sizeof(struct binder_write_read)) {
 		ret = -EINVAL;
@@ -5026,7 +5026,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		break;
 	case BINDER_SET_MAX_THREADS: {
-		int max_threads;
+		int max_threads __no_initialize;
 
 		if (copy_from_user(&max_threads, ubuf,
 				   sizeof(max_threads))) {
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d021377..b52a9bb2c7727 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 __no_initialize;			\
 	long __ret = ret;	/* explicit shadow */				\
 										\
 	init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE : 0);	\
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 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 __no_initialize annotation.

Cc: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

---
 v2:
  - changed __do_not_initialize to __no_initialize as requested by Kees
    Cook
---
 drivers/android/binder.c | 4 ++--
 include/linux/wait.h     | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index a59871532ff6b..66984e7c33094 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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 __no_initialize;
+	struct binder_write_read bwr;
 
 	if (size != sizeof(struct binder_write_read)) {
 		ret = -EINVAL;
@@ -5026,7 +5026,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		break;
 	case BINDER_SET_MAX_THREADS: {
-		int max_threads;
+		int max_threads __no_initialize;
 
 		if (copy_from_user(&max_threads, ubuf,
 				   sizeof(max_threads))) {
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3283c8d021377..b52a9bb2c7727 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 __no_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 v2 2/3] binder: do not initialize locals passed to copy_from_user()

[Joe Perches]

On Mon, 2020-03-02 at 14:04 +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:
[]
> diff --git 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 __no_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))

I fail to see any value in marking tr with __no_initialize
when it's immediately written to by copy_from_user.

>  				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 __no_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))

etc...

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

[Joe Perches]

On Mon, 2020-03-02 at 14:04 +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:
[]
> diff --git 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 __no_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))

I fail to see any value in marking tr with __no_initialize
when it's immediately written to by copy_from_user.

>  				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 __no_initialize;
>  
>  			if (copy_from_user(&tr, ptr, sizeof(tr)))

etc...


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

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 14:04 +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:
> []
> > diff --git 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 __no_initialize;
> >
> >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
>
> I fail to see any value in marking tr with __no_initialize
> when it's immediately written to by copy_from_user.

This is being done exactly because it's immediately written to by copy_to_user()
Clang is currently unable to figure out that copy_to_user() initializes memory.
So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
the following code:

  struct binder_transaction_data_sg tr;
  memset(&tr, 0xAA, sizeof(tr));
  if (copy_from_user(&tr, ptr, sizeof(tr))) {...}

This unnecessarily slows the code down, so we add __no_initialize to
prevent the compiler from emitting the redundant initialization.

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 14:04 +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:
> []
> > diff --git 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 __no_initialize;
> >
> >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
>
> I fail to see any value in marking tr with __no_initialize
> when it's immediately written to by copy_from_user.

This is being done exactly because it's immediately written to by copy_to_user()
Clang is currently unable to figure out that copy_to_user() initializes memory.
So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
the following code:

  struct binder_transaction_data_sg tr;
  memset(&tr, 0xAA, sizeof(tr));
  if (copy_from_user(&tr, ptr, sizeof(tr))) {...}

This unnecessarily slows the code down, so we add __no_initialize to
prevent the compiler from emitting the redundant initialization.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Mon, Mar 02, 2020 at 02:25:51PM +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> >
> > On Mon, 2020-03-02 at 14:04 +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:
> > []
> > > diff --git 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 __no_initialize;
> > >
> > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> >
> > I fail to see any value in marking tr with __no_initialize
> > when it's immediately written to by copy_from_user.
> 
> This is being done exactly because it's immediately written to by copy_to_user()
> Clang is currently unable to figure out that copy_to_user() initializes memory.
                                                    ^^
typo s/to/from/.

It feels more useful to annotate copy_from_user().  That would be useful
for Smatch as well.

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, Mar 02, 2020 at 02:25:51PM +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> >
> > On Mon, 2020-03-02 at 14:04 +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:
> > []
> > > diff --git 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 __no_initialize;
> > >
> > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> >
> > I fail to see any value in marking tr with __no_initialize
> > when it's immediately written to by copy_from_user.
> 
> This is being done exactly because it's immediately written to by copy_to_user()
> Clang is currently unable to figure out that copy_to_user() initializes memory.
                                                    ^^
typo s/to/from/.

It feels more useful to annotate copy_from_user().  That would be useful
for Smatch as well.

regards,
dan carpenter

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

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

[Joe Perches]

On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:04 +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:
> > []
> > > diff --git 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 __no_initialize;
> > > 
> > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > 
> > I fail to see any value in marking tr with __no_initialize
> > when it's immediately written to by copy_from_user.
> 
> This is being done exactly because it's immediately written to by copy_to_user()
> Clang is currently unable to figure out that copy_to_user() initializes memory.
> So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> the following code:
> 
>   struct binder_transaction_data_sg tr;
>   memset(&tr, 0xAA, sizeof(tr));
>   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> 
> This unnecessarily slows the code down, so we add __no_initialize to
> prevent the compiler from emitting the redundant initialization.

So?  CONFIG_INIT_STACK_ALL by design slows down code.

This marking would likely need to be done for nearly all
3000+ copy_from_user entries.

Why not try to get something done on the compiler side
to mark the function itself rather than the uses?

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

[Joe Perches]

On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:04 +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:
> > []
> > > diff --git 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 __no_initialize;
> > > 
> > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > 
> > I fail to see any value in marking tr with __no_initialize
> > when it's immediately written to by copy_from_user.
> 
> This is being done exactly because it's immediately written to by copy_to_user()
> Clang is currently unable to figure out that copy_to_user() initializes memory.
> So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> the following code:
> 
>   struct binder_transaction_data_sg tr;
>   memset(&tr, 0xAA, sizeof(tr));
>   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> 
> This unnecessarily slows the code down, so we add __no_initialize to
> prevent the compiler from emitting the redundant initialization.

So?  CONFIG_INIT_STACK_ALL by design slows down code.

This marking would likely need to be done for nearly all
3000+ copy_from_user entries.

Why not try to get something done on the compiler side
to mark the function itself rather than the uses?



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

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

[Todd Kjos]

On Mon, Mar 2, 2020 at 5:04 AM <glider@google.com> wrote:
>
> 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 __no_initialize annotation.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>
>
> ---
>  v2:
>   - changed __do_not_initialize to __no_initialize as requested by Kees
>     Cook
> ---
>  drivers/android/binder.c | 4 ++--
>  include/linux/wait.h     | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index a59871532ff6b..66984e7c33094 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -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 __no_initialize;
> +       struct binder_write_read bwr;

How did __no_initialize get set so that it can be removed here? Should
the addition of __no_initilize be removed earlier in the series (tip
doesn't have the __no_initialize).

>
>         if (size != sizeof(struct binder_write_read)) {
>                 ret = -EINVAL;
> @@ -5026,7 +5026,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                         goto err;
>                 break;
>         case BINDER_SET_MAX_THREADS: {
> -               int max_threads;
> +               int max_threads __no_initialize;

Is this really needed? A single integer in a rarely called ioctl()
being initialized twice doesn't warrant this optimization.

>
>                 if (copy_from_user(&max_threads, ubuf,
>                                    sizeof(max_threads))) {
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 3283c8d021377..b52a9bb2c7727 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 __no_initialize;                     \
>         long __ret = ret;       /* explicit shadow */                           \
>                                                                                 \
>         init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE : 0);        \
> --
> 2.25.0.265.gbab2e86ba0-goog
>

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

[Todd Kjos]

On Mon, Mar 2, 2020 at 5:04 AM <glider@google.com> wrote:
>
> 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 __no_initialize annotation.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>
>
> ---
>  v2:
>   - changed __do_not_initialize to __no_initialize as requested by Kees
>     Cook
> ---
>  drivers/android/binder.c | 4 ++--
>  include/linux/wait.h     | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index a59871532ff6b..66984e7c33094 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -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 __no_initialize;
> +       struct binder_write_read bwr;

How did __no_initialize get set so that it can be removed here? Should
the addition of __no_initilize be removed earlier in the series (tip
doesn't have the __no_initialize).

>
>         if (size != sizeof(struct binder_write_read)) {
>                 ret = -EINVAL;
> @@ -5026,7 +5026,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                         goto err;
>                 break;
>         case BINDER_SET_MAX_THREADS: {
> -               int max_threads;
> +               int max_threads __no_initialize;

Is this really needed? A single integer in a rarely called ioctl()
being initialized twice doesn't warrant this optimization.

>
>                 if (copy_from_user(&max_threads, ubuf,
>                                    sizeof(max_threads))) {
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 3283c8d021377..b52a9bb2c7727 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 __no_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 v2 2/3] binder: do not initialize locals passed to copy_from_user()

[Greg KH]

On Mon, Mar 02, 2020 at 02:04:29PM +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
> __no_initialize to local Binder structures.

I would like to see actual benchmark numbers showing this is
needed/useful otherwise it's going to just be random people adding this
marking to random places with no real reason.

thanks,

greg k-h

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

[Greg KH]

On Mon, Mar 02, 2020 at 02:04:29PM +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
> __no_initialize to local Binder structures.

I would like to see actual benchmark numbers showing this is
needed/useful otherwise it's going to just be random people adding this
marking to random places with no real reason.

thanks,

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

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 5:57 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Mon, Mar 2, 2020 at 5:04 AM <glider@google.com> wrote:
> >
> > 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 __no_initialize annotation.
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> >
> > ---
> >  v2:
> >   - changed __do_not_initialize to __no_initialize as requested by Kees
> >     Cook
> > ---
> >  drivers/android/binder.c | 4 ++--
> >  include/linux/wait.h     | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index a59871532ff6b..66984e7c33094 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -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 __no_initialize;
> > +       struct binder_write_read bwr;
>
> How did __no_initialize get set so that it can be removed here? Should
> the addition of __no_initilize be removed earlier in the series (tip
> doesn't have the __no_initialize).

Sorry, I messed up this patch. It should not be touching binder.c at
all. All binder changes should go into patch 2/3.


> >         case BINDER_SET_MAX_THREADS: {
> > -               int max_threads;
> > +               int max_threads __no_initialize;
>
> Is this really needed? A single integer in a rarely called ioctl()
> being initialized twice doesn't warrant this optimization.

It really does not, and I didn't have this bit in v1.
But if we don't want this diff to bit rot, we'd better have a
Coccinelle script generating it.
The script I added to the description of patch 2/3 introduced this
annotation, and I thought keeping it is better than trying to teach
the script about the size of the arguments.

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 5:57 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Mon, Mar 2, 2020 at 5:04 AM <glider@google.com> wrote:
> >
> > 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 __no_initialize annotation.
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> >
> > ---
> >  v2:
> >   - changed __do_not_initialize to __no_initialize as requested by Kees
> >     Cook
> > ---
> >  drivers/android/binder.c | 4 ++--
> >  include/linux/wait.h     | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index a59871532ff6b..66984e7c33094 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -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 __no_initialize;
> > +       struct binder_write_read bwr;
>
> How did __no_initialize get set so that it can be removed here? Should
> the addition of __no_initilize be removed earlier in the series (tip
> doesn't have the __no_initialize).

Sorry, I messed up this patch. It should not be touching binder.c at
all. All binder changes should go into patch 2/3.


> >         case BINDER_SET_MAX_THREADS: {
> > -               int max_threads;
> > +               int max_threads __no_initialize;
>
> Is this really needed? A single integer in a rarely called ioctl()
> being initialized twice doesn't warrant this optimization.

It really does not, and I didn't have this bit in v1.
But if we don't want this diff to bit rot, we'd better have a
Coccinelle script generating it.
The script I added to the description of patch 2/3 introduced this
annotation, and I thought keeping it is better than trying to teach
the script about the size of the arguments.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 14:04 +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:
> > > []
> > > > diff --git 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 __no_initialize;
> > > >
> > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > >
> > > I fail to see any value in marking tr with __no_initialize
> > > when it's immediately written to by copy_from_user.
> >
> > This is being done exactly because it's immediately written to by copy_to_user()
> > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > the following code:
> >
> >   struct binder_transaction_data_sg tr;
> >   memset(&tr, 0xAA, sizeof(tr));
> >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> >
> > This unnecessarily slows the code down, so we add __no_initialize to
> > prevent the compiler from emitting the redundant initialization.
>
> So?  CONFIG_INIT_STACK_ALL by design slows down code.
Correct.

> This marking would likely need to be done for nearly all
> 3000+ copy_from_user entries.
Unfortunately, yes. I was just hoping to do so for a handful of hot
cases that we encounter, but in the long-term a compiler solution must
supersede them.

> Why not try to get something done on the compiler side
> to mark the function itself rather than the uses?
This is being worked on in the meantime as well (see
http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
Do you have any particular requisitions about how this should look on
the source level?

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 14:04 +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:
> > > []
> > > > diff --git 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 __no_initialize;
> > > >
> > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > >
> > > I fail to see any value in marking tr with __no_initialize
> > > when it's immediately written to by copy_from_user.
> >
> > This is being done exactly because it's immediately written to by copy_to_user()
> > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > the following code:
> >
> >   struct binder_transaction_data_sg tr;
> >   memset(&tr, 0xAA, sizeof(tr));
> >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> >
> > This unnecessarily slows the code down, so we add __no_initialize to
> > prevent the compiler from emitting the redundant initialization.
>
> So?  CONFIG_INIT_STACK_ALL by design slows down code.
Correct.

> This marking would likely need to be done for nearly all
> 3000+ copy_from_user entries.
Unfortunately, yes. I was just hoping to do so for a handful of hot
cases that we encounter, but in the long-term a compiler solution must
supersede them.

> Why not try to get something done on the compiler side
> to mark the function itself rather than the uses?
This is being worked on in the meantime as well (see
http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
Do you have any particular requisitions about how this should look on
the source level?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 6:38 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 02, 2020 at 02:04:29PM +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
> > __no_initialize to local Binder structures.
>
> I would like to see actual benchmark numbers showing this is
> needed/useful otherwise it's going to just be random people adding this
> marking to random places with no real reason.
This were lib[hw]binder_benchmarks.
I will update patch description with benchmark data.
> thanks,
>
> greg k-h



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 6:38 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 02, 2020 at 02:04:29PM +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
> > __no_initialize to local Binder structures.
>
> I would like to see actual benchmark numbers showing this is
> needed/useful otherwise it's going to just be random people adding this
> marking to random places with no real reason.
This were lib[hw]binder_benchmarks.
I will update patch description with benchmark data.
> thanks,
>
> greg k-h



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Jann Horn]

On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > []
> > > > > diff --git 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 __no_initialize;
> > > > >
> > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > >
> > > > I fail to see any value in marking tr with __no_initialize
> > > > when it's immediately written to by copy_from_user.
> > >
> > > This is being done exactly because it's immediately written to by copy_to_user()
> > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > the following code:
> > >
> > >   struct binder_transaction_data_sg tr;
> > >   memset(&tr, 0xAA, sizeof(tr));
> > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > >
> > > This unnecessarily slows the code down, so we add __no_initialize to
> > > prevent the compiler from emitting the redundant initialization.
> >
> > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> Correct.
>
> > This marking would likely need to be done for nearly all
> > 3000+ copy_from_user entries.
> Unfortunately, yes. I was just hoping to do so for a handful of hot
> cases that we encounter, but in the long-term a compiler solution must
> supersede them.
>
> > Why not try to get something done on the compiler side
> > to mark the function itself rather than the uses?
> This is being worked on in the meantime as well (see
> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> Do you have any particular requisitions about how this should look on
> the source level?

Just thinking out loud: Should this be a function attribute, or should
it be a builtin - something like __builtin_assume_initialized(ptr,
len)? That would make it also work for macros, and it might simplify
the handling of inlining in the compiler. And you wouldn't need such a
complicated attribute that refers to function arguments by index and
such. The downside would be that it wouldn't work for non-inlined
functions without creating inline wrappers around them...

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

[Jann Horn]

On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > []
> > > > > diff --git 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 __no_initialize;
> > > > >
> > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > >
> > > > I fail to see any value in marking tr with __no_initialize
> > > > when it's immediately written to by copy_from_user.
> > >
> > > This is being done exactly because it's immediately written to by copy_to_user()
> > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > the following code:
> > >
> > >   struct binder_transaction_data_sg tr;
> > >   memset(&tr, 0xAA, sizeof(tr));
> > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > >
> > > This unnecessarily slows the code down, so we add __no_initialize to
> > > prevent the compiler from emitting the redundant initialization.
> >
> > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> Correct.
>
> > This marking would likely need to be done for nearly all
> > 3000+ copy_from_user entries.
> Unfortunately, yes. I was just hoping to do so for a handful of hot
> cases that we encounter, but in the long-term a compiler solution must
> supersede them.
>
> > Why not try to get something done on the compiler side
> > to mark the function itself rather than the uses?
> This is being worked on in the meantime as well (see
> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> Do you have any particular requisitions about how this should look on
> the source level?

Just thinking out loud: Should this be a function attribute, or should
it be a builtin - something like __builtin_assume_initialized(ptr,
len)? That would make it also work for macros, and it might simplify
the handling of inlining in the compiler. And you wouldn't need such a
complicated attribute that refers to function arguments by index and
such. The downside would be that it wouldn't work for non-inlined
functions without creating inline wrappers around them...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Greg Kroah-Hartman]

On Mon, Mar 02, 2020 at 07:03:19PM +0100, Alexander Potapenko wrote:
> > >         case BINDER_SET_MAX_THREADS: {
> > > -               int max_threads;
> > > +               int max_threads __no_initialize;
> >
> > Is this really needed? A single integer in a rarely called ioctl()
> > being initialized twice doesn't warrant this optimization.
> 
> It really does not, and I didn't have this bit in v1.
> But if we don't want this diff to bit rot, we'd better have a
> Coccinelle script generating it.
> The script I added to the description of patch 2/3 introduced this
> annotation, and I thought keeping it is better than trying to teach
> the script about the size of the arguments.

Please fix the script, don't add stuff to the kernel that is not needed.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Mon, Mar 02, 2020 at 07:03:19PM +0100, Alexander Potapenko wrote:
> > >         case BINDER_SET_MAX_THREADS: {
> > > -               int max_threads;
> > > +               int max_threads __no_initialize;
> >
> > Is this really needed? A single integer in a rarely called ioctl()
> > being initialized twice doesn't warrant this optimization.
> 
> It really does not, and I didn't have this bit in v1.
> But if we don't want this diff to bit rot, we'd better have a
> Coccinelle script generating it.
> The script I added to the description of patch 2/3 introduced this
> annotation, and I thought keeping it is better than trying to teach
> the script about the size of the arguments.

Please fix the script, don't add stuff to the kernel that is not needed.

thanks,

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

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

[Joe Perches]

On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > []
> > > > > diff --git 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 __no_initialize;
> > > > > 
> > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > 
> > > > I fail to see any value in marking tr with __no_initialize
> > > > when it's immediately written to by copy_from_user.
> > > 
> > > This is being done exactly because it's immediately written to by copy_to_user()
> > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > the following code:
> > > 
> > >   struct binder_transaction_data_sg tr;
> > >   memset(&tr, 0xAA, sizeof(tr));
> > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > 
> > > This unnecessarily slows the code down, so we add __no_initialize to
> > > prevent the compiler from emitting the redundant initialization.
> > 
> > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> Correct.
> 
> > This marking would likely need to be done for nearly all
> > 3000+ copy_from_user entries.
> Unfortunately, yes. I was just hoping to do so for a handful of hot
> cases that we encounter, but in the long-term a compiler solution must
> supersede them.
> 
> > Why not try to get something done on the compiler side
> > to mark the function itself rather than the uses?
> This is being worked on in the meantime as well (see
> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> Do you have any particular requisitions about how this should look on
> the source level?

I presume something like the below when appropriate for
automatic variables when not already initialized or modified.
---
 include/linux/uaccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 8a215c..3e034b5 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
 #endif
 
 static __always_inline unsigned long __must_check
-copy_from_user(void *to, const void __user *from, unsigned long n)
+copy_from_user(void __no_initialize *to, const void __user *from,
+	       unsigned long n)
 {
 	if (likely(check_copy_size(to, n, false)))
 		n = _copy_from_user(to, from, n);

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

[Joe Perches]

On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > []
> > > > > diff --git 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 __no_initialize;
> > > > > 
> > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > 
> > > > I fail to see any value in marking tr with __no_initialize
> > > > when it's immediately written to by copy_from_user.
> > > 
> > > This is being done exactly because it's immediately written to by copy_to_user()
> > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > the following code:
> > > 
> > >   struct binder_transaction_data_sg tr;
> > >   memset(&tr, 0xAA, sizeof(tr));
> > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > 
> > > This unnecessarily slows the code down, so we add __no_initialize to
> > > prevent the compiler from emitting the redundant initialization.
> > 
> > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> Correct.
> 
> > This marking would likely need to be done for nearly all
> > 3000+ copy_from_user entries.
> Unfortunately, yes. I was just hoping to do so for a handful of hot
> cases that we encounter, but in the long-term a compiler solution must
> supersede them.
> 
> > Why not try to get something done on the compiler side
> > to mark the function itself rather than the uses?
> This is being worked on in the meantime as well (see
> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> Do you have any particular requisitions about how this should look on
> the source level?

I presume something like the below when appropriate for
automatic variables when not already initialized or modified.
---
 include/linux/uaccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 8a215c..3e034b5 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
 #endif
 
 static __always_inline unsigned long __must_check
-copy_from_user(void *to, const void __user *from, unsigned long n)
+copy_from_user(void __no_initialize *to, const void __user *from,
+	       unsigned long n)
 {
 	if (likely(check_copy_size(to, n, false)))
 		n = _copy_from_user(to, from, n);


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

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > []
> > > > > > diff --git 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 __no_initialize;
> > > > > >
> > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > >
> > > > > I fail to see any value in marking tr with __no_initialize
> > > > > when it's immediately written to by copy_from_user.
> > > >
> > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > the following code:
> > > >
> > > >   struct binder_transaction_data_sg tr;
> > > >   memset(&tr, 0xAA, sizeof(tr));
> > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > >
> > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > prevent the compiler from emitting the redundant initialization.
> > >
> > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > Correct.
> >
> > > This marking would likely need to be done for nearly all
> > > 3000+ copy_from_user entries.
> > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > cases that we encounter, but in the long-term a compiler solution must
> > supersede them.
> >
> > > Why not try to get something done on the compiler side
> > > to mark the function itself rather than the uses?
> > This is being worked on in the meantime as well (see
> > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > Do you have any particular requisitions about how this should look on
> > the source level?
>
> I presume something like the below when appropriate for
> automatic variables when not already initialized or modified.
> ---
>  include/linux/uaccess.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 8a215c..3e034b5 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
>  #endif
>
>  static __always_inline unsigned long __must_check
> -copy_from_user(void *to, const void __user *from, unsigned long n)
> +copy_from_user(void __no_initialize *to, const void __user *from,
> +              unsigned long n)

Shall this __no_initialize attribute denote that the whole object
passed to it is initialized?
Or do we need to encode the length as well, as Jann suggests?
It's also interesting what should happen if *to is pointing _inside_ a
local object - presumably it's unsafe to disable initialization for
the whole object.

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

[Alexander Potapenko]

On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > []
> > > > > > diff --git 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 __no_initialize;
> > > > > >
> > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > >
> > > > > I fail to see any value in marking tr with __no_initialize
> > > > > when it's immediately written to by copy_from_user.
> > > >
> > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > the following code:
> > > >
> > > >   struct binder_transaction_data_sg tr;
> > > >   memset(&tr, 0xAA, sizeof(tr));
> > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > >
> > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > prevent the compiler from emitting the redundant initialization.
> > >
> > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > Correct.
> >
> > > This marking would likely need to be done for nearly all
> > > 3000+ copy_from_user entries.
> > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > cases that we encounter, but in the long-term a compiler solution must
> > supersede them.
> >
> > > Why not try to get something done on the compiler side
> > > to mark the function itself rather than the uses?
> > This is being worked on in the meantime as well (see
> > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > Do you have any particular requisitions about how this should look on
> > the source level?
>
> I presume something like the below when appropriate for
> automatic variables when not already initialized or modified.
> ---
>  include/linux/uaccess.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 8a215c..3e034b5 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
>  #endif
>
>  static __always_inline unsigned long __must_check
> -copy_from_user(void *to, const void __user *from, unsigned long n)
> +copy_from_user(void __no_initialize *to, const void __user *from,
> +              unsigned long n)

Shall this __no_initialize attribute denote that the whole object
passed to it is initialized?
Or do we need to encode the length as well, as Jann suggests?
It's also interesting what should happen if *to is pointing _inside_ a
local object - presumably it's unsafe to disable initialization for
the whole object.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Tue, Mar 03, 2020 at 10:14:18AM +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
> >
> > On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > > []
> > > > > > > diff --git 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 __no_initialize;
> > > > > > >
> > > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > > >
> > > > > > I fail to see any value in marking tr with __no_initialize
> > > > > > when it's immediately written to by copy_from_user.
> > > > >
> > > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > > the following code:
> > > > >
> > > > >   struct binder_transaction_data_sg tr;
> > > > >   memset(&tr, 0xAA, sizeof(tr));
> > > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > > >
> > > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > > prevent the compiler from emitting the redundant initialization.
> > > >
> > > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > > Correct.
> > >
> > > > This marking would likely need to be done for nearly all
> > > > 3000+ copy_from_user entries.
> > > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > > cases that we encounter, but in the long-term a compiler solution must
> > > supersede them.
> > >
> > > > Why not try to get something done on the compiler side
> > > > to mark the function itself rather than the uses?
> > > This is being worked on in the meantime as well (see
> > > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > > Do you have any particular requisitions about how this should look on
> > > the source level?
> >
> > I presume something like the below when appropriate for
> > automatic variables when not already initialized or modified.
> > ---
> >  include/linux/uaccess.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> > index 8a215c..3e034b5 100644
> > --- a/include/linux/uaccess.h
> > +++ b/include/linux/uaccess.h
> > @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
> >  #endif
> >
> >  static __always_inline unsigned long __must_check
> > -copy_from_user(void *to, const void __user *from, unsigned long n)
> > +copy_from_user(void __no_initialize *to, const void __user *from,
> > +              unsigned long n)
> 
> Shall this __no_initialize attribute denote that the whole object
> passed to it is initialized?
> Or do we need to encode the length as well, as Jann suggests?
> It's also interesting what should happen if *to is pointing _inside_ a
> local object - presumably it's unsafe to disable initialization for
> the whole object.

The real fix is to initialize everything manually, the automated
initialization is a hardenning feature which many people will disable.
So I don't think the hardenning needs to be perfect, it needs to simple
and fast.

regards,
dan carpenter

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

[Dan Carpenter]

On Tue, Mar 03, 2020 at 10:14:18AM +0100, Alexander Potapenko wrote:
> On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
> >
> > On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > > []
> > > > > > > diff --git 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 __no_initialize;
> > > > > > >
> > > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > > >
> > > > > > I fail to see any value in marking tr with __no_initialize
> > > > > > when it's immediately written to by copy_from_user.
> > > > >
> > > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > > the following code:
> > > > >
> > > > >   struct binder_transaction_data_sg tr;
> > > > >   memset(&tr, 0xAA, sizeof(tr));
> > > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > > >
> > > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > > prevent the compiler from emitting the redundant initialization.
> > > >
> > > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > > Correct.
> > >
> > > > This marking would likely need to be done for nearly all
> > > > 3000+ copy_from_user entries.
> > > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > > cases that we encounter, but in the long-term a compiler solution must
> > > supersede them.
> > >
> > > > Why not try to get something done on the compiler side
> > > > to mark the function itself rather than the uses?
> > > This is being worked on in the meantime as well (see
> > > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > > Do you have any particular requisitions about how this should look on
> > > the source level?
> >
> > I presume something like the below when appropriate for
> > automatic variables when not already initialized or modified.
> > ---
> >  include/linux/uaccess.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> > index 8a215c..3e034b5 100644
> > --- a/include/linux/uaccess.h
> > +++ b/include/linux/uaccess.h
> > @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
> >  #endif
> >
> >  static __always_inline unsigned long __must_check
> > -copy_from_user(void *to, const void __user *from, unsigned long n)
> > +copy_from_user(void __no_initialize *to, const void __user *from,
> > +              unsigned long n)
> 
> Shall this __no_initialize attribute denote that the whole object
> passed to it is initialized?
> Or do we need to encode the length as well, as Jann suggests?
> It's also interesting what should happen if *to is pointing _inside_ a
> local object - presumably it's unsafe to disable initialization for
> the whole object.

The real fix is to initialize everything manually, the automated
initialization is a hardenning feature which many people will disable.
So I don't think the hardenning needs to be perfect, it needs to simple
and fast.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Joe Perches]

On Tue, 2020-03-03 at 12:38 +0300, Dan Carpenter wrote:
> On Tue, Mar 03, 2020 at 10:14:18AM +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > > > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > > > []
> > > > > > > > diff --git 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 __no_initialize;
> > > > > > > > 
> > > > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > > > > 
> > > > > > > I fail to see any value in marking tr with __no_initialize
> > > > > > > when it's immediately written to by copy_from_user.
> > > > > > 
> > > > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > > > the following code:
> > > > > > 
> > > > > >   struct binder_transaction_data_sg tr;
> > > > > >   memset(&tr, 0xAA, sizeof(tr));
> > > > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > > > > 
> > > > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > > > prevent the compiler from emitting the redundant initialization.
> > > > > 
> > > > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > > > Correct.
> > > > 
> > > > > This marking would likely need to be done for nearly all
> > > > > 3000+ copy_from_user entries.
> > > > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > > > cases that we encounter, but in the long-term a compiler solution must
> > > > supersede them.
> > > > 
> > > > > Why not try to get something done on the compiler side
> > > > > to mark the function itself rather than the uses?
> > > > This is being worked on in the meantime as well (see
> > > > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > > > Do you have any particular requisitions about how this should look on
> > > > the source level?
> > > 
> > > I presume something like the below when appropriate for
> > > automatic variables when not already initialized or modified.
> > > ---
[]
> > > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
[]
> > > @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
> > >  #endif
> > > 
> > >  static __always_inline unsigned long __must_check
> > > -copy_from_user(void *to, const void __user *from, unsigned long n)
> > > +copy_from_user(void __no_initialize *to, const void __user *from,
> > > +              unsigned long n)
> > 
> > Shall this __no_initialize attribute denote that the whole object
> > passed to it is initialized?

My presumption is the compiler could determine that only if the
accessed variable is a local automatic, it does not need to be
initialized.

> > Or do we need to encode the length as well, as Jann suggests?

I think not.

> > It's also interesting what should happen if *to is pointing _inside_ a
> > local object - presumably it's unsafe to disable initialization for
> > the whole object.

Are you asking if for example:

	struct foo {
		...;
	};

	struct bar {
		struct foo a;
		...;
	};

	void func(void)
	{
		struct bar b;
		...;
		copy_from_user(&b.a, baz, len);
		...;
	}

that the containing struct b would not be initialized?

I presume a compiler would initialized all of b, but
if it manages to initialize all of b but b.a, good on
the compiler writer.

> The real fix is to initialize everything manually, the automated
> initialization is a hardenning feature which many people will disable.
> So I don't think the hardenning needs to be perfect, it needs to simple
> and fast.

Dan, perhaps I don't understand you.
Can you clarify what you mean?

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

[Joe Perches]

On Tue, 2020-03-03 at 12:38 +0300, Dan Carpenter wrote:
> On Tue, Mar 03, 2020 at 10:14:18AM +0100, Alexander Potapenko wrote:
> > On Mon, Mar 2, 2020 at 7:51 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-03-02 at 19:17 +0100, Alexander Potapenko wrote:
> > > > On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2020-03-02 at 14:25 +0100, Alexander Potapenko wrote:
> > > > > > On Mon, Mar 2, 2020 at 2:11 PM Joe Perches <joe@perches.com> wrote:
> > > > > > > On Mon, 2020-03-02 at 14:04 +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:
> > > > > > > []
> > > > > > > > diff --git 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 __no_initialize;
> > > > > > > > 
> > > > > > > >                       if (copy_from_user(&tr, ptr, sizeof(tr)))
> > > > > > > 
> > > > > > > I fail to see any value in marking tr with __no_initialize
> > > > > > > when it's immediately written to by copy_from_user.
> > > > > > 
> > > > > > This is being done exactly because it's immediately written to by copy_to_user()
> > > > > > Clang is currently unable to figure out that copy_to_user() initializes memory.
> > > > > > So building the kernel with CONFIG_INIT_STACK_ALL=y basically leads to
> > > > > > the following code:
> > > > > > 
> > > > > >   struct binder_transaction_data_sg tr;
> > > > > >   memset(&tr, 0xAA, sizeof(tr));
> > > > > >   if (copy_from_user(&tr, ptr, sizeof(tr))) {...}
> > > > > > 
> > > > > > This unnecessarily slows the code down, so we add __no_initialize to
> > > > > > prevent the compiler from emitting the redundant initialization.
> > > > > 
> > > > > So?  CONFIG_INIT_STACK_ALL by design slows down code.
> > > > Correct.
> > > > 
> > > > > This marking would likely need to be done for nearly all
> > > > > 3000+ copy_from_user entries.
> > > > Unfortunately, yes. I was just hoping to do so for a handful of hot
> > > > cases that we encounter, but in the long-term a compiler solution must
> > > > supersede them.
> > > > 
> > > > > Why not try to get something done on the compiler side
> > > > > to mark the function itself rather than the uses?
> > > > This is being worked on in the meantime as well (see
> > > > http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> > > > Do you have any particular requisitions about how this should look on
> > > > the source level?
> > > 
> > > I presume something like the below when appropriate for
> > > automatic variables when not already initialized or modified.
> > > ---
[]
> > > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
[]
> > > @@ -138,7 +138,8 @@ _copy_to_user(void __user *, const void *, unsigned long);
> > >  #endif
> > > 
> > >  static __always_inline unsigned long __must_check
> > > -copy_from_user(void *to, const void __user *from, unsigned long n)
> > > +copy_from_user(void __no_initialize *to, const void __user *from,
> > > +              unsigned long n)
> > 
> > Shall this __no_initialize attribute denote that the whole object
> > passed to it is initialized?

My presumption is the compiler could determine that only if the
accessed variable is a local automatic, it does not need to be
initialized.

> > Or do we need to encode the length as well, as Jann suggests?

I think not.

> > It's also interesting what should happen if *to is pointing _inside_ a
> > local object - presumably it's unsafe to disable initialization for
> > the whole object.

Are you asking if for example:

	struct foo {
		...;
	};

	struct bar {
		struct foo a;
		...;
	};

	void func(void)
	{
		struct bar b;
		...;
		copy_from_user(&b.a, baz, len);
		...;
	}

that the containing struct b would not be initialized?

I presume a compiler would initialized all of b, but
if it manages to initialize all of b but b.a, good on
the compiler writer.

> The real fix is to initialize everything manually, the automated
> initialization is a hardenning feature which many people will disable.
> So I don't think the hardenning needs to be perfect, it needs to simple
> and fast.

Dan, perhaps I don't understand you.
Can you clarify what you mean?

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

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

[Dan Carpenter]

On Tue, Mar 03, 2020 at 05:56:51AM -0800, Joe Perches wrote:
> > The real fix is to initialize everything manually, the automated
> > initialization is a hardenning feature which many people will disable.
> > So I don't think the hardenning needs to be perfect, it needs to simple
> > and fast.
> 
> Dan, perhaps I don't understand you.
> Can you clarify what you mean?

I'm basically agreeing with you.

Even though copy_from_user() might only initialize part of the struct
we should just record that it initializes the struct without getting
bogged down in details.  The annotation should be simple.

If the automated system to initialize stack variables doesn't work 100%
that's okay because it's a supplement and not a replacement for manually
initializing stack variables.

regards,
dan carpenter

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

[Dan Carpenter]

On Tue, Mar 03, 2020 at 05:56:51AM -0800, Joe Perches wrote:
> > The real fix is to initialize everything manually, the automated
> > initialization is a hardenning feature which many people will disable.
> > So I don't think the hardenning needs to be perfect, it needs to simple
> > and fast.
> 
> Dan, perhaps I don't understand you.
> Can you clarify what you mean?

I'm basically agreeing with you.

Even though copy_from_user() might only initialize part of the struct
we should just record that it initializes the struct without getting
bogged down in details.  The annotation should be simple.

If the automated system to initialize stack variables doesn't work 100%
that's okay because it's a supplement and not a replacement for manually
initializing stack variables.

regards,
dan carpenter

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

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

[Kees Cook]

On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> The real fix is to initialize everything manually, the automated
> initialization is a hardenning feature which many people will disable.

I cannot disagree more with this sentiment. Linus has specifically said he
wants this initialization on by default[1], and the major thing holding
that back from happening is that no one working on GCC has had time to
add this feature there. All the kernels I know of that are built with
Clang (Android, Chrome OS, OpenMandriva) either already have this turned
on or have plans to do so shortly.

> So I don't think the hardenning needs to be perfect, it needs to simple
> and fast.

I think it should be able to be intelligently optimized, so I'm all for
finding ways to mark function arguments as "will be initialized" in some
fashion.

-Kees

[1] "Oh, I love that patch." https://lore.kernel.org/lkml/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j942dA@mail.gmail.com/

-- 
Kees Cook

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

[Kees Cook]

On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> The real fix is to initialize everything manually, the automated
> initialization is a hardenning feature which many people will disable.

I cannot disagree more with this sentiment. Linus has specifically said he
wants this initialization on by default[1], and the major thing holding
that back from happening is that no one working on GCC has had time to
add this feature there. All the kernels I know of that are built with
Clang (Android, Chrome OS, OpenMandriva) either already have this turned
on or have plans to do so shortly.

> So I don't think the hardenning needs to be perfect, it needs to simple
> and fast.

I think it should be able to be intelligently optimized, so I'm all for
finding ways to mark function arguments as "will be initialized" in some
fashion.

-Kees

[1] "Oh, I love that patch." https://lore.kernel.org/lkml/CA+55aFykZL+cSBJjBBts7ebEFfyGPdMzTmLSxKnT_29=j942dA@mail.gmail.com/

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

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

[Dan Carpenter]

On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > The real fix is to initialize everything manually, the automated
> > initialization is a hardenning feature which many people will disable.
> 
> I cannot disagree more with this sentiment. Linus has specifically said he
> wants this initialization on by default[1],

Fine, but as long as it's a configurable thing then we need to manually
initialize as well or it's still a CVE etc.  It will take a while before
we drop support for old versions of GCC as well.

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > The real fix is to initialize everything manually, the automated
> > initialization is a hardenning feature which many people will disable.
> 
> I cannot disagree more with this sentiment. Linus has specifically said he
> wants this initialization on by default[1],

Fine, but as long as it's a configurable thing then we need to manually
initialize as well or it's still a CVE etc.  It will take a while before
we drop support for old versions of GCC as well.

regards,
dan carpenter

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

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

[Kees Cook]

On Thu, Mar 05, 2020 at 11:07:56AM +0300, Dan Carpenter wrote:
> On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> > On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > > The real fix is to initialize everything manually, the automated
> > > initialization is a hardenning feature which many people will disable.
> > 
> > I cannot disagree more with this sentiment. Linus has specifically said he
> > wants this initialization on by default[1],
> 
> Fine, but as long as it's a configurable thing then we need to manually
> initialize as well or it's still a CVE etc.  It will take a while before
> we drop support for old versions of GCC as well.

Yes, I agree; that's totally true. We need to continue to fix all the
uninitialized flaws we encounter unless this is on by default for all
supported compiler versions (which will be a looong time). (But it's
not relevant to this patch because copy_from_user() does already do
the initialization.)

This set of patches was about dealing with the pathological cases of
auto-init colliding with functions that do, in fact, fully init. Though
I must say, I remain concerned about inventing such markings for fear
they'll be used in places where the "trust me, it's fully initialized"
state does not actually hold[1] but the author thinks it does.

-Kees

[1] https://lore.kernel.org/netdev/1509471094.3828.26.camel@edumazet-glaptop3.roam.corp.google.com/

-- 
Kees Cook

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

[Kees Cook]

On Thu, Mar 05, 2020 at 11:07:56AM +0300, Dan Carpenter wrote:
> On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> > On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > > The real fix is to initialize everything manually, the automated
> > > initialization is a hardenning feature which many people will disable.
> > 
> > I cannot disagree more with this sentiment. Linus has specifically said he
> > wants this initialization on by default[1],
> 
> Fine, but as long as it's a configurable thing then we need to manually
> initialize as well or it's still a CVE etc.  It will take a while before
> we drop support for old versions of GCC as well.

Yes, I agree; that's totally true. We need to continue to fix all the
uninitialized flaws we encounter unless this is on by default for all
supported compiler versions (which will be a looong time). (But it's
not relevant to this patch because copy_from_user() does already do
the initialization.)

This set of patches was about dealing with the pathological cases of
auto-init colliding with functions that do, in fact, fully init. Though
I must say, I remain concerned about inventing such markings for fear
they'll be used in places where the "trust me, it's fully initialized"
state does not actually hold[1] but the author thinks it does.

-Kees

[1] https://lore.kernel.org/netdev/1509471094.3828.26.camel@edumazet-glaptop3.roam.corp.google.com/

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

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

[Alexander Potapenko]

On Thu, Mar 5, 2020 at 9:26 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Mar 05, 2020 at 11:07:56AM +0300, Dan Carpenter wrote:
> > On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> > > On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > > > The real fix is to initialize everything manually, the automated
> > > > initialization is a hardenning feature which many people will disable.
> > >
> > > I cannot disagree more with this sentiment. Linus has specifically said he
> > > wants this initialization on by default[1],
> >
> > Fine, but as long as it's a configurable thing then we need to manually
> > initialize as well or it's still a CVE etc.  It will take a while before
> > we drop support for old versions of GCC as well.
>
> Yes, I agree; that's totally true. We need to continue to fix all the
> uninitialized flaws we encounter unless this is on by default for all
> supported compiler versions (which will be a looong time). (But it's
> not relevant to this patch because copy_from_user() does already do
> the initialization.)
>
> This set of patches was about dealing with the pathological cases of
> auto-init colliding with functions that do, in fact, fully init. Though
> I must say, I remain concerned about inventing such markings for fear
> they'll be used in places where the "trust me, it's fully initialized"
> state does not actually hold[1] but the author thinks it does.
>
> -Kees

Right now I'm trying to make Clang understand that output arguments of
inline assembly initialize the memory.
Then it would be possible to write something like:

  struct binder_transaction_data tr;
  asm("": "=m"(tr));
  if (copy_from_user(&tr, ptr, sizeof(tr))) ...

, and the asm directive can be hidden into copy_from_user().

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

[Alexander Potapenko]

On Thu, Mar 5, 2020 at 9:26 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Mar 05, 2020 at 11:07:56AM +0300, Dan Carpenter wrote:
> > On Wed, Mar 04, 2020 at 10:13:40AM -0800, Kees Cook wrote:
> > > On Tue, Mar 03, 2020 at 12:38:32PM +0300, Dan Carpenter wrote:
> > > > The real fix is to initialize everything manually, the automated
> > > > initialization is a hardenning feature which many people will disable.
> > >
> > > I cannot disagree more with this sentiment. Linus has specifically said he
> > > wants this initialization on by default[1],
> >
> > Fine, but as long as it's a configurable thing then we need to manually
> > initialize as well or it's still a CVE etc.  It will take a while before
> > we drop support for old versions of GCC as well.
>
> Yes, I agree; that's totally true. We need to continue to fix all the
> uninitialized flaws we encounter unless this is on by default for all
> supported compiler versions (which will be a looong time). (But it's
> not relevant to this patch because copy_from_user() does already do
> the initialization.)
>
> This set of patches was about dealing with the pathological cases of
> auto-init colliding with functions that do, in fact, fully init. Though
> I must say, I remain concerned about inventing such markings for fear
> they'll be used in places where the "trust me, it's fully initialized"
> state does not actually hold[1] but the author thinks it does.
>
> -Kees

Right now I'm trying to make Clang understand that output arguments of
inline assembly initialize the memory.
Then it would be possible to write something like:

  struct binder_transaction_data tr;
  asm("": "=m"(tr));
  if (copy_from_user(&tr, ptr, sizeof(tr))) ...

, and the asm directive can be hidden into copy_from_user().
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Rasmus Villemoes]

On 02/03/2020 19.31, Jann Horn wrote:
> On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
>> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
>>>
>>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
>> Correct.
>>
>>> This marking would likely need to be done for nearly all
>>> 3000+ copy_from_user entries.
>> Unfortunately, yes. I was just hoping to do so for a handful of hot
>> cases that we encounter, but in the long-term a compiler solution must
>> supersede them.
>>
>>> Why not try to get something done on the compiler side
>>> to mark the function itself rather than the uses?
>> This is being worked on in the meantime as well (see
>> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
>> Do you have any particular requisitions about how this should look on
>> the source level?
> 
> Just thinking out loud: Should this be a function attribute, or should
> it be a builtin - something like __builtin_assume_initialized(ptr,
> len)? That would make it also work for macros,

But with macros (and static inlines), the compiler sees all the
initialization being done, no?

and it might simplify
> the handling of inlining in the compiler. And you wouldn't need such a
> complicated attribute that refers to function arguments by index and
> such.

Does copy_from_user guarantee to zero-initialize the remaining buffer if
copying fails partway through? Otherwise it will be hard for the
compiler to make use of an annotation such as __assume_initialized(buf,
size - ret_from_cfu) - it will have to say "ok, the caller is bailing
out unless ret_from_cfu is 0, and in that case, yes, the whole local
struct variable is indeed initialized". And we can't make the annotation
unconditionally __assume_initialized(buf, size) [unless c_f_u comes with
that guarantee] because we don't know that all callers of c_f_u() bail
out on non-zero.


Somewhat related: I've long wanted a bunch of function attributes

  __may_read(ptr, bytes)
  __may_write(ptr, bytes)
  __will_write(ptr, bytes)

The first could be used to warn about passing an uninitialized or
too-small buffer (e.g.

  struct pollfd fds[4];
  poll(fds, sizeof(fds), ...) // whoops, should have been ARRAY_SIZE)

the second also for warning about a too-small buffer, and the third
would essentially be the same as __assume_initializes. Perhaps with some
sanitization option the compiler could also instrument the function
definition to not read/write beyond the area declared via those attributes.

But the attribute syntax doesn't currently allow complex expressions in
terms of the parameter names; I'd want to annotate poll as

  int poll(struct pollfd *fds, nfds_t nfds, int to) __may_rw(fds, nfds *
sizeof(*fds))

Rasmus

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

[Rasmus Villemoes]

On 02/03/2020 19.31, Jann Horn wrote:
> On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
>> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
>>>
>>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
>> Correct.
>>
>>> This marking would likely need to be done for nearly all
>>> 3000+ copy_from_user entries.
>> Unfortunately, yes. I was just hoping to do so for a handful of hot
>> cases that we encounter, but in the long-term a compiler solution must
>> supersede them.
>>
>>> Why not try to get something done on the compiler side
>>> to mark the function itself rather than the uses?
>> This is being worked on in the meantime as well (see
>> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
>> Do you have any particular requisitions about how this should look on
>> the source level?
> 
> Just thinking out loud: Should this be a function attribute, or should
> it be a builtin - something like __builtin_assume_initialized(ptr,
> len)? That would make it also work for macros,

But with macros (and static inlines), the compiler sees all the
initialization being done, no?

and it might simplify
> the handling of inlining in the compiler. And you wouldn't need such a
> complicated attribute that refers to function arguments by index and
> such.

Does copy_from_user guarantee to zero-initialize the remaining buffer if
copying fails partway through? Otherwise it will be hard for the
compiler to make use of an annotation such as __assume_initialized(buf,
size - ret_from_cfu) - it will have to say "ok, the caller is bailing
out unless ret_from_cfu is 0, and in that case, yes, the whole local
struct variable is indeed initialized". And we can't make the annotation
unconditionally __assume_initialized(buf, size) [unless c_f_u comes with
that guarantee] because we don't know that all callers of c_f_u() bail
out on non-zero.


Somewhat related: I've long wanted a bunch of function attributes

  __may_read(ptr, bytes)
  __may_write(ptr, bytes)
  __will_write(ptr, bytes)

The first could be used to warn about passing an uninitialized or
too-small buffer (e.g.

  struct pollfd fds[4];
  poll(fds, sizeof(fds), ...) // whoops, should have been ARRAY_SIZE)

the second also for warning about a too-small buffer, and the third
would essentially be the same as __assume_initializes. Perhaps with some
sanitization option the compiler could also instrument the function
definition to not read/write beyond the area declared via those attributes.

But the attribute syntax doesn't currently allow complex expressions in
terms of the parameter names; I'd want to annotate poll as

  int poll(struct pollfd *fds, nfds_t nfds, int to) __may_rw(fds, nfds *
sizeof(*fds))

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

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

[Jann Horn]

On Thu, Mar 5, 2020 at 10:03 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 02/03/2020 19.31, Jann Horn wrote:
> > On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
> >> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> >>>
> >>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
> >> Correct.
> >>
> >>> This marking would likely need to be done for nearly all
> >>> 3000+ copy_from_user entries.
> >> Unfortunately, yes. I was just hoping to do so for a handful of hot
> >> cases that we encounter, but in the long-term a compiler solution must
> >> supersede them.
> >>
> >>> Why not try to get something done on the compiler side
> >>> to mark the function itself rather than the uses?
> >> This is being worked on in the meantime as well (see
> >> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> >> Do you have any particular requisitions about how this should look on
> >> the source level?
> >
> > Just thinking out loud: Should this be a function attribute, or should
> > it be a builtin - something like __builtin_assume_initialized(ptr,
> > len)? That would make it also work for macros,
>
> But with macros (and static inlines), the compiler sees all the
> initialization being done, no?

Depends on how the macro writes to the buffer, whether it's a normal
write or happens through another function call or whatever.

> and it might simplify
> > the handling of inlining in the compiler. And you wouldn't need such a
> > complicated attribute that refers to function arguments by index and
> > such.
>
> Does copy_from_user guarantee to zero-initialize the remaining buffer if
> copying fails partway through?

Basically yes. From include/linux/uaccess.h:

static __always_inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
  if (likely(check_copy_size(to, n, false)))
    n = _copy_from_user(to, from, n);
  return n;
}

check_copy_size() should be optimized out entirely for straightforward
use of stack objects; it will only return false if the specified
address range crosses beyond an allocation boundary.
_copy_from_user() is defined as follows (there are two possible
definitions, both of them have the same method body, but they differ
in whether the function is inline - which one is used depends on the
architecture):

static inline __must_check unsigned long
_copy_from_user(void *to, const void __user *from, unsigned long n)
{
  unsigned long res = n;
  might_fault();
  if (likely(access_ok(from, n))) {
    kasan_check_write(to, n);
    res = raw_copy_from_user(to, from, n);
  }
  if (unlikely(res))
    memset(to + (n - res), 0, res);
  return res;
}

So annotating _copy_from_user(), or calling a magic
fake-initialization builtin directly before calling _copy_from_user(),
should be safe. As long as the compiler can eliminate the call to
check_copy_size(), that should then make that propagate up to the
caller of copy_from_user(). (You could also try to annotate
copy_from_user() directly, but I'm not sure whether doing it before
the bounds check might confuse the compiler somehow.)

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

[Jann Horn]

On Thu, Mar 5, 2020 at 10:03 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 02/03/2020 19.31, Jann Horn wrote:
> > On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@google.com> wrote:
> >> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@perches.com> wrote:
> >>>
> >>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
> >> Correct.
> >>
> >>> This marking would likely need to be done for nearly all
> >>> 3000+ copy_from_user entries.
> >> Unfortunately, yes. I was just hoping to do so for a handful of hot
> >> cases that we encounter, but in the long-term a compiler solution must
> >> supersede them.
> >>
> >>> Why not try to get something done on the compiler side
> >>> to mark the function itself rather than the uses?
> >> This is being worked on in the meantime as well (see
> >> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> >> Do you have any particular requisitions about how this should look on
> >> the source level?
> >
> > Just thinking out loud: Should this be a function attribute, or should
> > it be a builtin - something like __builtin_assume_initialized(ptr,
> > len)? That would make it also work for macros,
>
> But with macros (and static inlines), the compiler sees all the
> initialization being done, no?

Depends on how the macro writes to the buffer, whether it's a normal
write or happens through another function call or whatever.

> and it might simplify
> > the handling of inlining in the compiler. And you wouldn't need such a
> > complicated attribute that refers to function arguments by index and
> > such.
>
> Does copy_from_user guarantee to zero-initialize the remaining buffer if
> copying fails partway through?

Basically yes. From include/linux/uaccess.h:

static __always_inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
  if (likely(check_copy_size(to, n, false)))
    n = _copy_from_user(to, from, n);
  return n;
}

check_copy_size() should be optimized out entirely for straightforward
use of stack objects; it will only return false if the specified
address range crosses beyond an allocation boundary.
_copy_from_user() is defined as follows (there are two possible
definitions, both of them have the same method body, but they differ
in whether the function is inline - which one is used depends on the
architecture):

static inline __must_check unsigned long
_copy_from_user(void *to, const void __user *from, unsigned long n)
{
  unsigned long res = n;
  might_fault();
  if (likely(access_ok(from, n))) {
    kasan_check_write(to, n);
    res = raw_copy_from_user(to, from, n);
  }
  if (unlikely(res))
    memset(to + (n - res), 0, res);
  return res;
}

So annotating _copy_from_user(), or calling a magic
fake-initialization builtin directly before calling _copy_from_user(),
should be safe. As long as the compiler can eliminate the call to
check_copy_size(), that should then make that propagate up to the
caller of copy_from_user(). (You could also try to annotate
copy_from_user() directly, but I'm not sure whether doing it before
the bounds check might confuse the compiler somehow.)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Al Viro]

On Thu, Mar 05, 2020 at 10:03:25AM +0100, Rasmus Villemoes wrote:

> Does copy_from_user guarantee to zero-initialize the remaining buffer if
> copying fails partway through?

That's guaranteed, short of raw_copy_from_user() being completely broken.
What raw_copy_from_user() implementation must guarantee is that if
raw_copy_from_user(to, from, N) returns N - n, then
	* 0 <= n <= N
	* all attempted reads had been within the range [from .. from + N - 1]
	* all stores had been to the range [to .. to + n - 1] and every byte
within that range had been overwritten
	* for all k in [0 .. n - 1], the value stored at to[k] by the end of
the call is equal to the value that would've been possible to read from
from[k] at some point during the call.  In particular, for all bytes in
range [from .. from + n - 1] there had been a successful read of some
object containing that byte.
	* if everything in [from .. from + N - 1] is readable, the call
will copy the entire range into [to .. to + N - 1] and return 0.

Provided that, copy_from_user() will leave no uninitialized data in
destination object in any case, success or no success.

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

[Al Viro]

On Thu, Mar 05, 2020 at 10:03:25AM +0100, Rasmus Villemoes wrote:

> Does copy_from_user guarantee to zero-initialize the remaining buffer if
> copying fails partway through?

That's guaranteed, short of raw_copy_from_user() being completely broken.
What raw_copy_from_user() implementation must guarantee is that if
raw_copy_from_user(to, from, N) returns N - n, then
	* 0 <= n <= N
	* all attempted reads had been within the range [from .. from + N - 1]
	* all stores had been to the range [to .. to + n - 1] and every byte
within that range had been overwritten
	* for all k in [0 .. n - 1], the value stored at to[k] by the end of
the call is equal to the value that would've been possible to read from
from[k] at some point during the call.  In particular, for all bytes in
range [from .. from + n - 1] there had been a successful read of some
object containing that byte.
	* if everything in [from .. from + N - 1] is readable, the call
will copy the entire range into [to .. to + N - 1] and return 0.

Provided that, copy_from_user() will leave no uninitialized data in
destination object in any case, success or no success.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel