Re: [PATCH v3 2/7] add support for the uring filter list
On 2021-11-01 11:58, Steve Grubb wrote:
> 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.
They do use the same mechanism to get the data into the kernel, but this
is not user-visible.
> > 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.
In terms of usibility, it is a different lookup table and the operations
don't map exactly to each other. I'd *expect* to use a different
command line option to specify io_uring ops than I did for syscalls
since they are not the same since if I changed a rule text from one list
to another, I'd also need to change the list of ops and which list they
came from. I'd want it to throw an error if I used the wrong argument
type.
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > 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(-)
> > >
> > >
> > >
> > > > 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
Re: [PATCH v3 2/7] add support for the uring filter list
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
> > > ---
> > > 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(-)
> >
> >
> >
> > > 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
Re: [PATCH v3 2/7] add support for the uring filter list
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_ring -U open -F uid!=0 -F key=io_ring
>
> But I wonder, why the choice of -U rather than -S? That would make
> remembering the syntax easier.
>
> auditctl -a always,io_ring -S open -F uid!=0 -F key=io_ring
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.
The io_uring operations name list is different than the syscall list, so
it needs to use a different lookup table.
Have I misunderstood something?
> > Signed-off-by: Richard Guy Briggs
> > ---
> > 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(-)
>
>
>
>
>
> > 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 ||
> > +
Re: [PATCH v3 2/7] add support for the uring filter list
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_ring -U open -F uid!=0 -F key=io_ring
But I wonder, why the choice of -U rather than -S? That would make
remembering the syntax easier.
auditctl -a always,io_ring -S open -F uid!=0 -F key=io_ring
> Signed-off-by: Richard Guy Briggs
> ---
> 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(-)
> 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
