On Mon, Feb 24, 2020 at 04:35:00PM +0100, glider@xxxxxxxxxx wrote:
> Certain copy_from_user() invocations in binder.c are known to
> unconditionally initialize locals before their first use, like e.g. in
> the following case:
>
> struct binder_transaction_data tr;
> if (copy_from_user(&tr, ptr, sizeof(tr)))
> return -EFAULT;
>
> In such cases enabling CONFIG_INIT_STACK_ALL leads to insertion of
> redundant locals initialization that the compiler fails to remove.
> To work around this problem till Clang can deal with it, we apply
> __do_not_initialize to local Binder structures.
It should be possible to write a Coccinelle script to identify all these
cases. (i.e. a single path from struct declaration to copy_from_user())
and apply the changes automatically. This script could be checked into
scripts/coccinelle/ to help keep these markings in sync...
-Kees
>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx>
> ---
> drivers/android/binder.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index a6b2082c24f8f..3c91d842ac704 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3788,7 +3788,7 @@ static int binder_thread_write(struct binder_proc *proc,
>
> case BC_TRANSACTION_SG:
> case BC_REPLY_SG: {
> - struct binder_transaction_data_sg tr;
> + struct binder_transaction_data_sg tr __do_not_initialize;
>
> if (copy_from_user(&tr, ptr, sizeof(tr)))
> return -EFAULT;
> @@ -3799,7 +3799,7 @@ static int binder_thread_write(struct binder_proc *proc,
> }
> case BC_TRANSACTION:
> case BC_REPLY: {
> - struct binder_transaction_data tr;
> + struct binder_transaction_data tr __do_not_initialize;
>
> if (copy_from_user(&tr, ptr, sizeof(tr)))
> return -EFAULT;
> @@ -4827,7 +4827,7 @@ static int binder_ioctl_write_read(struct file *filp,
> struct binder_proc *proc = filp->private_data;
> unsigned int size = _IOC_SIZE(cmd);
> void __user *ubuf = (void __user *)arg;
> - struct binder_write_read bwr;
> + struct binder_write_read bwr __do_not_initialize;
>
> if (size != sizeof(struct binder_write_read)) {
> ret = -EINVAL;
> @@ -5039,7 +5039,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> break;
> }
> case BINDER_SET_CONTEXT_MGR_EXT: {
> - struct flat_binder_object fbo;
> + struct flat_binder_object fbo __do_not_initialize;
>
> if (copy_from_user(&fbo, ubuf, sizeof(fbo))) {
> ret = -EINVAL;
> @@ -5076,7 +5076,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> break;
> }
> case BINDER_GET_NODE_INFO_FOR_REF: {
> - struct binder_node_info_for_ref info;
> + struct binder_node_info_for_ref info __do_not_initialize;
>
> if (copy_from_user(&info, ubuf, sizeof(info))) {
> ret = -EFAULT;
> @@ -5095,7 +5095,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> break;
> }
> case BINDER_GET_NODE_DEBUG_INFO: {
> - struct binder_node_debug_info info;
> + struct binder_node_debug_info info __do_not_initialize;
>
> if (copy_from_user(&info, ubuf, sizeof(info))) {
> ret = -EFAULT;
> --
> 2.25.0.265.gbab2e86ba0-goog
>
--
Kees Cook
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
