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

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

-Kees

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

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

Reply via email to