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]

Message-ID: <CAJWu+oo+Y4eNTX+vMceYTB-FF5A190OHP7yPKkDGYH-jTqSsmQ@mail.gmail.com>
Date:   Fri, 26 Jan 2018 11:23:47 -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

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)
=================================================================

thanks,

- Joel