Re: [PATCH 5/5] io_uring: fix use after free

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

On 04/07/2020 00:32, Jann Horn wrote:
> On Fri, Jul 3, 2020 at 9:50 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>> On 03/07/2020 05:39, Jann Horn wrote:
>>> On Mon, Jun 29, 2020 at 10:44 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>>> After __io_free_req() put a ctx ref, it should assumed that the ctx may
>>>> already be gone. However, it can be accessed to put back fallback req.
>>>> Free req first and then put a req.
>>>
>>> Please stick "Fixes" tags on bug fixes to make it easy to see when the
>>> fixed bug was introduced (especially for ones that fix severe issues
>>> like UAFs). From a cursory glance, it kinda seems like this one
>>> _might_ have been introduced in 2b85edfc0c90ef, which would mean that
>>> it landed in 5.6? But I can't really tell for sure without investing
>>> more time; you probably know that better.
>>
>> It was there from the beginning,
>> 0ddf92e848ab7 ("io_uring: provide fallback request for OOM situations")
>>
>>>
>>> And if this actually does affect existing releases, please also stick
>>> a "Cc: stable@vger.kernel.org" tag on it so that the fix can be
>>> shipped to users of those releases.
>>
>> As mentioned in the cover letter, it's pretty unlikely to ever happen.
>> No one seems to have seen it since its introduction in November 2019.
>> And as the patch can't be backported automatically, not sure it's worth
>> the effort. Am I misjudging here?
> 
> Use-after-free bugs are often security bugs; in particular when, as in
> this case, data is written through the freed pointer. That means that
> even if this is extremely unlikely to occur in practice under normal
> circumstances, you should assume that someone may invest a significant
> amount of time into engineering some way to make this bug happen. If

Good point, agree.

> you can show that the bug is _impossible_ to hit, that's fine, I
> guess. But if it's merely "it's a really tight race and unlikely to
> happen", I think we should be fixing it on the stable branches.
> For example, on kernels with PREEMPT=y (typically you get that config
> either with "lowlatency" kernels or on Android, I think), attackers
> can play games like giving their own tasks "idle" scheduling priority
> and intentionally preempting them right in the middle of a race
> window, which makes it possible to delay execution for intervals on
> the order of seconds if the attacker can manage to make the scheduler
> IPI hit in the right place.
> 
> I guess one way to hit this bug on mainline would be to go through the
> io_async_task_func() canceled==true case, right? That will drop
> references on a request at the very end, without holding any locks or
> so that might keep the context alive.

Yes, for an example, but even simpler to submit async reads/writes
(i.e. with kiocb->ki_complete set) that would trigger io_complete_rw()
from irq. Or do something with timeout or poll requests.

-- 
Pavel Begunkov