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.
Cc: Todd Kjos <[email protected]>
Cc: Arve Hjonnevag <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Reported-by: [email protected]
Signed-off-by: Joel Fernandes <[email protected]>
---
drivers/staging/android/ashmem.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 0f695df14c9d..248983cf2db1 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -343,7 +343,9 @@ static loff_t ashmem_llseek(struct file *file, loff_t offset, int origin)
goto out;
}
+ mutex_unlock(&ashmem_mutex);
ret = vfs_llseek(asma->file, offset, origin);
+ mutex_lock(&ashmem_mutex);
if (ret < 0)
goto out;
--
2.16.0.rc1.238.g530d649a79-goog
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?
Hi Al,
On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <[email protected]> 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
On Fri, Jan 26, 2018 at 11:23 AM, Joel Fernandes <[email protected]> wrote:
> Hi Al,
>
> On Thu, Jan 25, 2018 at 7:13 PM, Al Viro <[email protected]> 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
On Fri, Jan 26, 2018 at 11:23:47AM -0800, Joel Fernandes wrote:
> I was just trying to be careful with the least intrusive solution
> since I'm not the original author of the driver.
Sure, but that needs a proof that it *is* a solution...
> 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?
lseek(2) is serialized against lseek(2) and read(2) on the same struct
file - see fdget_pos() for details.
ashmem_mutex really does look like an overkill - something much lighter
should serve just fine...
Hi Al,
On Fri, Jan 26, 2018 at 11:39 AM, Al Viro <[email protected]> wrote:
[..]
>
>> 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?
>
> lseek(2) is serialized against lseek(2) and read(2) on the same struct
> file - see fdget_pos() for details.
Ah right, Ok. Thanks.
> ashmem_mutex really does look like an overkill - something much lighter
> should serve just fine...
There's also the issue of asma->size getting updated from while being
in read ashmem_llseek (although this is a theoretical concern):
if (asma->size == 0) {
ret = -EINVAL;
goto out;
}
Which could just use READ_ONCE and WRITE_ONCE instead of the mutex I suppose?
Other than that, I don't see any other issues at the moment with
dropping of the ashmem_mutex from ashmem_llseek.
thanks,
- Joel