Re: [RFC PATCH 4/9] audit: add filtering for io_uring records

On Mon, May 31, 2021 at 9:44 AM Richard Guy Briggs [off-list ref] wrote:

On 2021-05-30 11:26, Paul Moore wrote:

quoted

On Fri, May 28, 2021 at 6:36 PM Richard Guy Briggs [off-list ref] wrote:

quoted

On 2021-05-21 17:50, Paul Moore wrote:

...

quoted

quoted

quoted

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d8aa2c690bf9..4f6ab34020fb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c

@@ -799,6 +799,35 @@ static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
      return rule->mask[word] & bit;
 }

+/**
+ * audit_filter_uring - apply filters to an io_uring operation
+ * @tsk: associated task
+ * @ctx: audit context
+ */
+static void audit_filter_uring(struct task_struct *tsk,
+                            struct audit_context *ctx)
+{
+     struct audit_entry *e;
+     enum audit_state state;
+
+     if (auditd_test_task(tsk))
+             return;

Is this necessary?  auditd and auditctl don't (intentionally) use any
io_uring functionality.  Is it possible it might inadvertantly use some
by virtue of libc or other library calls now or in the future?

I think the better question is what harm does it do?  Yes, I'm not
aware of an auditd implementation that currently makes use of
io_uring, but it is also not inconceivable some future implementation
might want to make use of it and given the disjoint nature of kernel
and userspace development I don't want the kernel to block such
developments.  However, if you can think of a reason why having this
check here is bad I'm listening (note: we are already in the slow path
here so having the additional check isn't an issue as far as I'm
concerned).

As a reminder, auditd_test_task() only returns true/1 if the task is
registered with the audit subsystem as an auditd connection, an
auditctl process should not cause this function to return true.

My main concern was overhead, since the whole goal of io_uring is speed.

At the point where this test takes place we are already in the audit
slow path as far as io_uring is concerned.  I understand your concern,
but the advantage of being able to effectively use io_uring in the
future makes this worth keeping in my opinion.

The chances that audit does use this functionality in the future suggest
to me that it is best to leave this check in.

Sounds like we are in agreement.  We'll keep it for now.

quoted

quoted

quoted

+     rcu_read_lock();
+     list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
+                             list) {
+             if (audit_in_mask(&e->rule, ctx->uring_op) &&

While this seems like the most obvious approach given the parallels
between syscalls and io_uring operations, as coded here it won't work
due to the different mappings of syscall numbers and io_uring
operations unless we re-use the auditctl -S field with raw io_uring
operation numbers in the place of syscall numbers.  This should have
been obvious to me when I first looked at this patch.  It became obvious
when I started looking at the userspace auditctl.c.

FWIW, my intention was to treat io_uring opcodes exactly like we treat
syscall numbers.  Yes, this would potentially be an issue if we wanted
to combine syscalls and io_uring opcodes into one filter, but why
would we ever want to do that?  Combining the two into one filter not
only makes the filter lists longer than needed (we will always know if
we are filtering on a syscall or io_uring op) and complicates the
filter rule processing.

Or is there a problem with this that I'm missing?

No, I think you have a good understanding of it.  I'm asking hard
questions to avoid missing something important.  If we can reuse the
syscall infrastructure for this then that is extremely helpful (if not
lazy, which isn't necessarily a bad thing).  It does mean that the
io_uring op dictionary will need to live in userspace audit the way it
is currently implemented ....

Which I currently believe is the right thing to do.

quoted

quoted

The easy first step would be to use something like this:
        auditctl -a uring,always -S 18,28 -F key=uring_open
to monitor file open commands only.  The same is not yet possible for
the perm field, but there are so few io_uring ops at this point compared
with syscalls that it might be manageable.  The arch is irrelevant since
io_uring operation numbers are identical across all hardware as far as I
can tell.  Most of the rest of the fields should make sense if they do
for a syscall rule.

I've never been a fan of audit's "perm" filtering; I've always felt
there were better ways to handle that so I'm not overly upset that we
are skipping that functionality with this initial support.  If it
becomes a problem in the future we can always add that support at a
later date.

Ok, I don't see a pressing need to add it initially, but should add a
check to block that field from being used to avoid the confusion of
unpredictable behaviour should someone try to add a perm filter to a
io_uring filter.  That should be done protectively in the kernel and
proactively in userspace.

Sure, that's reasonable.

quoted

quoted

Here's a sample of userspace code to support this
patch:
        https://github.com/rgbriggs/audit-userspace/commit/a77baa1651b7ad841a220eb962d4cc92bc07dc96
        https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghau-iouring-filtering.v1.0

Great, thank you.  I haven't grabbed a copy yet for testing, but I will.

I've added a perm filter block as an additional patch in userspace and
updated the tree so that first commit is no longer the top of tree but
the branch name is current.

I'll add a kernel perm filter check.

I just noticed some list checking that is missing in tree and watch in
your patch.

Suggested fixup patches to follow...

I see them, thank you, comments will follow over there.  Although to
be honest I'm mostly focusing on the testing right now while we wait
to hear back from Jens on what he is willing to accept regarding audit
calls in io_issue_sqe().  If we can't do the _entry()/_exit() calls
then this work is pretty much dead and we just have to deal with it in
Kconfig.  I might make one last, clean patchset and put it in a branch
for the distros that want to carry the patchset, but it isn't clear to
me that it is something I would want to maintain long term.  Long
running out of tree patches are generally A Bad Idea.

quoted

quoted

If we abuse the syscall infrastructure at first, we'd need a transition
plan to coordinate user and kernel switchover to seperate mechanisms for
the two to work together if the need should arise to have both syscall
and uring filters in the same rule.

See my comments above, I don't currently see why we would ever want
syscall and io_uring filtering to happen in the same rule.  Please
speak up if you can think of a reason why this would either be needed,
or desirable for some reason.

I think they can be seperate rules for now.  Either a syscall rule
catching all io_uring ops can be added, or an io_uring rule can be added
to catch specific ops.  The scenario I was thinking of was catching
syscalls of specific io_uring ops.

Perhaps I'm misunderstand you, but that scenario really shouldn't
exist.  The io_uring ops function independently of syscalls; you can
*submit* io_uring ops via io_uring_enter(), but they are not
guaranteed to be dispatched synchronously (obviously), and given the
cred shenanigans that can happen with io_uring there is no guarantee
the filters would even be applicable.

It isn't an issue of "can" the filters be separate, they *have* to be separate.

-- 
paul moore
www.paul-moore.com