linux-kernel - Re: [PATCH] android: binder: Fix a possible data race in binder_alloc_mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 8 May 2018 06:38:58 -0700
From:   Martijn Coenen <maco@...roid.com>
To:     Jia-Ju Bai <baijiaju1990@...il.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>
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 <baijiaju1990@...il.com> 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 <baijiaju1990@...il.com>
> ---
>  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
>