Skip to contentSkip to conversation

android: binder: check result of binder_get_thread() in binder_poll()

PATCH In review Dmitry Torokhov 4 participants 4 messages May 8, 2017 – Aug 23, 2017 Android drivers

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

PATCH 1/1 android: binder: check result of binder_get_thread() in binder_poll()

If binder_get_thread() fails to give us a thread data, we should avoid dereferencing a NULL pointer and return POLLERR instead.

Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/android/binder.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/android/binder.c +8 -4
index aae4d8d4be36..66ed714fedd5 100644
@@ -3103,18 +3103,22 @@ static unsigned int binder_poll(struct file *filp,
 				struct poll_table_struct *wait)
 {
 	struct binder_proc *proc = filp->private_data;
-	struct binder_thread *thread = NULL;
+	struct binder_thread *thread;
 	int wait_for_proc_work;
 
 	binder_lock(__func__);
 
 	thread = binder_get_thread(proc);
-
-	wait_for_proc_work = thread->transaction_stack == NULL &&
-		list_empty(&thread->todo) && thread->return_error == BR_OK;
+	if (thread)
+		wait_for_proc_work = thread->transaction_stack == NULL &&
+				     list_empty(&thread->todo) &&
+				     thread->return_error == BR_OK;
 
 	binder_unlock(__func__);
 
+	if (!thread)
+		return POLLERR;
+
 	if (wait_for_proc_work) {
 		if (binder_has_proc_work(proc, thread))
 			return POLLIN;
Signature · 5 lines
2.13.0.rc1.294.g07d810a77f-goog


-- 
Dmitry
#1 lore raw
Quoted 4 lines · Dmitry Torokhov

If binder_get_thread() fails to give us a thread data, we should avoid dereferencing a NULL pointer and return POLLERR instead.

Signed-off-by Dmitry Torokhov <[email protected]>

Pulling Todd Kjos in on this too.
-john

Quoted 41 lines
---
 drivers/android/binder.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/android/binder.c +8 -5
index aae4d8d4be36..66ed714fedd5 100644
@@ -3103,18 +3103,22 @@ static unsigned int binder_poll(struct file *filp,
                                struct poll_table_struct *wait)
 {
        struct binder_proc *proc = filp->private_data;
-       struct binder_thread *thread = NULL;
+       struct binder_thread *thread;
        int wait_for_proc_work;
 
        binder_lock(__func__);
 
        thread = binder_get_thread(proc);
-
-       wait_for_proc_work = thread->transaction_stack == NULL &&
-               list_empty(&thread->todo) && thread->return_error == BR_OK;
+       if (thread)
+               wait_for_proc_work = thread->transaction_stack == NULL &&
+                                    list_empty(&thread->todo) &&
+                                    thread->return_error == BR_OK;
 
        binder_unlock(__func__);
 
+       if (!thread)
+               return POLLERR;
+
        if (wait_for_proc_work) {
                if (binder_has_proc_work(proc, thread))
                        return POLLIN;
--

2.13.0.rc1.294.g07d810a77f-goog

--
Dmitry

Reviewed-by
#2 lore raw

Dmitry,

Quoted 39 lines · John Stultz
Quoted 4 lines · Dmitry Torokhov

If binder_get_thread() fails to give us a thread data, we should avoid dereferencing a NULL pointer and return POLLERR instead.

Signed-off-by Dmitry Torokhov <[email protected]>

Pulling Todd Kjos in on this too.
-john

Quoted 29 lines
---
 drivers/android/binder.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/android/binder.c +5 -4
index aae4d8d4be36..66ed714fedd5 100644
@@ -3103,18 +3103,22 @@ static unsigned int binder_poll(struct file *filp,
                                struct poll_table_struct *wait)
 {
        struct binder_proc *proc = filp->private_data;
-       struct binder_thread *thread = NULL;
+       struct binder_thread *thread;
        int wait_for_proc_work;
 
        binder_lock(__func__);
 
        thread = binder_get_thread(proc);
-
-       wait_for_proc_work = thread->transaction_stack == NULL &&
-               list_empty(&thread->todo) && thread->return_error == BR_OK;
+       if (thread)
+               wait_for_proc_work = thread->transaction_stack == NULL &&
+                                    list_empty(&thread->todo) &&
+                                    thread->return_error == BR_OK;
 
        binder_unlock(__func__);
Quoted 7 lines

drivers/android/binder.c static unsigned int binder_poll(struct file *filp,

Quoted 7 lines
drivers/android/binder.c +3 -1
+       if (!thread)
+               return POLLERR;
+
        if (wait_for_proc_work) {
                if (binder_has_proc_work(proc, thread))
                        return POLLIN;
--

I'm no expert on the poll function, but I agree that it's wise to check the result of binder_get_thread() since it can return NULL. FWIW:

Reviewed-by Douglas Anderson <[email protected]>
maintainer
#3 lore raw
In reply to Dmitry Torokhov
Quoted 8 lines · Dmitry Torokhov

If binder_get_thread() fails to give us a thread data, we should avoid dereferencing a NULL pointer and return POLLERR instead.

Signed-off-by Dmitry Torokhov <[email protected]>
Reviewed-by Douglas Anderson <[email protected]>
---
 drivers/android/binder.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Doesn't apply to my tree at all :(

Please rebase and resend, thanks.

greg k-h