Re: [PATCH 3/3] io_uring: refactor io_sq_offload_create()

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

On 7/22/21 10:59 PM, Al Viro wrote:
> On Tue, Apr 20, 2021 at 12:03:33PM +0100, Pavel Begunkov wrote:
>> Just a bit of code tossing in io_sq_offload_create(), so it looks a bit
>> better. No functional changes.
> 
> Does a use-after-free count as a functional change?
> 
>>  		f = fdget(p->wq_fd);
> 
> Descriptor table is shared with another thread, grabbed a reference to file.
> Refcount is 2 (1 from descriptor table, 1 held by us)
> 
>>  		if (!f.file)
>>  			return -ENXIO;
> 
> Nope, not NULL.
> 
>> -		if (f.file->f_op != &io_uring_fops) {
>> -			fdput(f);
>> -			return -EINVAL;
>> -		}
>>  		fdput(f);
> 
> Decrement refcount, get preempted away.  f.file->f_count is 1 now.
> 
> Another thread: close() on the same descriptor.  Final reference to
> struct file (from descriptor table) is gone, file closed, memory freed.
> 
> Regain CPU...
> 
>> +		if (f.file->f_op != &io_uring_fops)
>> +			return -EINVAL;
> 
> ... and dereference an already freed structure.
> 
> What scares me here is that you are playing with bloody fundamental objects,
> without understanding even the basics regarding their handling ;-/

Yes, it's a stupid bug and slipped through accidentally, not proud of
it. And it's obvious to anyone that it shouldn't be touched after a
put, so have no clue why there is such a long explanation.
Anyway, thanks for letting know.

By luck, it should be of low severity, as it's a compatibility check,
the result of which is not depended upon by any code after. To
fault would need some RAM hot-remove (?). Not an excuse, how you
put it, but useful to notice.


> 1) descriptor tables can be shared.
> 2) another thread can close file right under you.
> 3) once all references to opened file are gone, it gets shut down and
> struct file gets freed.
> 4) inside an fdget()/fdput() pair you are guaranteed that (3) won't happen.
> As soon as you've done fdput(), that promise is gone.
> 
> 	In the above only (1) might have been non-obvious, because if you
> accept _that_, you have to ask yourself what the fuck would prevent file
> disappearing once you've done fdput(), seeing that it might be the last
> thing your syscall is doing to the damn thing.  So either that would've
> leaked it, or _something_ in the operations you've done to it must've
> made it possible for close(2) to get the damn thing.  And dereferencing
> ->f_op is unlikely to be that, isn't it?  Which leaves fdput() the
> only candidate.  It's common sense stuff...
> 
> 	Again, descriptor table is a shared resource and threads sharing
> it can issue syscalls at the same time.  Sure, I've got fewer excuses
> than you do for lack of documentation, but that's really basic...
> 

-- 
Pavel Begunkov