linux-kernel - Re: [PATCH] ashmem: Fix lockdep issue during llseek

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 26 Jan 2018 11:25:14 -0800
From:   Joel Fernandes <joelaf@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     LKML <linux-kernel@...r.kernel.org>, Todd Kjos <tkjos@...gle.com>,
        Arve Hjonnevag <arve@...roid.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] ashmem: Fix lockdep issue during llseek

On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <joelaf@...gle.com> wrote:
> Hi Al,
>
> On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>> On Thu, Jan 25, 2018 at 06:46:49PM -0800, Joel Fernandes wrote:
>>> ashmem_mutex create a chain of dependencies like so:
>>>
>>> (1)
>>> mmap syscall ->
>>>   mmap_sem ->  (acquired)
>>>   ashmem_mmap
>>>   ashmem_mutex (try to acquire)
>>>   (block)
>>>
>>> (2)
>>> llseek syscall ->
>>>   ashmem_llseek ->
>>>   ashmem_mutex ->  (acquired)
>>>   inode_lock ->
>>>   inode->i_rwsem (try to acquire)
>>>   (block)
>>>
>>> (3)
>>> getdents ->
>>>   iterate_dir ->
>>>   inode_lock ->
>>>   inode->i_rwsem   (acquired)
>>>   copy_to_user ->
>>>   mmap_sem         (try to acquire)
>>>
>>> There is a lock ordering created between mmap_sem and inode->i_rwsem
>>> during a syzcaller test, this patch fixes the issue by releasing the
>>> ashmem_mutex before the call to vfs_llseek, and reacquiring it after.
>>
>> That looks odd.  If this approach works, what the hell do you need
>> ashmem_mutex for in ashmem_llseek() in the first place?  What is
>> it protecting there?
>
> I was just trying to be careful with the least intrusive solution
> since I'm not the original author of the driver.
>
> But one usecase for the mutex is with concurrent lseeks, you can end
> up with a file->f_pos that is different from the latest update to
> asma->file->f_pos. A barrier could fix this it too though. Any
> thoughts?
>
> =================================================================
> CPU 1                           CPU 2
>
> // vfs_llseek updated
> // asma->file->f_pos to X
>                                  // vfs_llseek updated
>                                  // file->f_pos to Y
>
>                                  asma->file->f_pos updated to Y
> asma->file->f_pos updated to X
> (lost write)
> =================================================================

I screwed up the explanation here, this is what I had in mind:

=================================================================
CPU 1                           CPU 2

// vfs_llseek updated
// asma->file->f_pos to X
                                 // vfs_llseek updated
                                 // asma->file->f_pos to Y

                                 file->f_pos updated to Y
file->f_pos updated to X
(lost write)
=================================================================


Sorry,

- Joel