lkml.org 
[lkml]   [2020]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
Subject[PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()
From
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
\
 
 \ /
  Last update: 2020-03-02 14:05    [W:0.101 / U:3.992 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site