linux-kernel - [PATCH v2 2/3] binder: do not initialize locals passed to copy_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200302130430.201037-2-glider@google.com>
Date:   Mon,  2 Mar 2020 14:04:29 +0100
From:   glider@...gle.com
To:     tkjos@...gle.com, keescook@...omium.org,
        gregkh@...uxfoundation.org, arve@...roid.com, mingo@...hat.com
Cc:     dvyukov@...gle.com, jannh@...gle.com, devel@...verdev.osuosl.org,
        peterz@...radead.org, linux-kernel@...r.kernel.org,
        Alexander Potapenko <glider@...gle.com>
Subject: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()

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@...omium.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Signed-off-by: Alexander Potapenko <glider@...gle.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