binder: fix sparse warnings on locking context

PATCH Superseded Todd Kjos 3 participants 9 messages Dec 3, 2018 – Dec 5, 2018 1 message not in the archive Android drivers

lore

Share & tools

lore

Todd Kjos

author maintainer

2018-12-03 20:26 UTC #0 lore raw

PATCH 1/1 binder: fix sparse warnings on locking context

Add __acquire()/__release() annnotations to fix warnings in sparse context checking

There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.

Signed-off-by Todd Kjos <[email protected]>

---
 drivers/android/binder.c       | 43 +++++++++++++++++++++++++++++++++-
 drivers/android/binder_alloc.c |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

drivers/android/binder.c +42 -1

index 9f1000d2a40c7..9f2059d24ae2d 100644
@@ -660,6 +660,7 @@ struct binder_transaction {
 #define binder_proc_lock(proc) _binder_proc_lock(proc, __LINE__)
 static void
 _binder_proc_lock(struct binder_proc *proc, int line)
+	__acquires(&proc->outer_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -675,6 +676,7 @@ _binder_proc_lock(struct binder_proc *proc, int line)
 #define binder_proc_unlock(_proc) _binder_proc_unlock(_proc, __LINE__)
 static void
 _binder_proc_unlock(struct binder_proc *proc, int line)
+	__releases(&proc->outer_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -690,6 +692,7 @@ _binder_proc_unlock(struct binder_proc *proc, int line)
 #define binder_inner_proc_lock(proc) _binder_inner_proc_lock(proc, __LINE__)
 static void
 _binder_inner_proc_lock(struct binder_proc *proc, int line)
+	__acquires(&proc->inner_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -705,6 +708,7 @@ _binder_inner_proc_lock(struct binder_proc *proc, int line)
 #define binder_inner_proc_unlock(proc) _binder_inner_proc_unlock(proc, __LINE__)
 static void
 _binder_inner_proc_unlock(struct binder_proc *proc, int line)
+	__releases(&proc->inner_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -720,6 +724,7 @@ _binder_inner_proc_unlock(struct binder_proc *proc, int line)
 #define binder_node_lock(node) _binder_node_lock(node, __LINE__)
 static void
 _binder_node_lock(struct binder_node *node, int line)
+	__acquires(&node->lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -735,6 +740,7 @@ _binder_node_lock(struct binder_node *node, int line)
 #define binder_node_unlock(node) _binder_node_unlock(node, __LINE__)
 static void
 _binder_node_unlock(struct binder_node *node, int line)
+	__releases(&node->lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);

@@ -751,12 +757,16 @@ _binder_node_unlock(struct binder_node *node, int line)
 #define binder_node_inner_lock(node) _binder_node_inner_lock(node, __LINE__)
 static void
 _binder_node_inner_lock(struct binder_node *node, int line)
+	__acquires(&node->lock) __acquires(&node->proc->inner_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);
 	spin_lock(&node->lock);
 	if (node->proc)
 		binder_inner_proc_lock(node->proc);
+	else
+		/* annotation for sparse */
+		__acquire(&node->proc->inner_lock);
 }
 
 /**

@@ -768,6 +778,7 @@ _binder_node_inner_lock(struct binder_node *node, int line)
 #define binder_node_inner_unlock(node) _binder_node_inner_unlock(node, __LINE__)
 static void
 _binder_node_inner_unlock(struct binder_node *node, int line)
+	__releases(&node->lock) __releases(&node->proc->inner_lock)
 {
 	struct binder_proc *proc = node->proc;

@@ -775,6 +786,9 @@ _binder_node_inner_unlock(struct binder_node *node, int line)
 		     "%s: line=%d\n", __func__, line);
 	if (proc)
 		binder_inner_proc_unlock(proc);
+	else
+		/* annotation for sparse */
+		__release(&node->proc->inner_lock);
 	spin_unlock(&node->lock);
 }

@@ -1384,10 +1398,14 @@ static void binder_dec_node_tmpref(struct binder_node *node)
 	binder_node_inner_lock(node);
 	if (!node->proc)
 		spin_lock(&binder_dead_nodes_lock);
+	else
+		__acquire(&binder_dead_nodes_lock);
 	node->tmp_refs--;
 	BUG_ON(node->tmp_refs < 0);
 	if (!node->proc)
 		spin_unlock(&binder_dead_nodes_lock);
+	else
+		__release(&binder_dead_nodes_lock);
 	/*
 	 * Call binder_dec_node() to check if all refcounts are 0
 	 * and cleanup is needed. Calling with strong=0 and internal=1

@@ -1890,18 +1908,22 @@ static struct binder_thread *binder_get_txn_from(
  */
 static struct binder_thread *binder_get_txn_from_and_acq_inner(
 		struct binder_transaction *t)
+	__acquires(&t->from->proc->inner_lock)
 {
 	struct binder_thread *from;
 
 	from = binder_get_txn_from(t);
-	if (!from)
+	if (!from) {
+		__acquire(&from->proc->inner_lock);
 		return NULL;
+	}
 	binder_inner_proc_lock(from->proc);
 	if (t->from) {
 		BUG_ON(from != t->from);
 		return from;
 	}
 	binder_inner_proc_unlock(from->proc);
+	__acquire(&from->proc->inner_lock);
 	binder_thread_dec_tmpref(from);
 	return NULL;
 }

@@ -1973,6 +1995,8 @@ static void binder_send_failed_reply(struct binder_transaction *t,
 			binder_thread_dec_tmpref(target_thread);
 			binder_free_transaction(t);
 			return;
+		} else {
+			__release(&target_thread->proc->inner_lock);
 		}
 		next = t->from_parent;

@@ -2394,11 +2418,15 @@ static int binder_translate_handle(struct flat_binder_object *fp,
 		fp->cookie = node->cookie;
 		if (node->proc)
 			binder_inner_proc_lock(node->proc);
+		else
+			__acquire(&node->proc->inner_lock);
 		binder_inc_node_nilocked(node,
 					 fp->hdr.type == BINDER_TYPE_BINDER,
 					 0, NULL);
 		if (node->proc)
 			binder_inner_proc_unlock(node->proc);
+		else
+			__release(&node->proc->inner_lock);
 		trace_binder_transaction_ref_to_node(t, node, &src_rdata);
 		binder_debug(BINDER_DEBUG_TRANSACTION,
 			     "        ref %d desc %d -> node %d u%016llx\n",

@@ -2762,6 +2790,8 @@ static void binder_transaction(struct binder_proc *proc,
 		binder_set_nice(in_reply_to->saved_priority);
 		target_thread = binder_get_txn_from_and_acq_inner(in_reply_to);
 		if (target_thread == NULL) {
+			/* annotation for sparse */
+			__release(&target_thread->proc->inner_lock);
 			return_error = BR_DEAD_REPLY;
 			return_error_line = __LINE__;
 			goto err_dead_binder;

@@ -4164,6 +4194,11 @@ static int binder_thread_read(struct binder_proc *proc,
 			if (cmd == BR_DEAD_BINDER)
 				goto done; /* DEAD_BINDER notifications can cause transactions */
 		} break;
+		default:
+			binder_inner_proc_unlock(proc);
+			pr_err("%d:%d: bad work type %d\n",
+			       proc->pid, thread->pid, w->type);
+			break;
 		}
 
 		if (!t)

@@ -4467,6 +4502,8 @@ static int binder_thread_release(struct binder_proc *proc,
 		spin_lock(&t->lock);
 		if (t->to_thread == thread)
 			send_reply = t;
+	} else {
+		__acquire(&t->lock);
 	}
 	thread->is_dead = true;

@@ -4495,7 +4532,11 @@ static int binder_thread_release(struct binder_proc *proc,
 		spin_unlock(&last_t->lock);
 		if (t)
 			spin_lock(&t->lock);
+		else
+			__acquire(&t->lock);
 	}
+	/* annotation for sparse, lock not acquired in last iteration above */
+	__release(&t->lock);
 
 	/*
 	 * If this thread used poll, make sure we remove the waitqueue

drivers/android/binder_alloc.c +1 -0

index 030c98f35cca7..022cd80e80cc3 100644
@@ -939,6 +939,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,
 				       struct list_lru_one *lru,
 				       spinlock_t *lock,
 				       void *cb_arg)
+	__must_hold(lock)
 {
 	struct mm_struct *mm = NULL;
 	struct binder_lru_page *page = container_of(item,

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Todd Kjos

author maintainer

2018-12-03 20:26 UTC #1 lore raw

PATCH 1/1 binder: fix kerneldoc header for struct binder_buffer

Fix the incomplete kerneldoc header for struct binder_buffer.

Change-Id If3ca10cf6d90f605a0c078e4cdce28f02a475877

Signed-off-by Todd Kjos <[email protected]>

---
 drivers/android/binder_alloc.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/android/binder_alloc.h +10 ~~-10~~

index fb3238c74c8a8..c0aadbbf7f193 100644
@@ -30,16 +30,16 @@ struct binder_transaction;
  * struct binder_buffer - buffer used for binder transactions
  * @entry:              entry alloc->buffers
  * @rb_node:            node for allocated_buffers/free_buffers rb trees
- * @free:               true if buffer is free
- * @allow_user_free:    describe the second member of struct blah,
- * @async_transaction:  describe the second member of struct blah,
- * @debug_id:           describe the second member of struct blah,
- * @transaction:        describe the second member of struct blah,
- * @target_node:        describe the second member of struct blah,
- * @data_size:          describe the second member of struct blah,
- * @offsets_size:       describe the second member of struct blah,
- * @extra_buffers_size: describe the second member of struct blah,
- * @data:i              describe the second member of struct blah,
+ * @free:               %true if buffer is free
+ * @allow_user_free:    %true if user is allowed to free buffer
+ * @async_transaction:  %true if buffer is in use for an async txn
+ * @debug_id:           unique ID for debugging
+ * @transaction:        pointer to associated struct binder_transaction
+ * @target_node:        struct binder_node associated with this buffer
+ * @data_size:          size of @transaction data
+ * @offsets_size:       size of array of offsets
+ * @extra_buffers_size: size of space for other objects (like sg lists)
+ * @data:               pointer to base of buffer space
  *
  * Bookkeeping structure for binder transaction buffers
  */

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Greg KH

maintainer

2018-12-05 10:58 UTC #2 lore raw

Re: [PATCH] binder: fix kerneldoc header for struct binder_buffer

Quoted 3 lines · Todd Kjos

Fix the incomplete kerneldoc header for struct binder_buffer.
Change-Id: If3ca10cf6d90f605a0c078e4cdce28f02a475877

No need for this here :)

Todd Kjos

author maintainer

2018-12-03 20:26 UTC #3 lore raw

PATCH 1/1 binder: filter out nodes when showing binder procs

In reply to Todd Kjos

When dumping out binder transactions via a debug node, the output is too verbose if a process has many nodes. Change the output for transaction dumps to only display nodes with pending async transactions.

Signed-off-by Todd Kjos <[email protected]>

---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/android/binder.c +3 -0

index 9f2059d24ae2d..c525b164d39d2 100644
@@ -5432,6 +5432,9 @@ static void print_binder_proc(struct seq_file *m,
 	for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n)) {
 		struct binder_node *node = rb_entry(n, struct binder_node,
 						    rb_node);
+		if (!print_all && !node->has_async_transaction)
+			continue;
+
 		/*
 		 * take a temporary reference on the node so it
 		 * survives and isn't removed from the tree

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Todd Kjos

author maintainer

2018-12-03 20:26 UTC #4 lore raw

PATCH 1/1 binder: fix use-after-free due to fdget() optimization

In reply to Todd Kjos

44d8047f1d87a ("binder: use standard functions to allocate fds") exposed a pre-existing issue in the binder driver.

fdget() is used in ksys_ioctl() as a performance optimization. One of the rules associated with fdget() is that ksys_close() must not be called between the fdget() and the fdput(). There is a case where this requirement is not met in the binder driver (and possibly other drivers) which results in the reference count dropping to 0 when the device is still in use. This can result in use-after-free or other issues.

This was observed with the following sequence of events:

Task A and task B are connected via binder; task A has /dev/binder open at file descriptor number X. Both tasks are single-threaded.

1. task B sends a binder message with a file descriptor array (BINDER_TYPE_FDA) containing one file descriptor to task A
2. task A reads the binder message with the translated file descriptor number Y
3. task A uses dup2(X, Y) to overwrite file descriptor Y with the /dev/binder file
4. task A unmaps the userspace binder memory mapping; the reference count on task A's /dev/binder is now 2
5. task A closes file descriptor X; the reference count on task A's /dev/binder is now 1
6. task A forks off a child, task C, duplicating the file descriptor table; the reference count on task A's /dev/binder is now 2
7. task A invokes the BC_FREE_BUFFER command on file descriptor X to release the incoming binder message
8. fdget() in ksys_ioctl() suppresses the reference count increment, since the file descriptor table is not shared
9. the BC_FREE_BUFFER handler removes the file descriptor table entry for X and decrements the reference count of task A's /dev/binder file to 1
10.task C calls close(X), which drops the reference count of
task A's /dev/binder to 0 and frees it
11.task A continues processing of the ioctl and accesses some
property of e.g. the binder_proc => KASAN-detectable UAF

Fixed by using get_file() / fput() in binder_ioctl().

Suggested-by Jann Horn <[email protected]>

Signed-off-by Todd Kjos <[email protected]>

Acked-by Martijn Coenen <[email protected]>

---
 drivers/android/binder.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/android/binder.c +8 -0

index c525b164d39d2..cbaef3b0d3078 100644
@@ -4774,6 +4774,13 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	unsigned int size = _IOC_SIZE(cmd);
 	void __user *ubuf = (void __user *)arg;
 
+	/*
+	 * Need a reference on filp since ksys_close() could
+	 * be called on binder fd and the fdget() used in
+	 * ksys_ioctl() might have optimized out the reference.
+	 */
+	get_file(filp);
+
 	/*pr_info("binder_ioctl: %d:%d %x %lx\n",
 			proc->pid, current->pid, cmd, arg);*/

@@ -4885,6 +4892,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
 err_unlocked:
 	trace_binder_ioctl_done(ret);
+	fput(filp);
 	return ret;
 }

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Greg KH

maintainer

2018-12-05 10:58 UTC #5 lore raw

Re: [PATCH] binder: fix use-after-free due to fdget() optimization

Quoted 40 lines · Todd Kjos

44d8047f1d87a ("binder: use standard functions to allocate fds") exposed a pre-existing issue in the binder driver.
fdget() is used in ksys_ioctl() as a performance optimization. One of the rules associated with fdget() is that ksys_close() must not be called between the fdget() and the fdput(). There is a case where this requirement is not met in the binder driver (and possibly other drivers) which results in the reference count dropping to 0 when the device is still in use. This can result in use-after-free or other issues.
This was observed with the following sequence of events:
Task A and task B are connected via binder; task A has /dev/binder open at file descriptor number X. Both tasks are single-threaded.
1. task B sends a binder message with a file descriptor array (BINDER_TYPE_FDA) containing one file descriptor to task A
2. task A reads the binder message with the translated file descriptor number Y
3. task A uses dup2(X, Y) to overwrite file descriptor Y with the /dev/binder file
4. task A unmaps the userspace binder memory mapping; the reference count on task A's /dev/binder is now 2
5. task A closes file descriptor X; the reference count on task A's /dev/binder is now 1
6. task A forks off a child, task C, duplicating the file descriptor table; the reference count on task A's /dev/binder is now 2
7. task A invokes the BC_FREE_BUFFER command on file descriptor X to release the incoming binder message
8. fdget() in ksys_ioctl() suppresses the reference count increment, since the file descriptor table is not shared
9. the BC_FREE_BUFFER handler removes the file descriptor table entry for X and decrements the reference count of task A's /dev/binder file to 1
10.task C calls close(X), which drops the reference count of
task A's /dev/binder to 0 and frees it
11.task A continues processing of the ioctl and accesses some
property of e.g. the binder_proc => KASAN-detectable UAF

Quoted 5 lines

Fixed by using get_file() / fput() in binder_ioctl().
Suggested-by Jann Horn <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
Acked-by Martijn Coenen <[email protected]>

Shouldn't this go to 4.20-final? And have a stable@ tag? And a "Fixes:" tag?

thanks,

greg k-h

Luc Van Oostenryck

2018-12-03 22:29 UTC #6 lore raw

In reply to Todd Kjos

Quoted 22 lines · Todd Kjos

Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c       | 43 +++++++++++++++++++++++++++++++++-
 drivers/android/binder_alloc.c |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)
drivers/android/binder.c +0 -0
index 9f1000d2a40c7..9f2059d24ae2d 100644
@@ -751,12 +757,16 @@ _binder_node_unlock(struct binder_node *node, int line)
 #define binder_node_inner_lock(node) _binder_node_inner_lock(node, __LINE__)
 static void
 _binder_node_inner_lock(struct binder_node *node, int line)

drivers/android/binder.c _binder_node_unlock(struct binder_node *node, int line) · line 769

Quoted 10 lines

drivers/android/binder.c +4 -0

+	__acquires(&node->lock) __acquires(&node->proc->inner_lock)
 {
 	binder_debug(BINDER_DEBUG_SPINLOCKS,
 		     "%s: line=%d\n", __func__, line);
 	spin_lock(&node->lock);
 	if (node->proc)
 		binder_inner_proc_lock(node->proc);
+	else
+		/* annotation for sparse */
+		__acquire(&node->proc->inner_lock);

This one is questionnable because:
1) if !node->proc, then '&node->proc->inner_lock' is not acquired since it doesn't even exist.
2) OTOH, the function can't have the annotation 100% right because it semantics allows unbalanced locking depending on node->proc being null or not.
But I see very well the intent and maybe it's a right solution. I dunno.

Same for most of the following ones.

Best regards,
-- Luc Van Oostenryck

Greg KH

maintainer

2018-12-05 10:57 UTC #7 lore raw

In reply to Todd Kjos

Quoted 9 lines · Todd Kjos

Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Signed-off-by Todd Kjos <[email protected]>

You sent out 4 patches here, as a series, but with no numbering so I don't know what order to put them in :(

Can you resend them properly numbered so I have a chance to get it right?

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): CAHRSSEyj8xsJk76r5Rinmor9szYHaca5nLATWgDvt4Vkq-NtQw@mail.gmail.com

Greg Kroah-Hartman

maintainer

2018-12-05 16:29 UTC #9 lore raw

Quoted 13 lines · Todd Kjos

Quoted 11 lines · Greg KH
Quoted 9 lines · Todd Kjos
Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Signed-off-by Todd Kjos <[email protected]>

Quoted 8 lines

Quoted 6 lines
You sent out 4 patches here, as a series, but with no numbering so I don't know what order to put them in :(
Can you resend them properly numbered so I have a chance to get it right?
I didn't number them because they are independent and can go in any order.

Ah, that wasn't obvious :)

Quoted 2 lines

They are not really a series. I can resend with numbers though if you want to reflect the order I sent them in.

Ok, no need for numbering them, but they do need to be resent based on the feedback I gave. I've dropped them all from my queue because of that.

thanks,

greg k-h