Re: [PATCH] aio: take an extra file reference before call vfs_poll()

Re: [PATCH] aio: take an extra file reference before call vfs_poll()

[Hou Tao]

ping ?

On 2019/2/25 17:03, Hou Tao wrote:
> Taking an extra file reference before call vfs_poll(), else
> the file may be released by aio_poll_wake() if an expected
> event is triggered immediately (e.g., by the close of a
> pair of pipes) after the return of vfs_poll(), and we may
> hit a use-after-free splat as shown below:
> 
>  BUG: KASAN: use-after-free in perf_trace_lock_acquire+0x3ab/0x570
>  Read of size 8 at addr ffff888379bfd4b0 by task syz-executor.1/4953
> 
>  CPU: 0 PID: 4953 Comm: syz-executor.1 Not tainted 4.19.24
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1
>  Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0xca/0x13e lib/dump_stack.c:113
>   print_address_description+0x79/0x330 mm/kasan/report.c:256
>   kasan_report_error mm/kasan/report.c:354 [inline]
>   kasan_report+0x18a/0x2e0 mm/kasan/report.c:412
>   trace_event_get_offsets_lock_acquire include/trace/events/lock.h:13 [inline]
>   perf_trace_lock_acquire+0x3ab/0x570 include/trace/events/lock.h:13
>   trace_lock_acquire include/trace/events/lock.h:13 [inline]
>   lock_acquire+0x202/0x310 kernel/locking/lockdep.c:3899
>   __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
>   _raw_spin_lock+0x2c/0x40 kernel/locking/spinlock.c:144
>   spin_lock include/linux/spinlock.h:329 [inline]
>   aio_poll fs/aio.c:1750 [inline]
>   io_submit_one+0xb90/0x1b30 fs/aio.c:1853
>   __do_sys_io_submit fs/aio.c:1919 [inline]
>   __se_sys_io_submit fs/aio.c:1890 [inline]
>   __x64_sys_io_submit+0x19b/0x500 fs/aio.c:1890
>   do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>   ......
>   Allocated by task 4953:
>   set_track mm/kasan/kasan.c:460 [inline]
>   kasan_kmalloc+0xa0/0xd0 mm/kasan/kasan.c:553
>   kmem_cache_alloc_trace+0x12f/0x2d0 mm/slub.c:2733
>   kmalloc include/linux/slab.h:513 [inline]
>   kzalloc include/linux/slab.h:707 [inline]
>   alloc_pipe_info+0xdf/0x410 fs/pipe.c:633
>   get_pipe_inode fs/pipe.c:712 [inline]
>   create_pipe_files+0x98/0x780 fs/pipe.c:744
>   __do_pipe_flags+0x35/0x230 fs/pipe.c:781
>   do_pipe2+0x87/0x150 fs/pipe.c:829
>   __do_sys_pipe2 fs/pipe.c:847 [inline]
>   __se_sys_pipe2 fs/pipe.c:845 [inline]
>   __x64_sys_pipe2+0x55/0x80 fs/pipe.c:845
>   do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
>  Freed by task 4952:
>   set_track mm/kasan/kasan.c:460 [inline]
>   __kasan_slab_free+0x12e/0x180 mm/kasan/kasan.c:521
>   slab_free_hook mm/slub.c:1371 [inline]
>   slab_free_freelist_hook mm/slub.c:1398 [inline]
>   slab_free mm/slub.c:2953 [inline]
>   kfree+0xeb/0x2f0 mm/slub.c:3906
>   put_pipe_info+0xb0/0xd0 fs/pipe.c:556
>   pipe_release+0x1ab/0x240 fs/pipe.c:577
>   __fput+0x27f/0x7f0 fs/file_table.c:278
>   task_work_run+0x136/0x1b0 kernel/task_work.c:113
>   tracehook_notify_resume include/linux/tracehook.h:193 [inline]
>   exit_to_usermode_loop+0x1a7/0x1d0 arch/x86/entry/common.c:166
>   prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
>   syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
>   do_syscall_64+0x461/0x580 arch/x86/entry/common.c:293
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: bfe4037e722e ("aio: implement IOCB_CMD_POLL")
> Cc: stable@vger.kernel.org [4.19+]
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/aio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index aaaaf4d12c73..ea2f5de4feac 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1763,6 +1763,12 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
>  	/* one for removal from waitqueue, one for this function */
>  	refcount_set(&aiocb->ki_refcnt, 2);
>  
> +	/*
> +	 * file may be released by aio_poll_wake() if an expected event
> +	 * is triggered immediately after the return of vfs_poll(), so
> +	 * an extra reference is needed here to prevent use-after-free.
> +	 */
> +	get_file(req->file);
>  	mask = vfs_poll(req->file, &apt.pt) & req->events;
>  	if (unlikely(!req->head)) {
>  		/* we did not manage to set up a waitqueue, done */
> @@ -1788,6 +1794,8 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
>  	spin_unlock_irq(&ctx->ctx_lock);
>  
>  out:
> +	/* release the extra reference for vfs_poll() */
> +	fput(req->file);
>  	if (unlikely(apt.error)) {
>  		fput(req->file);
>  		return apt.error;
> 

Re: [PATCH] aio: take an extra file reference before call vfs_poll()

[Hou Tao]

ping ?

On 2019/3/1 18:09, Hou Tao wrote:
> ping ?
> 
> On 2019/2/25 17:03, Hou Tao wrote:
>> Taking an extra file reference before call vfs_poll(), else
>> the file may be released by aio_poll_wake() if an expected
>> event is triggered immediately (e.g., by the close of a
>> pair of pipes) after the return of vfs_poll(), and we may
>> hit a use-after-free splat as shown below:
>>
>>  BUG: KASAN: use-after-free in perf_trace_lock_acquire+0x3ab/0x570
>>  Read of size 8 at addr ffff888379bfd4b0 by task syz-executor.1/4953
>>
>>  CPU: 0 PID: 4953 Comm: syz-executor.1 Not tainted 4.19.24
>>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1
>>  Call Trace:
>>   __dump_stack lib/dump_stack.c:77 [inline]
>>   dump_stack+0xca/0x13e lib/dump_stack.c:113
>>   print_address_description+0x79/0x330 mm/kasan/report.c:256
>>   kasan_report_error mm/kasan/report.c:354 [inline]
>>   kasan_report+0x18a/0x2e0 mm/kasan/report.c:412
>>   trace_event_get_offsets_lock_acquire include/trace/events/lock.h:13 [inline]
>>   perf_trace_lock_acquire+0x3ab/0x570 include/trace/events/lock.h:13
>>   trace_lock_acquire include/trace/events/lock.h:13 [inline]
>>   lock_acquire+0x202/0x310 kernel/locking/lockdep.c:3899
>>   __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
>>   _raw_spin_lock+0x2c/0x40 kernel/locking/spinlock.c:144
>>   spin_lock include/linux/spinlock.h:329 [inline]
>>   aio_poll fs/aio.c:1750 [inline]
>>   io_submit_one+0xb90/0x1b30 fs/aio.c:1853
>>   __do_sys_io_submit fs/aio.c:1919 [inline]
>>   __se_sys_io_submit fs/aio.c:1890 [inline]
>>   __x64_sys_io_submit+0x19b/0x500 fs/aio.c:1890
>>   do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>   ......
>>   Allocated by task 4953:
>>   set_track mm/kasan/kasan.c:460 [inline]
>>   kasan_kmalloc+0xa0/0xd0 mm/kasan/kasan.c:553
>>   kmem_cache_alloc_trace+0x12f/0x2d0 mm/slub.c:2733
>>   kmalloc include/linux/slab.h:513 [inline]
>>   kzalloc include/linux/slab.h:707 [inline]
>>   alloc_pipe_info+0xdf/0x410 fs/pipe.c:633
>>   get_pipe_inode fs/pipe.c:712 [inline]
>>   create_pipe_files+0x98/0x780 fs/pipe.c:744
>>   __do_pipe_flags+0x35/0x230 fs/pipe.c:781
>>   do_pipe2+0x87/0x150 fs/pipe.c:829
>>   __do_sys_pipe2 fs/pipe.c:847 [inline]
>>   __se_sys_pipe2 fs/pipe.c:845 [inline]
>>   __x64_sys_pipe2+0x55/0x80 fs/pipe.c:845
>>   do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>>  Freed by task 4952:
>>   set_track mm/kasan/kasan.c:460 [inline]
>>   __kasan_slab_free+0x12e/0x180 mm/kasan/kasan.c:521
>>   slab_free_hook mm/slub.c:1371 [inline]
>>   slab_free_freelist_hook mm/slub.c:1398 [inline]
>>   slab_free mm/slub.c:2953 [inline]
>>   kfree+0xeb/0x2f0 mm/slub.c:3906
>>   put_pipe_info+0xb0/0xd0 fs/pipe.c:556
>>   pipe_release+0x1ab/0x240 fs/pipe.c:577
>>   __fput+0x27f/0x7f0 fs/file_table.c:278
>>   task_work_run+0x136/0x1b0 kernel/task_work.c:113
>>   tracehook_notify_resume include/linux/tracehook.h:193 [inline]
>>   exit_to_usermode_loop+0x1a7/0x1d0 arch/x86/entry/common.c:166
>>   prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
>>   syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
>>   do_syscall_64+0x461/0x580 arch/x86/entry/common.c:293
>>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Fixes: bfe4037e722e ("aio: implement IOCB_CMD_POLL")
>> Cc: stable@vger.kernel.org [4.19+]
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  fs/aio.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index aaaaf4d12c73..ea2f5de4feac 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1763,6 +1763,12 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
>>  	/* one for removal from waitqueue, one for this function */
>>  	refcount_set(&aiocb->ki_refcnt, 2);
>>  
>> +	/*
>> +	 * file may be released by aio_poll_wake() if an expected event
>> +	 * is triggered immediately after the return of vfs_poll(), so
>> +	 * an extra reference is needed here to prevent use-after-free.
>> +	 */
>> +	get_file(req->file);
>>  	mask = vfs_poll(req->file, &apt.pt) & req->events;
>>  	if (unlikely(!req->head)) {
>>  		/* we did not manage to set up a waitqueue, done */
>> @@ -1788,6 +1794,8 @@ static ssize_t aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb)
>>  	spin_unlock_irq(&ctx->ctx_lock);
>>  
>>  out:
>> +	/* release the extra reference for vfs_poll() */
>> +	fput(req->file);
>>  	if (unlikely(apt.error)) {
>>  		fput(req->file);
>>  		return apt.error;
>>
> 
> 
> .
> 

Re: [PATCH] aio: take an extra file reference before call vfs_poll()

[Al Viro]

On Tue, Mar 05, 2019 at 09:17:07AM +0800, Hou Tao wrote:
> ping ?

Oh, hell...  I really wish I'd seen your mail back then ;-/
Would've avoided a lot of headache now (and essentially
duplicating your analysis + patch when a later report
got noticed - right before the -final, Murphy Law being what
it is).

Unfortunately, that approach (and I'd ended up with pretty
much the same patch) doesn't solve all problems in the
area; I think I have a sane fix, will post tomorrow morning...

Again, my apologies.  I probably need to do what Linus had
done years ago and unsubscribe from l-k - the traffic is
too high and stuff gets lost in the noise ;-/