Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Pavel Begunkov <asml.silence@gmail.com>]

On 09/10/2020 15:12, Matthew Wilcox wrote:
> On Fri, Oct 09, 2020 at 02:10:49PM +0300, Pavel Begunkov wrote:
>>>  kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
>>>  xas_next_entry include/linux/xarray.h:1630 [inline]
>>>  __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
>>>  io_uring_files_cancel include/linux/io_uring.h:35 [inline]
>>>  exit_files+0xe4/0x170 fs/file.c:456
>>>  do_exit+0xae9/0x2930 kernel/exit.c:801
>>>  do_group_exit+0x125/0x310 kernel/exit.c:903
>>>  get_signal+0x428/0x1f00 kernel/signal.c:2757
>>>  arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
>>>  exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
>>>  exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
>>>  syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
>>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> It seems this fails on "node->shift" in xas_next_entry(), that would
>> mean that the node itself was freed while we're iterating on it.
>>
>> __io_uring_files_cancel() iterates with xas_next_entry() and creates
>> XA_STATE once by hand, but it also removes entries in the loop with
>> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
>> the iterating XA_STATE. Could it be the problem? See a diff below
> 
> No, the problem is that the lock is dropped after calling
> xas_next_entry(), and at any point after calling xas_next_entry(),
> the node that it's pointing to can be freed.

Only the task itself clears/removes entries, others can only insert.
So, could it be freed even though there are no parallel erases?

> 
> I don't think there's any benefit to using the advanced API here.
> Since io_uring_cancel_task_requests() can sleep, we have to drop the lock
> for each iteration around the loop, and so we have to walk from the top of the tree each time.  So we may as well make this easy to read:

Thanks for looking into it, looks definitely better.

> 
> @@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
>  void __io_uring_files_cancel(struct files_struct *files)
>  {
>         struct io_uring_task *tctx = current->io_uring;
> -       XA_STATE(xas, &tctx->xa, 0);
> +       struct file *file;
> +       unsigned long index;
>  
>         /* make sure overflow events are dropped */
>         tctx->in_idle = true;
>  
> -       do {
> -               struct io_ring_ctx *ctx;
> -               struct file *file;
> -
> -               xas_lock(&xas);
> -               file = xas_next_entry(&xas, ULONG_MAX);
> -               xas_unlock(&xas);
> -
> -               if (!file)
> -                       break;
> -
> -               ctx = file->private_data;
> +       xa_for_each(&tctx->xa, index, file) {
> +               struct io_ring_ctx *ctx = file->private_data;
>  
>                 io_uring_cancel_task_requests(ctx, files);
>                 if (files)
>                         io_uring_del_task_file(file);
> -       } while (1);
> +       }
>  }
>  
>  static inline bool io_uring_task_idle(struct io_uring_task *tctx)
> 
> I'll send a proper patch in a few minutes -- I'd like to neaten up a
> few of the other XArray uses.
> 

-- 
Pavel Begunkov