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(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9f1000d2a40c7..9f2059d24ae2d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 030c98f35cca7..022cd80e80cc3 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -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,
--
2.20.0.rc1.387.gf8505762e3-goog
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(-)
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index fb3238c74c8a8..c0aadbbf7f193 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -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
*/
--
2.20.0.rc1.387.gf8505762e3-goog
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(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9f2059d24ae2d..c525b164d39d2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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
--
2.20.0.rc1.387.gf8505762e3-goog
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(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c525b164d39d2..cbaef3b0d3078 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -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;
}
--
2.20.0.rc1.387.gf8505762e3-goog
On Mon, Dec 03, 2018 at 12:24:54PM -0800, Todd Kjos wrote:
> 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(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 9f1000d2a40c7..9f2059d24ae2d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -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);
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
On Mon, Dec 03, 2018 at 12:24:57PM -0800, Todd Kjos wrote:
> 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]>
Shouldn't this go to 4.20-final? And have a stable@ tag?
And a "Fixes:" tag?
thanks,
greg k-h
On Mon, Dec 03, 2018 at 12:24:54PM -0800, Todd Kjos wrote:
> 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
On Mon, Dec 03, 2018 at 12:24:55PM -0800, Todd Kjos wrote:
> Fix the incomplete kerneldoc header for struct binder_buffer.
>
> Change-Id: If3ca10cf6d90f605a0c078e4cdce28f02a475877
No need for this here :)
On Wed, Dec 05, 2018 at 06:02:22AM -0800, Todd Kjos wrote:
> On Wed, Dec 5, 2018 at 2:57 AM Greg KH <[email protected]> wrote:
>
> > On Mon, Dec 03, 2018 at 12:24:54PM -0800, Todd Kjos wrote:
> > > 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?
> >
>
> I didn't number them because they are independent and can go in any order.
Ah, that wasn't obvious :)
> 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