* [PATCH v2 1/3] compiler.h: define __no_initialize @ 2020-03-02 13:04 ` glider 0 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: dvyukov, jannh, devel, peterz, linux-kernel, Alexander Potapenko 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 1/3] compiler.h: define __no_initialize @ 2020-03-02 13:04 ` glider 0 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: devel, jannh, peterz, linux-kernel, Alexander Potapenko, dvyukov 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:04 ` glider @ 2020-03-02 13:04 ` glider -1 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: dvyukov, jannh, devel, peterz, linux-kernel, Alexander Potapenko 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 13:04 ` glider 0 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: devel, jannh, peterz, linux-kernel, Alexander Potapenko, dvyukov 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:04 ` glider @ 2020-03-02 13:09 ` Joe Perches -1 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 13:09 UTC (permalink / raw) To: glider, tkjos, keescook, gregkh, arve, mingo Cc: dvyukov, jannh, devel, peterz, linux-kernel 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... ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 13:09 ` Joe Perches 0 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 13:09 UTC (permalink / raw) To: glider, tkjos, keescook, gregkh, arve, mingo Cc: devel, peterz, linux-kernel, dvyukov, jannh 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:09 ` Joe Perches @ 2020-03-02 13:25 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 13:25 UTC (permalink / raw) To: Joe Perches Cc: Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 13:25 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 13:25 UTC (permalink / raw) To: Joe Perches Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:25 ` Alexander Potapenko @ 2020-03-02 13:52 ` Dan Carpenter -1 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-02 13:52 UTC (permalink / raw) To: Alexander Potapenko Cc: Joe Perches, open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 13:52 ` Dan Carpenter 0 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-02 13:52 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:25 ` Alexander Potapenko @ 2020-03-02 13:58 ` Joe Perches -1 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 13:58 UTC (permalink / raw) To: Alexander Potapenko Cc: Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 13:58 ` Joe Perches 0 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 13:58 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:58 ` Joe Perches @ 2020-03-02 18:17 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:17 UTC (permalink / raw) To: Joe Perches Cc: Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 18:17 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:17 UTC (permalink / raw) To: Joe Perches Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 18:17 ` Alexander Potapenko @ 2020-03-02 18:31 ` Jann Horn -1 siblings, 0 replies; 52+ messages in thread From: Jann Horn @ 2020-03-02 18:31 UTC (permalink / raw) To: Alexander Potapenko Cc: Joe Perches, Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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... ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 18:31 ` Jann Horn 0 siblings, 0 replies; 52+ messages in thread From: Jann Horn @ 2020-03-02 18:31 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 18:31 ` Jann Horn @ 2020-03-05 9:03 ` Rasmus Villemoes -1 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2020-03-05 9:03 UTC (permalink / raw) To: Jann Horn, Alexander Potapenko Cc: Joe Perches, Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-05 9:03 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2020-03-05 9:03 UTC (permalink / raw) To: Jann Horn, Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-05 9:03 ` Rasmus Villemoes @ 2020-03-05 12:45 ` Jann Horn -1 siblings, 0 replies; 52+ messages in thread From: Jann Horn @ 2020-03-05 12:45 UTC (permalink / raw) To: Rasmus Villemoes Cc: Alexander Potapenko, Joe Perches, Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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.) ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-05 12:45 ` Jann Horn 0 siblings, 0 replies; 52+ messages in thread From: Jann Horn @ 2020-03-05 12:45 UTC (permalink / raw) To: Rasmus Villemoes Cc: open list:ANDROID DRIVERS, Kees Cook, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-05 9:03 ` Rasmus Villemoes @ 2020-03-06 2:29 ` Al Viro -1 siblings, 0 replies; 52+ messages in thread From: Al Viro @ 2020-03-06 2:29 UTC (permalink / raw) To: Rasmus Villemoes Cc: Jann Horn, Alexander Potapenko, Joe Perches, Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-06 2:29 ` Al Viro 0 siblings, 0 replies; 52+ messages in thread From: Al Viro @ 2020-03-06 2:29 UTC (permalink / raw) To: Rasmus Villemoes Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 18:17 ` Alexander Potapenko @ 2020-03-02 18:50 ` Joe Perches -1 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 18:50 UTC (permalink / raw) To: Alexander Potapenko Cc: Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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); ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 18:50 ` Joe Perches 0 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-02 18:50 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 18:50 ` Joe Perches @ 2020-03-03 9:14 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-03 9:14 UTC (permalink / raw) To: Joe Perches Cc: Todd Kjos, Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-03 9:14 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-03 9:14 UTC (permalink / raw) To: Joe Perches Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-03 9:14 ` Alexander Potapenko @ 2020-03-03 9:38 ` Dan Carpenter -1 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-03 9:38 UTC (permalink / raw) To: Alexander Potapenko Cc: Joe Perches, open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-03 9:38 ` Dan Carpenter 0 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-03 9:38 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-03 9:38 ` Dan Carpenter @ 2020-03-03 13:56 ` Joe Perches -1 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-03 13:56 UTC (permalink / raw) To: Dan Carpenter, Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-03 13:56 ` Joe Perches 0 siblings, 0 replies; 52+ messages in thread From: Joe Perches @ 2020-03-03 13:56 UTC (permalink / raw) To: Dan Carpenter, Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-03 13:56 ` Joe Perches @ 2020-03-03 14:15 ` Dan Carpenter -1 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-03 14:15 UTC (permalink / raw) To: Joe Perches Cc: Alexander Potapenko, open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-03 14:15 ` Dan Carpenter 0 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-03 14:15 UTC (permalink / raw) To: Joe Perches Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-03 9:38 ` Dan Carpenter @ 2020-03-04 18:13 ` Kees Cook -1 siblings, 0 replies; 52+ messages in thread From: Kees Cook @ 2020-03-04 18:13 UTC (permalink / raw) To: Dan Carpenter Cc: Alexander Potapenko, Joe Perches, open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-04 18:13 ` Kees Cook 0 siblings, 0 replies; 52+ messages in thread From: Kees Cook @ 2020-03-04 18:13 UTC (permalink / raw) To: Dan Carpenter Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-04 18:13 ` Kees Cook @ 2020-03-05 8:07 ` Dan Carpenter -1 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-05 8:07 UTC (permalink / raw) To: Kees Cook Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-05 8:07 ` Dan Carpenter 0 siblings, 0 replies; 52+ messages in thread From: Dan Carpenter @ 2020-03-05 8:07 UTC (permalink / raw) To: Kees Cook Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-05 8:07 ` Dan Carpenter @ 2020-03-05 8:26 ` Kees Cook -1 siblings, 0 replies; 52+ messages in thread From: Kees Cook @ 2020-03-05 8:26 UTC (permalink / raw) To: Dan Carpenter Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-05 8:26 ` Kees Cook 0 siblings, 0 replies; 52+ messages in thread From: Kees Cook @ 2020-03-05 8:26 UTC (permalink / raw) To: Dan Carpenter Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Alexander Potapenko, Joe Perches, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-05 8:26 ` Kees Cook @ 2020-03-05 8:33 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-05 8:33 UTC (permalink / raw) To: Kees Cook Cc: Dan Carpenter, open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Joe Perches, Dmitriy Vyukov, Todd Kjos 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(). ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-05 8:33 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-05 8:33 UTC (permalink / raw) To: Kees Cook Cc: open list:ANDROID DRIVERS, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Joe Perches, Dan Carpenter, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 13:04 ` glider @ 2020-03-02 17:38 ` Greg KH -1 siblings, 0 replies; 52+ messages in thread From: Greg KH @ 2020-03-02 17:38 UTC (permalink / raw) To: glider Cc: tkjos, keescook, arve, mingo, dvyukov, jannh, devel, peterz, linux-kernel 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 17:38 ` Greg KH 0 siblings, 0 replies; 52+ messages in thread From: Greg KH @ 2020-03-02 17:38 UTC (permalink / raw) To: glider Cc: devel, keescook, jannh, peterz, linux-kernel, arve, mingo, dvyukov, tkjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() 2020-03-02 17:38 ` Greg KH @ 2020-03-02 18:28 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:28 UTC (permalink / raw) To: Greg KH Cc: Todd Kjos, Kees Cook, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() @ 2020-03-02 18:28 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:28 UTC (permalink / raw) To: Greg KH Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitriy Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() 2020-03-02 13:04 ` glider @ 2020-03-02 13:04 ` glider -1 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: dvyukov, jannh, devel, peterz, linux-kernel, Alexander Potapenko 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() @ 2020-03-02 13:04 ` glider 0 siblings, 0 replies; 52+ messages in thread From: glider @ 2020-03-02 13:04 UTC (permalink / raw) To: tkjos, keescook, gregkh, arve, mingo Cc: devel, jannh, peterz, linux-kernel, Alexander Potapenko, dvyukov 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() 2020-03-02 13:04 ` glider @ 2020-03-02 16:56 ` Todd Kjos -1 siblings, 0 replies; 52+ messages in thread From: Todd Kjos @ 2020-03-02 16:56 UTC (permalink / raw) To: Alexander Potapenko Cc: keescook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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 > ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() @ 2020-03-02 16:56 ` Todd Kjos 0 siblings, 0 replies; 52+ messages in thread From: Todd Kjos @ 2020-03-02 16:56 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, keescook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() 2020-03-02 16:56 ` Todd Kjos @ 2020-03-02 18:03 ` Alexander Potapenko -1 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:03 UTC (permalink / raw) To: Todd Kjos Cc: Kees Cook, Greg Kroah-Hartman, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov, Jann Horn, open list:ANDROID DRIVERS, Peter Zijlstra, LKML 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. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() @ 2020-03-02 18:03 ` Alexander Potapenko 0 siblings, 0 replies; 52+ messages in thread From: Alexander Potapenko @ 2020-03-02 18:03 UTC (permalink / raw) To: Todd Kjos Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, Greg Kroah-Hartman, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() 2020-03-02 18:03 ` Alexander Potapenko @ 2020-03-02 18:39 ` Greg Kroah-Hartman -1 siblings, 0 replies; 52+ messages in thread From: Greg Kroah-Hartman @ 2020-03-02 18:39 UTC (permalink / raw) To: Alexander Potapenko Cc: Todd Kjos, open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() @ 2020-03-02 18:39 ` Greg Kroah-Hartman 0 siblings, 0 replies; 52+ messages in thread From: Greg Kroah-Hartman @ 2020-03-02 18:39 UTC (permalink / raw) To: Alexander Potapenko Cc: open list:ANDROID DRIVERS, Kees Cook, Jann Horn, Peter Zijlstra, LKML, Arve Hjønnevåg, Ingo Molnar, Dmitry Vyukov, Todd Kjos 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2020-03-06 2:29 UTC | newest] Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-02 13:04 [PATCH v2 1/3] compiler.h: define __no_initialize glider 2020-03-02 13:04 ` glider 2020-03-02 13:04 ` [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user() glider 2020-03-02 13:04 ` glider 2020-03-02 13:09 ` Joe Perches 2020-03-02 13:09 ` Joe Perches 2020-03-02 13:25 ` Alexander Potapenko 2020-03-02 13:25 ` Alexander Potapenko 2020-03-02 13:52 ` Dan Carpenter 2020-03-02 13:52 ` Dan Carpenter 2020-03-02 13:58 ` Joe Perches 2020-03-02 13:58 ` Joe Perches 2020-03-02 18:17 ` Alexander Potapenko 2020-03-02 18:17 ` Alexander Potapenko 2020-03-02 18:31 ` Jann Horn 2020-03-02 18:31 ` Jann Horn 2020-03-05 9:03 ` Rasmus Villemoes 2020-03-05 9:03 ` Rasmus Villemoes 2020-03-05 12:45 ` Jann Horn 2020-03-05 12:45 ` Jann Horn 2020-03-06 2:29 ` Al Viro 2020-03-06 2:29 ` Al Viro 2020-03-02 18:50 ` Joe Perches 2020-03-02 18:50 ` Joe Perches 2020-03-03 9:14 ` Alexander Potapenko 2020-03-03 9:14 ` Alexander Potapenko 2020-03-03 9:38 ` Dan Carpenter 2020-03-03 9:38 ` Dan Carpenter 2020-03-03 13:56 ` Joe Perches 2020-03-03 13:56 ` Joe Perches 2020-03-03 14:15 ` Dan Carpenter 2020-03-03 14:15 ` Dan Carpenter 2020-03-04 18:13 ` Kees Cook 2020-03-04 18:13 ` Kees Cook 2020-03-05 8:07 ` Dan Carpenter 2020-03-05 8:07 ` Dan Carpenter 2020-03-05 8:26 ` Kees Cook 2020-03-05 8:26 ` Kees Cook 2020-03-05 8:33 ` Alexander Potapenko 2020-03-05 8:33 ` Alexander Potapenko 2020-03-02 17:38 ` Greg KH 2020-03-02 17:38 ` Greg KH 2020-03-02 18:28 ` Alexander Potapenko 2020-03-02 18:28 ` Alexander Potapenko 2020-03-02 13:04 ` [PATCH v2 3/3] sched/wait: avoid double initialization in ___wait_event() glider 2020-03-02 13:04 ` glider 2020-03-02 16:56 ` Todd Kjos 2020-03-02 16:56 ` Todd Kjos 2020-03-02 18:03 ` Alexander Potapenko 2020-03-02 18:03 ` Alexander Potapenko 2020-03-02 18:39 ` Greg Kroah-Hartman 2020-03-02 18:39 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.