public inbox for [email protected]
 help / color / mirror / Atom feed
From: Steve Grubb <[email protected]>
To: Richard Guy Briggs <[email protected]>
Cc: Linux-Audit Mailing List <[email protected]>,
	[email protected]
Subject: Re: [PATCH v3 2/7] add support for the uring filter list
Date: Mon, 01 Nov 2021 11:58:41 -0400	[thread overview]
Message-ID: <2282661.ElGaqSPkdT@x2> (raw)
In-Reply-To: <[email protected]>

Hello Richard,

On Monday, November 1, 2021 11:05:49 AM EDT Richard Guy Briggs wrote:
> On 2021-10-29 14:39, Steve Grubb wrote:
> > On Thursday, October 28, 2021 3:59:34 PM EDT Richard Guy Briggs wrote:
> > > Kernel support to audit io_uring operations filtering was added with
> > > commit 67daf270cebc ("audit: add filtering for io_uring records").  Add
> > > support for the "uring" filter list to auditctl.
> > 
> > Might have been good to show what the resulting auditctl command looks
> > like. I think it would be:
> > 
> > auditctl -a always,io_uring  -U  open -F uid!=0 -F key=io_uring
> > 
> > But I wonder, why the choice of  -U rather than -S? That would make
> > remembering the syntax easier.
> > 
> > auditctl -a always,io_uring  -S  open -F uid!=0 -F key=io_uring
> 
> Well, I keep seeing the same what I assume is a typo in your
> communications about io_uring where the "u" is missing, which might help
> trigger your memory about the syntax.

Yeah, but I'm thinking that we can abstract that technicality away and keep 
the syntax the same.

> The io_uring operations name list is different than the syscall list, so
> it needs to use a different lookup table.

Right. So, if you choose an operation that is not supported, you get an 
error. But to help people know what is supported, we can add the lookup to 
ausyscall where  --io_uring could direct it to the right lookup table.

> Have I misunderstood something?

No, but I'm thinking of aesthetics and usability. You already have to specify 
a filter. We don't really need to have completely different syntax in 
addition. Especially since the operations map to the equivalent of a syscall.
 

> > > Signed-off-by: Richard Guy Briggs <[email protected]>
> > > ---
> > > docs/audit.rules.7         |  19 ++++--
> > > docs/audit_add_rule_data.3 |   4 ++
> > > docs/auditctl.8            |  10 ++-
> > > lib/flagtab.h              |  11 ++--
> > > lib/libaudit.c             |  50 ++++++++++++---
> > > lib/libaudit.h             |   7 +++
> > > lib/lookup_table.c         |  20 ++++++
> > > lib/private.h              |   1 +
> > > src/auditctl-listing.c     |  52 ++++++++++------
> > > src/auditctl.c             | 121 ++++++++++++++++++++++++++++++++-----
> > > 10 files changed, 240 insertions(+), 55 deletions(-)
> > 
> > <snip a whole lot of documentation>
> > 
> > > diff --git a/lib/libaudit.c b/lib/libaudit.c
> > > index 54e276156ef0..3790444f4497 100644
> > > --- a/lib/libaudit.c
> > > +++ b/lib/libaudit.c
> > > @@ -86,6 +86,7 @@ static const struct nv_list failure_actions[] =
> > > int _audit_permadded = 0;
> > > int _audit_archadded = 0;
> > > int _audit_syscalladded = 0;
> > > +int _audit_uringopadded = 0;
> > > int _audit_exeadded = 0;
> > > int _audit_filterfsadded = 0;
> > > unsigned int _audit_elf = 0U;
> > > @@ -999,6 +1000,26 @@ int audit_rule_syscallbyname_data(struct
> > > audit_rule_data *rule, return -1;
> > > }
> > > 
> > > +int audit_rule_uringopbyname_data(struct audit_rule_data *rule,
> > > +                                  const char *uringop)
> > > +{
> > > +       int nr, i;
> > > +
> > > +       if (!strcmp(uringop, "all")) {
> > > +               for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
> > > +                       rule->mask[i] = ~0;
> > > +               return 0;
> > > +       }
> > > +       nr = audit_name_to_uringop(uringop);
> > > +       if (nr < 0) {
> > > +               if (isdigit(uringop[0]))
> > > +                       nr = strtol(uringop, NULL, 0);
> > > +       }
> > > +       if (nr >= 0)
> > > +               return audit_rule_syscall_data(rule, nr);
> > > +       return -1;
> > > +}
> > > +
> > > int audit_rule_interfield_comp_data(struct audit_rule_data **rulep,
> > > const char *pair,
> > > int flags)
> > > @@ -1044,7 +1065,7 @@ int audit_rule_interfield_comp_data(struct
> > > audit_rule_data **rulep, return -EAU_COMPVALUNKNOWN;
> > > 
> > > /* Interfield comparison can only be in exit filter */
> > > -       if (flags != AUDIT_FILTER_EXIT)
> > > +       if (flags != AUDIT_FILTER_EXIT && flags !=
> > > AUDIT_FILTER_URING_EXIT) return -EAU_EXITONLY;
> > > 
> > > // It should always be AUDIT_FIELD_COMPARE
> > > @@ -1557,7 +1578,8 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data
> > > **rulep, const char *pair, }
> > > break;
> > > case AUDIT_EXIT:
> > > -                       if (flags != AUDIT_FILTER_EXIT)
> > > +                       if (flags != AUDIT_FILTER_EXIT &&
> > > +                           flags != AUDIT_FILTER_URING_EXIT)
> > > return -EAU_EXITONLY;
> > > vlen = strlen(v);
> > > if (isdigit((char)*(v)))
> > > @@ -1599,7 +1621,8 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data
> > > **rulep, const char *pair, case AUDIT_DIR:
> > > /* Watch & object filtering is invalid on anything
> > > * but exit */
> > > -                       if (flags != AUDIT_FILTER_EXIT)
> > > +                       if (flags != AUDIT_FILTER_EXIT &&
> > > +                           flags != AUDIT_FILTER_URING_EXIT)
> > > return -EAU_EXITONLY;
> > > if (field == AUDIT_WATCH || field == AUDIT_DIR)
> > > _audit_permadded = 1;
> > > @@ -1621,9 +1644,11 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data **rulep, const char *pair, _audit_exeadded = 1;
> > > }
> > > if (field == AUDIT_FILTERKEY &&
> > > -                               !(_audit_syscalladded ||
> > > _audit_permadded || -                               _audit_exeadded ||
> > > -                               _audit_filterfsadded))
> > > +                               !(_audit_syscalladded ||
> > > +                                 _audit_uringopadded ||
> > > +                                 _audit_permadded ||
> > > +                                 _audit_exeadded ||
> > > +                                 _audit_filterfsadded))
> > > return -EAU_KEYDEP;
> > > vlen = strlen(v);
> > > if (field == AUDIT_FILTERKEY &&
> > > @@ -1712,7 +1737,8 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data
> > > **rulep, const char *pair, }
> > > break;
> > > case AUDIT_FILETYPE:
> > > -                       if (!(flags == AUDIT_FILTER_EXIT))
> > > +                       if (!(flags == AUDIT_FILTER_EXIT ||
> > > +                             flags == AUDIT_FILTER_URING_EXIT))
> > > return -EAU_EXITONLY;
> > > rule->values[rule->field_count] =
> > > audit_name_to_ftype(v);
> > > @@ -1754,7 +1780,8 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data
> > > **rulep, const char *pair, return -EAU_FIELDNOSUPPORT;
> > > if (flags != AUDIT_FILTER_EXCLUDE &&
> > > flags != AUDIT_FILTER_USER &&
> > > -                           flags != AUDIT_FILTER_EXIT)
> > > +                           flags != AUDIT_FILTER_EXIT &&
> > > +                           flags != AUDIT_FILTER_URING_EXIT)
> > 
> > This is in the session_id code. Looking at the example audit event:
> > 
> > https://listman.redhat.com/archives/linux-audit/2021-September/msg00058.h
> > tml
> > 
> > session_id is not in the record.
> 
> Fair enough.  It can be re-added if we are able to reliably report it.

Thanks.

> > > return -EAU_FIELDNOFILTER;
> > > // Do positive & negative separate for 32 bit systems
> > > vlen = strlen(v);
> > > @@ -1775,7 +1802,8 @@ int audit_rule_fieldpair_data(struct
> > > audit_rule_data
> > > **rulep, const char *pair, break;
> > 
> > > case AUDIT_DEVMAJOR...AUDIT_INODE:
> > ^^^ Can you audit by devmajor, devminor, or inode in io_ring?
> 
> Should be able to monitor files.  The old "-w" syntax is not supported
> but path= and dir= should be.

But that's not what this is. These is for:
 -F dev_major=
 -F dev_minor=
 -F inode=

It dates back to before watches were possible. In any event, this is being 
allowed when I suspect it shouldn't.

-Steve




  reply	other threads:[~2021-11-01 15:59 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 19:59 [PATCH v3 0/7] Add uringop support Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 1/7] add basic support for the AUDIT_URINGOP record type Richard Guy Briggs
2021-10-28 21:19   ` Steve Grubb
2021-10-28 19:59 ` [PATCH v3 2/7] add support for the uring filter list Richard Guy Briggs
2021-10-29 18:39   ` Steve Grubb
2021-11-01 15:05     ` Richard Guy Briggs
2021-11-01 15:58       ` Steve Grubb [this message]
2021-11-02 16:32         ` Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 3/7] add support for uringop names Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 4/7] add field support for the AUDIT_URINGOP record type Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 5/7] add ausearch --uringop option Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 6/7] add aureport " Richard Guy Briggs
2021-10-28 19:59 ` [PATCH v3 7/7] add iouring support to the normalizer Richard Guy Briggs

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2282661.ElGaqSPkdT@x2 \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox