Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180508090639.14275-1-baijiaju1990@gmail.com>
References: <20180508090639.14275-1-baijiaju1990@gmail.com>
From:   Martijn Coenen <maco@android.com>
Date:   Tue, 8 May 2018 06:38:58 -0700
Message-ID: <CAB0TPYHVNhNoHTSuDR2Ew8iOeMo9SaKfoY2bF6WF7C5ADCyU+Q@mail.gmail.com>
Subject: Re: [PATCH] android: binder: Fix a possible data race in binder_alloc_mmap_handler
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 8, 2018 at 2:06 AM, Jia-Ju Bai <baijiaju1990@gmail.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@gmail.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
>