2018-05-08 09:08:48

by Jia-Ju Bai

Subject: [PATCH] android: binder: Fix a possible data race in binder_alloc_mmap_handler

The write operations to "alloc->buffer" are protected by
the lock on line 679 and 730, but the read operation to
this data on line 712 is not protected by the lock.
Thus, there may exist a data race for "alloc->buffer".

To fix this data race, the read operation to "alloc->buffer"
should be also protected by the lock.

Signed-off-by: Jia-Ju Bai <[email protected]>
---
drivers/android/binder_alloc.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 5a426c877dfb..596acc3a84e4 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -709,7 +709,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
goto err_alloc_buf_struct_failed;
}

+ mutex_lock(&binder_alloc_mmap_lock);
buffer->data = alloc->buffer;
+ mutex_unlock(&binder_alloc_mmap_lock);
list_add(&buffer->entry, &alloc->buffers);
buffer->free = 1;
binder_insert_free_buffer(alloc, buffer);
--
2.17.0

2018-05-08 13:41:20

by Martijn Coenen

Subject: Re: [PATCH] android: binder: Fix a possible data race in binder_alloc_mmap_handler

On Tue, May 8, 2018 at 2:06 AM, Jia-Ju Bai <[email protected]> wrote:
> The write operations to "alloc->buffer" are protected by
> the lock on line 679 and 730, but the read operation to
> this data on line 712 is not protected by the lock.
> Thus, there may exist a data race for "alloc->buffer".

It's read by the same thread that just wrote it, there is no data race.

The locks at line 679 and 730 protect against multiple threads calling
mmap() at the same time.

>
> To fix this data race, the read operation to "alloc->buffer"
> should be also protected by the lock.
>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> drivers/android/binder_alloc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 5a426c877dfb..596acc3a84e4 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -709,7 +709,9 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
> goto err_alloc_buf_struct_failed;
> }
>
> + mutex_lock(&binder_alloc_mmap_lock);
> buffer->data = alloc->buffer;
> + mutex_unlock(&binder_alloc_mmap_lock);
> list_add(&buffer->entry, &alloc->buffers);
> buffer->free = 1;
> binder_insert_free_buffer(alloc, buffer);
> --
> 2.17.0
>