binder: fix memory corruption in binder_transaction binder

PATCH In review Todd Kjos 5 participants 8 messages Sep 5, 2017 – Oct 3, 2017 1 message not in the archive Android drivers

lore

Share & tools

lore

Todd Kjos

author maintainer

2017-09-05 17:22 UTC #0 lore raw

PATCH 1/1 binder: fix memory corruption in binder_transaction binder

From: Xu YiPing <[email protected]>

commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.

So, dequeue it before freeing.

Signed-off-by Xu YiPing <[email protected]>

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

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

drivers/android/binder.c +1 -0

index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc *proc,
 err_dead_proc_or_thread:
 	return_error = BR_DEAD_REPLY;
 	return_error_line = __LINE__;
+	binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
 err_bad_offset:

Signature · 1 line

2.14.1.581.gf28d330327-goog

Amit Pundir

2017-09-11 12:19 UTC #1 lore raw

Quoted 9 lines · Todd Kjos

From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.

I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.

It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.

Regards,
Amit Pundir

Quoted 22 lines

Signed-off-by Xu YiPing <[email protected]>

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

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

drivers/android/binder.c +1 -1

index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
 err_bad_offset:
--

2.14.1.581.gf28d330327-goog

A message that is not in the archive (sent off-list or not yet received): CAHRSSEwVSHSoYJc8JEaAkNwG+v5Fgaq4sO7GZ7otHzyv9m+qFg@mail.gmail.com

Todd Kjos

2017-09-11 15:40 UTC #3 lore raw

(resend in plain-text mode -- sorry about that)

Amit,

Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.

I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).

-Todd

Quoted 48 lines · Amit Pundir

Quoted 45 lines · Todd Kjos
Quoted 9 lines
From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.
I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.
It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.
Regards,
Amit Pundir
Quoted 23 lines
Signed-off-by Xu YiPing <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder.c +1 -0
index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc
 *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
err_bad_offset:
--
2.14.1.581.gf28d330327-goog

Amit Pundir

2017-09-11 16:55 UTC #4 lore raw

Hi Todd,

Quoted 12 lines · Todd Kjos

(resend in plain-text mode -- sorry about that)
Amit,
Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.

Quoted 2 lines

I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).

Sorry for not being clear enough in the bug report. android-4.9 is fine, I see this issue on linux mainline tree with this patch.

I can reproduce it on John's minimal Android tree for hikey hosted
here https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/hikey-mainline-WIP
and hikey-llct (android-4.9 patchset rebased to mainline) tree hosted
here https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I have already reverted this patch in hikey-llct so you have to revert
that revert to reproduce this issue on hikey-llct tree.

Regards,
Amit Pundir

Quoted 51 lines

-Todd
Quoted 48 lines · Amit Pundir
Quoted 45 lines · Todd Kjos
Quoted 9 lines
From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.
I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.
It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.
Regards,
Amit Pundir
Quoted 23 lines
Signed-off-by Xu YiPing <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder.c +1 -0
index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc
 *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
err_bad_offset:
--
2.14.1.581.gf28d330327-goog

Greg Kroah-Hartman

maintainer

2017-09-11 17:24 UTC #5 lore raw

Quoted 15 lines · Amit Pundir

Hi Todd,
Quoted 12 lines · Todd Kjos
(resend in plain-text mode -- sorry about that)
Amit,
Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.

Quoted 5 lines

Quoted 2 lines
I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).
Sorry for not being clear enough in the bug report. android-4.9 is fine, I see this issue on linux mainline tree with this patch.

What exact kernel release? A number of binder fixes have recently landed in the stable trees, and in Linus's tree.

thanks,

greg k-h

Todd Kjos

2017-09-11 19:59 UTC #6 lore raw

In reply to Amit Pundir

Amit,

I tested with https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I added a pr_info() above the patch's single line change and in
binder_init (so I could easily prove that I was running the correct
kernel).

First I did 10 reboots with the patch. I saw one failure to reach the Android home screen in boot #7 (but the new line of code was never reached, so the patch cannot be the cause)... so 9 out of 10 reboots were fine and the failure does not point to this patch.

Then I did 10 reboots without the patch. No failures.

Then 10 more with the patch. No failures.

Then with the patch: power-on, reboot twice, no failures (repeat, no failures).

I think the issue you are seeing cannot be caused by this patch -- take a look at it and see if you think its really possible...

-Todd

Quoted 82 lines · Amit Pundir

Hi Todd,
Quoted 12 lines · Todd Kjos
(resend in plain-text mode -- sorry about that)
Amit,
Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.
Quoted 2 lines
I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).
Sorry for not being clear enough in the bug report. android-4.9 is fine, I see this issue on linux mainline tree with this patch.
I can reproduce it on John's minimal Android tree for hikey hosted
here https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/hikey-mainline-WIP
and hikey-llct (android-4.9 patchset rebased to mainline) tree hosted
here https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I have already reverted this patch in hikey-llct so you have to revert
that revert to reproduce this issue on hikey-llct tree.
Regards,
Amit Pundir
Quoted 51 lines
-Todd
Quoted 48 lines · Amit Pundir
Quoted 45 lines · Todd Kjos
Quoted 9 lines
From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.
I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.
It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.
Regards,
Amit Pundir
Quoted 23 lines
Signed-off-by Xu YiPing <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder.c +1 -0
index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc
 *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
err_bad_offset:
--
2.14.1.581.gf28d330327-goog

Martijn Coenen

2017-09-12 08:20 UTC #7 lore raw

Hi Amit,

Can you try with the patch I sent to LKML recently, "[PATCH v2 10/13]
ANDROID: binder: call poll_wait() unconditionally."? This fixes a problem in binder's poll() implementation that only causes issues under certain racy conditions. I'm not sure why it would only trigger now, as this problem has always been there, but perhaps my patches to remove the proc waitqueue (which were merged recently) have exacerbated this problem.

Thanks,
Martijn

Quoted 106 lines · Todd Kjos

Amit,
I tested with https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I added a pr_info() above the patch's single line change and in
binder_init (so I could easily prove that I was running the correct
kernel).
First I did 10 reboots with the patch. I saw one failure to reach the Android home screen in boot #7 (but the new line of code was never reached, so the patch cannot be the cause)... so 9 out of 10 reboots were fine and the failure does not point to this patch.
Then I did 10 reboots without the patch. No failures.
Then 10 more with the patch. No failures.
Then with the patch: power-on, reboot twice, no failures (repeat, no failures).
I think the issue you are seeing cannot be caused by this patch -- take a look at it and see if you think its really possible...
-Todd
Quoted 82 lines · Amit Pundir
Hi Todd,
Quoted 12 lines · Todd Kjos
(resend in plain-text mode -- sorry about that)
Amit,
Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.
Quoted 2 lines
I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).
Sorry for not being clear enough in the bug report. android-4.9 is fine, I see this issue on linux mainline tree with this patch.
I can reproduce it on John's minimal Android tree for hikey hosted
here https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/hikey-mainline-WIP
and hikey-llct (android-4.9 patchset rebased to mainline) tree hosted
here https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I have already reverted this patch in hikey-llct so you have to revert
that revert to reproduce this issue on hikey-llct tree.
Regards,
Amit Pundir
Quoted 51 lines
-Todd
Quoted 48 lines · Amit Pundir
Quoted 45 lines · Todd Kjos
Quoted 9 lines
From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.
I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.
It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.
Regards,
Amit Pundir
Quoted 23 lines
Signed-off-by Xu YiPing <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder.c +1 -0
index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc
 *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
err_bad_offset:
--
2.14.1.581.gf28d330327-goog

Amit Pundir

2017-10-03 08:46 UTC #8 lore raw

Hi,

Quoted 10 lines · Martijn Coenen

Hi Amit,
Can you try with the patch I sent to LKML recently, "[PATCH v2 10/13]
ANDROID: binder: call poll_wait() unconditionally."? This fixes a problem in binder's poll() implementation that only causes issues under certain racy conditions. I'm not sure why it would only trigger now, as this problem has always been there, but perhaps my patches to remove the proc waitqueue (which were merged recently) have exacerbated this problem.

Sorry took me a while to get back to test this patch again. I didn't try your binder poll fix yet. I can not reproduce this problem on hikey anyway, running 4.14-rc3 on latest AOSP(rootfs) master snapshot.

It could be my older AOSP rootfs snapshot which was running into that random system crash on boot, causing boot animation loop. I've been bitten by such intermittent AOSP issues before.

I also ran binder tests from frameworks/native/libs/binder/tests/ to be sure and found the results mostly inline with android-4.9 kernel. Sorry for all the noise.

Regards,
Amit Pundir

Quoted 110 lines

Thanks,
Martijn
Quoted 106 lines · Todd Kjos
Amit,
I tested with https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I added a pr_info() above the patch's single line change and in
binder_init (so I could easily prove that I was running the correct
kernel).
First I did 10 reboots with the patch. I saw one failure to reach the Android home screen in boot #7 (but the new line of code was never reached, so the patch cannot be the cause)... so 9 out of 10 reboots were fine and the failure does not point to this patch.
Then I did 10 reboots without the patch. No failures.
Then 10 more with the patch. No failures.
Then with the patch: power-on, reboot twice, no failures (repeat, no failures).
I think the issue you are seeing cannot be caused by this patch -- take a look at it and see if you think its really possible...
-Todd
Quoted 82 lines · Amit Pundir
Hi Todd,
Quoted 12 lines · Todd Kjos
(resend in plain-text mode -- sorry about that)
Amit,
Are you sure this patch is the culprit? That is pretty surprising since this change can only be hit in a uncommon case (the target node is valid when we start creating the transaction, but dead when we check right before sending it) so it is unlikely to be hit during a normal boot. It also fixes a corruption -- so if you were actually hitting the case, it would likely have caused issues before and not now. Take a look at it and see if you think it is really possible.
Quoted 2 lines
I just booted hikey to Android with this patch 10 times in a row with no issues (used hikey-linaro 4.9 kernel which has this patch).
Sorry for not being clear enough in the bug report. android-4.9 is fine, I see this issue on linux mainline tree with this patch.
I can reproduce it on John's minimal Android tree for hikey hosted
here https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/hikey-mainline-WIP
and hikey-llct (android-4.9 patchset rebased to mainline) tree hosted
here https://android-git.linaro.org/kernel/linaro-android.git/log/?h=test/hikey-llct.
I have already reverted this patch in hikey-llct so you have to revert
that revert to reproduce this issue on hikey-llct tree.
Regards,
Amit Pundir
Quoted 51 lines
-Todd
Quoted 48 lines · Amit Pundir
Quoted 45 lines · Todd Kjos
Quoted 9 lines
From: Xu YiPing <[email protected]>
commit 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe") made a change to enqueue tcomplete to thread->todo before enqueuing the transaction. However, in err_dead_proc_or_thread case, the tcomplete is directly freed, without dequeued. It may cause the thread->todo list to be corrupted.
So, dequeue it before freeing.
I see Android boot loops with this patch on hikey tracking linux/master branch. 1st boot is fine but hikey runs into an unexpected short boot loops on 2nd and successive boots.
It takes about 3-4 iterations to finally come to sane state and boot to UI. I don't see this behaviour if I revert this patch.
Regards,
Amit Pundir
Quoted 23 lines
Signed-off-by Xu YiPing <[email protected]>
Signed-off-by Todd Kjos <[email protected]>
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/android/binder.c +1 -0
index d055b3f2a207..96cc28afa383 100644
@@ -3083,6 +3083,7 @@ static void binder_transaction(struct binder_proc
 *proc,
 err_dead_proc_or_thread:
        return_error = BR_DEAD_REPLY;
        return_error_line = __LINE__;
+       binder_dequeue_work(proc, tcomplete);
 err_translate_failed:
 err_bad_object_type:
err_bad_offset:
--
2.14.1.581.gf28d330327-goog