Re: [RFC PATCH 2/9] audit,io_uring,io-wq: add some basic audit support to io_uring

From: Pavel Begunkov <asml.silence@gmail.com>
To: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	linux-audit@redhat.com, io-uring@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	Jens Axboe <axboe@kernel.dk>,
	Alexander Viro <viro@zeniv.linux.org.uk>
Subject: Re: [RFC PATCH 2/9] audit,io_uring,io-wq: add some basic audit support to io_uring
Date: Sun, 23 May 2021 21:26:22 +0100	[thread overview]
Message-ID: <162219f9-7844-0c78-388f-9b5c06557d06@gmail.com> (raw)
In-Reply-To: <CAHC9VhRjzWxweB8d8fypUx11CX6tRBnxSWbXH+5qM1virE509A@mail.gmail.com>

On 5/22/21 3:36 AM, Paul Moore wrote:
> On Fri, May 21, 2021 at 8:22 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>> On 5/21/21 10:49 PM, Paul Moore wrote:
[...]
>>>
>>> +     if (req->opcode < IORING_OP_LAST)
>>
>> always true at this point
> 
> I placed the opcode check before the audit call because the switch
> statement below which handles the operation dispatching has a 'ret =
> -EINVAL' for the default case, implying that there are some paths
> where an invalid opcode could be passed into the function.  Obviously
> if that is not the case and you can guarantee that req->opcode will
> always be valid we can easily drop the check prior to the audit call.

It is always true at this point, would be completely broken
otherwise

>>> +             audit_uring_entry(req->opcode);
>>
>> So, it adds two if's with memory loads (i.e. current->audit_context)
>> per request in one of the hottest functions here... No way, nack
>>
>> Maybe, if it's dynamically compiled into like kprobes if it's
>> _really_ used.
> 
> I'm open to suggestions on how to tweak the io_uring/audit
> integration, if you don't like what I've proposed in this patchset,
> lets try to come up with a solution that is more palatable.  If you
> were going to add audit support for these io_uring operations, how
> would you propose we do it?  Not being able to properly audit io_uring
> operations is going to be a significant issue for a chunk of users, if
> it isn't already, we need to work to find a solution to this problem.

Who knows. First of all, seems CONFIG_AUDIT is enabled by default
for many popular distributions, so I assume that is not compiled out.

What are use cases for audit? Always running I guess? Putting aside
compatibility problems, it sounds that with the amount of overhead
it adds there is no much profit in using io_uring in the first place.
Is that so?

__audit_uring_exit()
-> audit_filter_syscall()
  -> for (audit_list) if (...) audit_filter_rules()
    -> ...
-> audit_filter_inodes()
  -> ...

> Unfortunately I don't think dynamically inserting audit calls is
> something that would meet the needs of the audit community (I fear it
> would run afoul of the various security certifications), and it
> definitely isn't something that we support at present.

I see

-- 
Pavel Begunkov