Re: [PATCH] rust_binder: call set_notification_done() without proc lock

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:

> Consider the following sequence of events on a death listener:
> 1. The remote process dies and sends a BR_DEAD_BINDER message.
> 2. The local process invokes the BC_CLEAR_DEATH_NOTIFICATION command.
> 3. The local process then invokes the BC_DEAD_BINDER_DONE.
> Then, the kernel will reply to the BC_DEAD_BINDER_DONE command with a
> BR_CLEAR_DEATH_NOTIFICATION_DONE reply using push_work_if_looper().
>
> However, this can result in a deadlock if the current thread is not a
> looper. This is because dead_binder_done() still holds the proc lock
> during set_notification_done(), which called push_work_if_looper().
> Normally, push_work_if_looper() takes the thread lock, which is fine to
> take under the proc lock. But if the current thread is not a looper,
> then it falls back to delivering the reply to the process work queue,
> which involves taking the proc lock. Since the proc lock is already
> held, this is a deadlock.
>
> Fix this by releasing the proc lock during set_notification_done(). It
> was not intentional that it was held during that function to begin with.
>
> I don't think this ever happens in Android because BC_DEAD_BINDER_DONE
> is only invoked in response to BR_DEAD_BINDER messages, and the kernel
> always delivers BR_DEAD_BINDER to a looper. So there's no scenario where
> Android userspace will call BC_DEAD_BINDER_DONE on a non-looper thread.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: syzbot+c8287e65a57a89e7fb72@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: syzbot+c8287e65a57a89e7fb72@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>

Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>

Best regards,
Andreas Hindborg