linux-kernel - Re: [PATCH] fs/io_uring: fix O_PATH fds in openat, openat2, statx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <672e9220-2634-95f1-95a1-62c35bcf7341@kernel.dk>
Date:   Thu, 7 May 2020 20:53:24 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Max Kellermann <mk@...all.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] fs/io_uring: fix O_PATH fds in openat, openat2, statx

On 5/7/20 8:28 PM, Jens Axboe wrote:
> On 5/7/20 5:31 PM, Al Viro wrote:
>> On Thu, May 07, 2020 at 05:03:17PM -0600, Jens Axboe wrote:
>>> On 5/7/20 4:44 PM, Al Viro wrote:
>>>> On Thu, May 07, 2020 at 04:25:24PM -0600, Jens Axboe wrote:
>>>>
>>>>>  static int io_close(struct io_kiocb *req, bool force_nonblock)
>>>>>  {
>>>>> +	struct files_struct *files = current->files;
>>>>>  	int ret;
>>>>>  
>>>>>  	req->close.put_file = NULL;
>>>>> -	ret = __close_fd_get_file(req->close.fd, &req->close.put_file);
>>>>> +	spin_lock(&files->file_lock);
>>>>> +	if (req->file->f_op == &io_uring_fops ||
>>>>> +	    req->close.fd == req->ctx->ring_fd) {
>>>>> +		spin_unlock(&files->file_lock);
>>>>> +		return -EBADF;
>>>>> +	}
>>>>> +
>>>>> +	ret = __close_fd_get_file_locked(files, req->close.fd,
>>>>> +						&req->close.put_file);
>>>>
>>>> Pointless.  By that point req->file might have nothing in common with
>>>> anything in any descriptor table.
>>>
>>> How about the below then? Stop using req->file, defer the lookup until
>>> we're in the handler instead. Not sure the 'fd' check makes sense
>>> at this point, but at least we should be consistent in terms of
>>> once we lookup the file and check the f_op.
>>
>> Actually, what _is_ the reason for that check?  Note, BTW, that if the
>> file in question happens to be an AF_UNIX socket, closing it will
>> close all references held in SCM_RIGHTS datagrams sitting in its queue,
>> which might very well include io_uring files.
>>
>> IOW, if tries to avoid something really unpleasant, it's not enough.
>> And if it doesn't, then what is it for?
> 
> Maybe there is no issue at all, the point was obviously to not have
> io_uring close itself. But we might just need an ordering of the
> fput vs put_request to make that just fine. Let me experiment a bit
> and see what's going on.

Ran various bits of testing and tracing with just the below, and
I don't see anything wrong. Even verified the same cases with
pending poll requests and an async read (punted to thread), and
it works and doesn't complain with KASAN either.

And I did think this would work after looking at it. The ctx
referencing should handle this just fine. Hence it seems to me
that my initial attempts at blocking the ring closing itself
were not needed at all.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 979d9f977409..9099a9362ad4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -786,7 +786,6 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_fs		= 1,
 	},
 	[IORING_OP_CLOSE] = {
-		.needs_file		= 1,
 		.file_table		= 1,
 	},
 	[IORING_OP_FILES_UPDATE] = {
@@ -3399,10 +3398,6 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
-	if (req->file->f_op == &io_uring_fops ||
-	    req->close.fd == req->ctx->ring_fd)
-		return -EBADF;
-
 	return 0;
 }
 
-- 
Jens Axboe