diff mbox series

io_uring: don't audit the capability check in io_uring_create()

Message ID 20230718115607.65652-1-omosnace@redhat.com
State New
Headers
Series io_uring: don't audit the capability check in io_uring_create() |

Commit Message

Ondrej Mosnacek July 18, 2023, 11:56 a.m. UTC 
  The check being unconditional may lead to unwanted denials reported by
LSMs when a process has the capability granted by DAC, but denied by an
LSM. In the case of SELinux such denials are a problem, since they can't
be effectively filtered out via the policy and when not silenced, they
produce noise that may hide a true problem or an attack.

Since not having the capability merely means that the created io_uring
context will be accounted against the current user's RLIMIT_MEMLOCK
limit, we can disable auditing of denials for this check by using
ns_capable_noaudit() instead of capable().

Fixes: 2b188cc1bb85 ("Add io_uring IO interface")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2193317
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 io_uring/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff Moyer July 18, 2023, 1:30 p.m. UTC | #1 
Hi, Ondrej,

Ondrej Mosnacek <omosnace@redhat.com> writes:

> The check being unconditional may lead to unwanted denials reported by
> LSMs when a process has the capability granted by DAC, but denied by an
> LSM. In the case of SELinux such denials are a problem, since they can't
> be effectively filtered out via the policy and when not silenced, they
> produce noise that may hide a true problem or an attack.
>
> Since not having the capability merely means that the created io_uring
> context will be accounted against the current user's RLIMIT_MEMLOCK
> limit, we can disable auditing of denials for this check by using
> ns_capable_noaudit() instead of capable().

Could you add a comment, or add some documentation to
ns_capable_noaudit() about when it should be used?  It wasn't apparent
to me, at least, before this explanation.

> Fixes: 2b188cc1bb85 ("Add io_uring IO interface")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2193317
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  io_uring/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 7505de2428e03..a9923676d16d6 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3870,7 +3870,7 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
>  		ctx->syscall_iopoll = 1;
>  
>  	ctx->compat = in_compat_syscall();
> -	if (!capable(CAP_IPC_LOCK))
> +	if (!ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
>  		ctx->user = get_uid(current_user());
>  
>  	/*

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Jens Axboe July 18, 2023, 8:16 p.m. UTC | #2 
On Tue, 18 Jul 2023 13:56:07 +0200, Ondrej Mosnacek wrote:
> The check being unconditional may lead to unwanted denials reported by
> LSMs when a process has the capability granted by DAC, but denied by an
> LSM. In the case of SELinux such denials are a problem, since they can't
> be effectively filtered out via the policy and when not silenced, they
> produce noise that may hide a true problem or an attack.
> 
> Since not having the capability merely means that the created io_uring
> context will be accounted against the current user's RLIMIT_MEMLOCK
> limit, we can disable auditing of denials for this check by using
> ns_capable_noaudit() instead of capable().
> 
> [...]

Applied, thanks!

[1/1] io_uring: don't audit the capability check in io_uring_create()
      commit: 6adc2272aaaf84f34b652cf77f770c6fcc4b8336

Best regards,
Ondrej Mosnacek July 25, 2023, 11:07 a.m. UTC | #3 
On Tue, Jul 18, 2023 at 3:24 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Hi, Ondrej,
>
> Ondrej Mosnacek <omosnace@redhat.com> writes:
>
> > The check being unconditional may lead to unwanted denials reported by
> > LSMs when a process has the capability granted by DAC, but denied by an
> > LSM. In the case of SELinux such denials are a problem, since they can't
> > be effectively filtered out via the policy and when not silenced, they
> > produce noise that may hide a true problem or an attack.
> >
> > Since not having the capability merely means that the created io_uring
> > context will be accounted against the current user's RLIMIT_MEMLOCK
> > limit, we can disable auditing of denials for this check by using
> > ns_capable_noaudit() instead of capable().
>
> Could you add a comment, or add some documentation to
> ns_capable_noaudit() about when it should be used?  It wasn't apparent
> to me, at least, before this explanation.

This has been requested before, so I finally forced myself to look
into it and only now I realized that there is a subtle difference
between the has_capability and capable helpers. As the docstrings say,
the former doesn't set the PF_SUPERPRIV on the task when the check
succeeds, while the latter does. The problem is that I don't know what
the exact implications are and thus I'm not able to document which
helper should be used in what situation... It is possible some of the
existing call sites use the wrong helper in the noaudit case (possibly
including ones that I added/suggested).

The comment at its declaration says "Used super-user privileges" and
it seems to be used only to propagate into the ASU flag in task
accounting information. But in the case of capability checks that do
not fail the syscall it is not easy to tell if "super-user privileges"
were "used" or not (or, rather, whether the task should be accounted
as such or not after a successful check).

If anyone is reading this and has a better understanding of the
PF_SUPERPRIV flag semantics, I'd be thankful for a clarification so
that we can sort out this mess :)

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
diff mbox series

Patch

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7505de2428e03..a9923676d16d6 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3870,7 +3870,7 @@  static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
 		ctx->syscall_iopoll = 1;
 
 	ctx->compat = in_compat_syscall();
-	if (!capable(CAP_IPC_LOCK))
+	if (!ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
 		ctx->user = get_uid(current_user());
 
 	/*