Re: [PATCH 5.4 033/222] io_uring: only allow submit from owning task

[Stefan Metzmacher]

Am 22.01.20 um 10:26 schrieb Greg Kroah-Hartman:
> From: Jens Axboe <axboe@xxxxxxxxx>
> 
> commit 44d282796f81eb1debc1d7cb53245b4cb3214cb5 upstream.
> 
> If the credentials or the mm doesn't match, don't allow the task to
> submit anything on behalf of this ring. The task that owns the ring can
> pass the file descriptor to another task, but we don't want to allow
> that task to submit an SQE that then assumes the ring mm and creds if
> it needs to go async.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Stefan Metzmacher <metze@xxxxxxxxx>
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> 
> 
> ---
>  fs/io_uring.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -3716,6 +3716,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned
>  			wake_up(&ctx->sqo_wait);
>  		submitted = to_submit;
>  	} else if (to_submit) {
> +		if (current->mm != ctx->sqo_mm ||
> +		    current_cred() != ctx->creds) {
> +			ret = -EPERM;
> +			goto out;
> +		}
> +

I thought about this a bit more.

I'm not sure if this is actually to restrictive,
because it means applications like Samba won't
be able to use io-uring at all.

As even if current_cred() and ctx->creds describe the same
set of uid,gids the != won't ever match again and
makes the whole ring unuseable.

I'm not sure about what the best short term solution could be...

1. May just doing the check for path based operations?
  and fail individual requests with EPERM.

2. Or force REQ_F_FORCE_ASYNC for path based operations,
  so that they're always executed from within the workqueue
  with were ctx->creds is active.

3. Or (as proposed earlier) do the override_creds/revert_creds dance
  (and similar for mm) if needed.

To summaries the problem again:

For path based operations like:
- IORING_OP_CONNECT (maybe also - IORING_OP_ACCEPT???)
- IORING_OP_SEND*, IORING_OP_RECV* on DGRAM sockets
- IORING_OP_OPENAT, IORING_OP_STATX, IORING_OP_OPENAT2
it's important under which current_cred they are called.

Are IORING_OP_MADVISE, IORING_OP_FADVISE and IORING_OP_FALLOCATE
are only bound to the credentials of the passed fd they operate on?

The current assumption is that the io_uring_setup() syscall captures
the current_cred() to ctx->cred and all operations on the ring
are executed under the context of ctx->cred.
Therefore all helper threads do the override_creds/revert_creds dance.

But the possible non-blocking line execution of operations in
the io_uring_enter() syscall doesn't do the override_creds/revert_creds
dance and execute the operations under current_cred().

This means it's random depending on filled cached under what
credentials an operation is executed.

In order to prevent security problems the current patch is enough,
but as outlined above it will make io-uring complete unuseable
for applications using any syscall that changes current_cred().

Change 1. would be a little bit better, but still not really useful.

I'd actually prefer solution 3. as it's still possible to make
use of non-blocking operations, while the security is the
same as solution 2.

metze

Attachment: signature.asc
Description: OpenPGP digital signature