[PATCH v1 0/2] two suggested iouring op audit updates

[PATCH v1 0/2] two suggested iouring op audit updates

[Richard Guy Briggs]

A couple of updates to the iouring ops audit bypass selections suggested in
consultation with Steve Grubb.

Richard Guy Briggs (2):
  io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
  io_uring,audit: do not log IORING_OP_*GETXATTR

 io_uring/opdef.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Richard Guy Briggs]

Since FADVISE can truncate files and MADVISE operates on memory, reverse
the audit_skip tags.

Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 io_uring/opdef.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 3aa0d65c50e3..a2bf53b4a38a 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
 	},
 	[IORING_OP_FADVISE] = {
 		.needs_file		= 1,
-		.audit_skip		= 1,
 		.name			= "FADVISE",
 		.prep			= io_fadvise_prep,
 		.issue			= io_fadvise,
 	},
 	[IORING_OP_MADVISE] = {
+		.audit_skip		= 1,
 		.name			= "MADVISE",
 		.prep			= io_madvise_prep,
 		.issue			= io_madvise,
-- 
2.27.0

[PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Richard Guy Briggs]

Getting XATTRs is not particularly interesting security-wise.

Suggested-by: Steve Grubb <sgrubb@redhat.com>
Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 io_uring/opdef.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index a2bf53b4a38a..f6bfe2cf078c 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
 	},
 	[IORING_OP_FGETXATTR] = {
 		.needs_file = 1,
+		.audit_skip		= 1,
 		.name			= "FGETXATTR",
 		.prep			= io_fgetxattr_prep,
 		.issue			= io_fgetxattr,
 		.cleanup		= io_xattr_cleanup,
 	},
 	[IORING_OP_GETXATTR] = {
+		.audit_skip		= 1,
 		.name			= "GETXATTR",
 		.prep			= io_getxattr_prep,
 		.issue			= io_getxattr,
-- 
2.27.0

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Jens Axboe]

On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> A couple of updates to the iouring ops audit bypass selections suggested in
> consultation with Steve Grubb.
> 
> Richard Guy Briggs (2):
>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
>   io_uring,audit: do not log IORING_OP_*GETXATTR
> 
>  io_uring/opdef.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Look fine to me - we should probably add stable to both of them, just
to keep things consistent across releases. I can queue them up for 6.3.

-- 
Jens Axboe

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Paul Moore]

On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> > A couple of updates to the iouring ops audit bypass selections suggested in
> > consultation with Steve Grubb.
> >
> > Richard Guy Briggs (2):
> >   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> >   io_uring,audit: do not log IORING_OP_*GETXATTR
> >
> >  io_uring/opdef.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Look fine to me - we should probably add stable to both of them, just
> to keep things consistent across releases. I can queue them up for 6.3.

Please hold off until I've had a chance to look them over ...

-- 
paul-moore.com

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Jens Axboe]

On 1/27/23 12:42 PM, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
>>> A couple of updates to the iouring ops audit bypass selections suggested in
>>> consultation with Steve Grubb.
>>>
>>> Richard Guy Briggs (2):
>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
>>>
>>>  io_uring/opdef.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> Look fine to me - we should probably add stable to both of them, just
>> to keep things consistent across releases. I can queue them up for 6.3.
> 
> Please hold off until I've had a chance to look them over ...

I haven't taken anything yet, for things like this I always let it
simmer until people have had a chance to do so.

-- 
Jens Axboe

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Paul Moore]

On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> the audit_skip tags.
>
> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  io_uring/opdef.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index 3aa0d65c50e3..a2bf53b4a38a 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
>         },
>         [IORING_OP_FADVISE] = {
>                 .needs_file             = 1,
> -               .audit_skip             = 1,
>                 .name                   = "FADVISE",
>                 .prep                   = io_fadvise_prep,
>                 .issue                  = io_fadvise,
>         },

I've never used posix_fadvise() or the associated fadvise64*()
syscalls, but from quickly reading the manpages and the
generic_fadvise() function in the kernel I'm missing where the fadvise
family of functions could be used to truncate a file, can you show me
where this happens?  The closest I can see is the manipulation of the
page cache, but that shouldn't actually modify the file ... right?

>         [IORING_OP_MADVISE] = {
> +               .audit_skip             = 1,
>                 .name                   = "MADVISE",
>                 .prep                   = io_madvise_prep,
>                 .issue                  = io_madvise,

I *think* this should be okay, what testing/verification have you done
on this?  One of the things I like to check is to see if any LSMs
might perform an access check and/or generate an audit record on an
operation, if there is a case where that could happen we should setup
audit properly.  I did a very quick check of do_madvise() and nothing
jumped out at me, but I would be interested in knowing what testing or
verification you did here.

-- 
paul-moore.com

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Paul Moore]

On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 12:42 PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> >>> A couple of updates to the iouring ops audit bypass selections suggested in
> >>> consultation with Steve Grubb.
> >>>
> >>> Richard Guy Briggs (2):
> >>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> >>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> >>>
> >>>  io_uring/opdef.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> Look fine to me - we should probably add stable to both of them, just
> >> to keep things consistent across releases. I can queue them up for 6.3.
> >
> > Please hold off until I've had a chance to look them over ...
>
> I haven't taken anything yet, for things like this I always let it
> simmer until people have had a chance to do so.

Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
of different behaviors across subsystems and wanted to make sure we
were on the same page.

-- 
paul-moore.com

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Paul Moore]

On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Getting XATTRs is not particularly interesting security-wise.
>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  io_uring/opdef.c | 2 ++
>  1 file changed, 2 insertions(+)

Depending on your security policy, fetching file data, including
xattrs, can be interesting from a security perspective.  As an
example, look at the SELinux file/getattr permission.

https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions

> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index a2bf53b4a38a..f6bfe2cf078c 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
>         },
>         [IORING_OP_FGETXATTR] = {
>                 .needs_file = 1,
> +               .audit_skip             = 1,
>                 .name                   = "FGETXATTR",
>                 .prep                   = io_fgetxattr_prep,
>                 .issue                  = io_fgetxattr,
>                 .cleanup                = io_xattr_cleanup,
>         },
>         [IORING_OP_GETXATTR] = {
> +               .audit_skip             = 1,
>                 .name                   = "GETXATTR",
>                 .prep                   = io_getxattr_prep,
>                 .issue                  = io_getxattr,
> --
> 2.27.0

-- 
paul-moore.com

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Jens Axboe]

On 1/27/23 3:35?PM, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>
>> Since FADVISE can truncate files and MADVISE operates on memory, reverse
>> the audit_skip tags.
>>
>> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> ---
>>  io_uring/opdef.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
>> index 3aa0d65c50e3..a2bf53b4a38a 100644
>> --- a/io_uring/opdef.c
>> +++ b/io_uring/opdef.c
>> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
>>         },
>>         [IORING_OP_FADVISE] = {
>>                 .needs_file             = 1,
>> -               .audit_skip             = 1,
>>                 .name                   = "FADVISE",
>>                 .prep                   = io_fadvise_prep,
>>                 .issue                  = io_fadvise,
>>         },
> 
> I've never used posix_fadvise() or the associated fadvise64*()
> syscalls, but from quickly reading the manpages and the
> generic_fadvise() function in the kernel I'm missing where the fadvise
> family of functions could be used to truncate a file, can you show me
> where this happens?  The closest I can see is the manipulation of the
> page cache, but that shouldn't actually modify the file ... right?

Yeah, honestly not sure where that came from. Maybe it's being mixed up
with fallocate? All fadvise (or madvise, for that matter) does is
provide hints on the caching or access pattern. On second thought, both
of these should be able to set audit_skip as far as I can tell.

-- 
Jens Axboe

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Jens Axboe]

On 1/27/23 3:38 PM, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/27/23 12:42 PM, Paul Moore wrote:
>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
>>>>> A couple of updates to the iouring ops audit bypass selections suggested in
>>>>> consultation with Steve Grubb.
>>>>>
>>>>> Richard Guy Briggs (2):
>>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
>>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
>>>>>
>>>>>  io_uring/opdef.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> Look fine to me - we should probably add stable to both of them, just
>>>> to keep things consistent across releases. I can queue them up for 6.3.
>>>
>>> Please hold off until I've had a chance to look them over ...
>>
>> I haven't taken anything yet, for things like this I always let it
>> simmer until people have had a chance to do so.
> 
> Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> of different behaviors across subsystems and wanted to make sure we
> were on the same page.

Sounds fair. BTW, can we stop CC'ing closed lists on patch
submissions? Getting these:

Your message to Linux-audit awaits moderator approval

on every reply is really annoying.

-- 
Jens Axboe

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Paul Moore]

On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 3:38 PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/27/23 12:42 PM, Paul Moore wrote:
> >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> >>>>> A couple of updates to the iouring ops audit bypass selections suggested in
> >>>>> consultation with Steve Grubb.
> >>>>>
> >>>>> Richard Guy Briggs (2):
> >>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> >>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> >>>>>
> >>>>>  io_uring/opdef.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> Look fine to me - we should probably add stable to both of them, just
> >>>> to keep things consistent across releases. I can queue them up for 6.3.
> >>>
> >>> Please hold off until I've had a chance to look them over ...
> >>
> >> I haven't taken anything yet, for things like this I always let it
> >> simmer until people have had a chance to do so.
> >
> > Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> > of different behaviors across subsystems and wanted to make sure we
> > were on the same page.
>
> Sounds fair. BTW, can we stop CC'ing closed lists on patch
> submissions? Getting these:
>
> Your message to Linux-audit awaits moderator approval
>
> on every reply is really annoying.

We kinda need audit related stuff on the linux-audit list, that's our
mailing list for audit stuff.

However, I agree that it is crap that the linux-audit list is
moderated, but unfortunately that isn't something I control (I haven't
worked for RH in years, and even then the list owner was really weird
about managing the list).  Occasionally I grumble about moving the
kernel audit development to a linux-audit list on vger but haven't
bothered yet, perhaps this is as good a reason as any.

Richard, Steve - any chance of opening the linux-audit list?

-- 
paul-moore.com

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Richard Guy Briggs]

On 2023-01-27 17:35, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Since FADVISE can truncate files and MADVISE operates on memory, reverse
> > the audit_skip tags.
> >
> > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  io_uring/opdef.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > index 3aa0d65c50e3..a2bf53b4a38a 100644
> > --- a/io_uring/opdef.c
> > +++ b/io_uring/opdef.c
> > @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> >         },
> >         [IORING_OP_FADVISE] = {
> >                 .needs_file             = 1,
> > -               .audit_skip             = 1,
> >                 .name                   = "FADVISE",
> >                 .prep                   = io_fadvise_prep,
> >                 .issue                  = io_fadvise,
> >         },
> 
> I've never used posix_fadvise() or the associated fadvise64*()
> syscalls, but from quickly reading the manpages and the
> generic_fadvise() function in the kernel I'm missing where the fadvise
> family of functions could be used to truncate a file, can you show me
> where this happens?  The closest I can see is the manipulation of the
> page cache, but that shouldn't actually modify the file ... right?

I don't know.  I was going on the advice of Steve Grubb.  I'm looking
for feedback, validation, correction, here.

> >         [IORING_OP_MADVISE] = {
> > +               .audit_skip             = 1,
> >                 .name                   = "MADVISE",
> >                 .prep                   = io_madvise_prep,
> >                 .issue                  = io_madvise,
> 
> I *think* this should be okay, what testing/verification have you done
> on this?  One of the things I like to check is to see if any LSMs
> might perform an access check and/or generate an audit record on an
> operation, if there is a case where that could happen we should setup
> audit properly.  I did a very quick check of do_madvise() and nothing
> jumped out at me, but I would be interested in knowing what testing or
> verification you did here.

No testing other than build/boot/audit-testsuite.  You had a test you
had developed that went through several iterations?

> paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Paul Moore]

On Fri, Jan 27, 2023 at 5:45 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 3:35?PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>
> >> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> >> the audit_skip tags.
> >>
> >> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> ---
> >>  io_uring/opdef.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> >> index 3aa0d65c50e3..a2bf53b4a38a 100644
> >> --- a/io_uring/opdef.c
> >> +++ b/io_uring/opdef.c
> >> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> >>         },
> >>         [IORING_OP_FADVISE] = {
> >>                 .needs_file             = 1,
> >> -               .audit_skip             = 1,
> >>                 .name                   = "FADVISE",
> >>                 .prep                   = io_fadvise_prep,
> >>                 .issue                  = io_fadvise,
> >>         },
> >
> > I've never used posix_fadvise() or the associated fadvise64*()
> > syscalls, but from quickly reading the manpages and the
> > generic_fadvise() function in the kernel I'm missing where the fadvise
> > family of functions could be used to truncate a file, can you show me
> > where this happens?  The closest I can see is the manipulation of the
> > page cache, but that shouldn't actually modify the file ... right?
>
> Yeah, honestly not sure where that came from. Maybe it's being mixed up
> with fallocate?

That was my thought too when I was looking at it.

> All fadvise (or madvise, for that matter) does is
> provide hints on the caching or access pattern. On second thought, both
> of these should be able to set audit_skip as far as I can tell.

Agreed on the fadvise side, and probably the madvise side too,
although the latter has more options/code to sift through so I'm
curious to hear what analysis Richard has done on that one.

-- 
paul-moore.com

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Richard Guy Briggs]

On 2023-01-27 17:43, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Getting XATTRs is not particularly interesting security-wise.
> >
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  io_uring/opdef.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> Depending on your security policy, fetching file data, including
> xattrs, can be interesting from a security perspective.  As an
> example, look at the SELinux file/getattr permission.
> 
> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions

The intent here is to lessen the impact of audit operations.  Read and
Write were explicitly removed from io_uring auditing due to performance
concerns coupled with the denial of service implications from sheer
volume of records making other messages harder to locate.  Those
operations are still possible for syscall auditing but they are strongly
discouraged for normal use.

If the frequency of getxattr io_uring ops is so infrequent as to be no
distraction, then this patch may be more of a liability than a benefit.

> > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > index a2bf53b4a38a..f6bfe2cf078c 100644
> > --- a/io_uring/opdef.c
> > +++ b/io_uring/opdef.c
> > @@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
> >         },
> >         [IORING_OP_FGETXATTR] = {
> >                 .needs_file = 1,
> > +               .audit_skip             = 1,
> >                 .name                   = "FGETXATTR",
> >                 .prep                   = io_fgetxattr_prep,
> >                 .issue                  = io_fgetxattr,
> >                 .cleanup                = io_xattr_cleanup,
> >         },
> >         [IORING_OP_GETXATTR] = {
> > +               .audit_skip             = 1,
> >                 .name                   = "GETXATTR",
> >                 .prep                   = io_getxattr_prep,
> >                 .issue                  = io_getxattr,
> > --
> > 2.27.0
> 
> -- 
> paul-moore.com
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Jens Axboe]

On 1/27/23 3:53 PM, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/27/23 3:38 PM, Paul Moore wrote:
>>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/27/23 12:42 PM, Paul Moore wrote:
>>>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
>>>>>>> A couple of updates to the iouring ops audit bypass selections suggested in
>>>>>>> consultation with Steve Grubb.
>>>>>>>
>>>>>>> Richard Guy Briggs (2):
>>>>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
>>>>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
>>>>>>>
>>>>>>>  io_uring/opdef.c | 4 +++-
>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> Look fine to me - we should probably add stable to both of them, just
>>>>>> to keep things consistent across releases. I can queue them up for 6.3.
>>>>>
>>>>> Please hold off until I've had a chance to look them over ...
>>>>
>>>> I haven't taken anything yet, for things like this I always let it
>>>> simmer until people have had a chance to do so.
>>>
>>> Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
>>> of different behaviors across subsystems and wanted to make sure we
>>> were on the same page.
>>
>> Sounds fair. BTW, can we stop CC'ing closed lists on patch
>> submissions? Getting these:
>>
>> Your message to Linux-audit awaits moderator approval
>>
>> on every reply is really annoying.
> 
> We kinda need audit related stuff on the linux-audit list, that's our
> mailing list for audit stuff.

Sure, but then it should be open. Or do separate postings or something.
CC'ing a closed list with open lists and sending email to people that
are not on that closed list is bad form.

-- 
Jens Axboe

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Richard Guy Briggs]

On 2023-01-27 15:45, Jens Axboe wrote:
> On 1/27/23 3:35?PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>
> >> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> >> the audit_skip tags.
> >>
> >> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> ---
> >>  io_uring/opdef.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> >> index 3aa0d65c50e3..a2bf53b4a38a 100644
> >> --- a/io_uring/opdef.c
> >> +++ b/io_uring/opdef.c
> >> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> >>         },
> >>         [IORING_OP_FADVISE] = {
> >>                 .needs_file             = 1,
> >> -               .audit_skip             = 1,
> >>                 .name                   = "FADVISE",
> >>                 .prep                   = io_fadvise_prep,
> >>                 .issue                  = io_fadvise,
> >>         },
> > 
> > I've never used posix_fadvise() or the associated fadvise64*()
> > syscalls, but from quickly reading the manpages and the
> > generic_fadvise() function in the kernel I'm missing where the fadvise
> > family of functions could be used to truncate a file, can you show me
> > where this happens?  The closest I can see is the manipulation of the
> > page cache, but that shouldn't actually modify the file ... right?
> 
> Yeah, honestly not sure where that came from. Maybe it's being mixed up
> with fallocate? All fadvise (or madvise, for that matter) does is
> provide hints on the caching or access pattern. On second thought, both
> of these should be able to set audit_skip as far as I can tell.

That was one suspicion I had.  If this is the case, I'd agree both could
be skipped.

> Jens Axboe

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Jens Axboe]

On 1/27/23 4:02 PM, Richard Guy Briggs wrote:
> On 2023-01-27 15:45, Jens Axboe wrote:
>> On 1/27/23 3:35?PM, Paul Moore wrote:
>>> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>>>
>>>> Since FADVISE can truncate files and MADVISE operates on memory, reverse
>>>> the audit_skip tags.
>>>>
>>>> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
>>>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>>> ---
>>>>  io_uring/opdef.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
>>>> index 3aa0d65c50e3..a2bf53b4a38a 100644
>>>> --- a/io_uring/opdef.c
>>>> +++ b/io_uring/opdef.c
>>>> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
>>>>         },
>>>>         [IORING_OP_FADVISE] = {
>>>>                 .needs_file             = 1,
>>>> -               .audit_skip             = 1,
>>>>                 .name                   = "FADVISE",
>>>>                 .prep                   = io_fadvise_prep,
>>>>                 .issue                  = io_fadvise,
>>>>         },
>>>
>>> I've never used posix_fadvise() or the associated fadvise64*()
>>> syscalls, but from quickly reading the manpages and the
>>> generic_fadvise() function in the kernel I'm missing where the fadvise
>>> family of functions could be used to truncate a file, can you show me
>>> where this happens?  The closest I can see is the manipulation of the
>>> page cache, but that shouldn't actually modify the file ... right?
>>
>> Yeah, honestly not sure where that came from. Maybe it's being mixed up
>> with fallocate? All fadvise (or madvise, for that matter) does is
>> provide hints on the caching or access pattern. On second thought, both
>> of these should be able to set audit_skip as far as I can tell.
> 
> That was one suspicion I had.  If this is the case, I'd agree both could
> be skipped.

I'd be surprised if Steve didn't mix them up. Once he responds, can you
send a v2 with the correction?

-- 
Jens Axboe

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Jens Axboe]

On 1/27/23 4:01 PM, Richard Guy Briggs wrote:
> On 2023-01-27 17:43, Paul Moore wrote:
>> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>> Getting XATTRs is not particularly interesting security-wise.
>>>
>>> Suggested-by: Steve Grubb <sgrubb@redhat.com>
>>> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
>>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>> ---
>>>  io_uring/opdef.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>
>> Depending on your security policy, fetching file data, including
>> xattrs, can be interesting from a security perspective.  As an
>> example, look at the SELinux file/getattr permission.
>>
>> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> 
> The intent here is to lessen the impact of audit operations.  Read and
> Write were explicitly removed from io_uring auditing due to performance
> concerns coupled with the denial of service implications from sheer
> volume of records making other messages harder to locate.  Those
> operations are still possible for syscall auditing but they are strongly
> discouraged for normal use.
> 
> If the frequency of getxattr io_uring ops is so infrequent as to be no
> distraction, then this patch may be more of a liability than a benefit.

(audit list removed)

Right now the xattr related functions are io-wq driven, and hence not
super performance sensitive. But I'd greatly prefer to clean these up
regardless, because once opcodes get upgraded from needing io-wq, then
we don't have to go through the audit discussion at that point. Better
to do it upfront, like now, regardless of expectation of frequency of
calls.

-- 
Jens Axboe

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Paul Moore]

On Fri, Jan 27, 2023 at 5:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2023-01-27 17:35, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > Since FADVISE can truncate files and MADVISE operates on memory, reverse
> > > the audit_skip tags.
> > >
> > > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  io_uring/opdef.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > > index 3aa0d65c50e3..a2bf53b4a38a 100644
> > > --- a/io_uring/opdef.c
> > > +++ b/io_uring/opdef.c
> > > @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> > >         },
> > >         [IORING_OP_FADVISE] = {
> > >                 .needs_file             = 1,
> > > -               .audit_skip             = 1,
> > >                 .name                   = "FADVISE",
> > >                 .prep                   = io_fadvise_prep,
> > >                 .issue                  = io_fadvise,
> > >         },
> >
> > I've never used posix_fadvise() or the associated fadvise64*()
> > syscalls, but from quickly reading the manpages and the
> > generic_fadvise() function in the kernel I'm missing where the fadvise
> > family of functions could be used to truncate a file, can you show me
> > where this happens?  The closest I can see is the manipulation of the
> > page cache, but that shouldn't actually modify the file ... right?
>
> I don't know.  I was going on the advice of Steve Grubb.  I'm looking
> for feedback, validation, correction, here.

Keep in mind it's your name on the patch, not Steve's, and I would
hope that you should be able to stand up and vouch for your own patch.
Something to keep in mind for the future.

As it stands, I think the audit_skip line should stay for
IORING_OP_FADVISE, if you feel otherwise please provide more
explanation as to why auditing is necessary for this operation.

> > >         [IORING_OP_MADVISE] = {
> > > +               .audit_skip             = 1,
> > >                 .name                   = "MADVISE",
> > >                 .prep                   = io_madvise_prep,
> > >                 .issue                  = io_madvise,
> >
> > I *think* this should be okay, what testing/verification have you done
> > on this?  One of the things I like to check is to see if any LSMs
> > might perform an access check and/or generate an audit record on an
> > operation, if there is a case where that could happen we should setup
> > audit properly.  I did a very quick check of do_madvise() and nothing
> > jumped out at me, but I would be interested in knowing what testing or
> > verification you did here.
>
> No testing other than build/boot/audit-testsuite.  You had a test you
> had developed that went through several iterations?

There is an io_uring test in the audit-testsuite that verifies basic
audit record generation, it is not exhaustive.

Patch 2/2 is a no-go from a security perspective (we want to see those
records), and I think skipping on IORING_OP_FADVISE is the correct
thing to do.  It may be that skipping on IORING_OP_MADVISE is the
correct thing, but given that it doesn't appear a lot of in-depth
investigation has gone into these patches I would really like to see
some more reasoning on this before we change the current behavior.

-- 
paul-moore.com

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Richard Guy Briggs]

On 2023-01-27 16:02, Jens Axboe wrote:
> On 1/27/23 3:53 PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/27/23 3:38 PM, Paul Moore wrote:
> >>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 1/27/23 12:42 PM, Paul Moore wrote:
> >>>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> >>>>>>> A couple of updates to the iouring ops audit bypass selections suggested in
> >>>>>>> consultation with Steve Grubb.
> >>>>>>>
> >>>>>>> Richard Guy Briggs (2):
> >>>>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> >>>>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> >>>>>>>
> >>>>>>>  io_uring/opdef.c | 4 +++-
> >>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> Look fine to me - we should probably add stable to both of them, just
> >>>>>> to keep things consistent across releases. I can queue them up for 6.3.
> >>>>>
> >>>>> Please hold off until I've had a chance to look them over ...
> >>>>
> >>>> I haven't taken anything yet, for things like this I always let it
> >>>> simmer until people have had a chance to do so.
> >>>
> >>> Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> >>> of different behaviors across subsystems and wanted to make sure we
> >>> were on the same page.
> >>
> >> Sounds fair. BTW, can we stop CC'ing closed lists on patch
> >> submissions? Getting these:
> >>
> >> Your message to Linux-audit awaits moderator approval
> >>
> >> on every reply is really annoying.
> > 
> > We kinda need audit related stuff on the linux-audit list, that's our
> > mailing list for audit stuff.
> 
> Sure, but then it should be open. Or do separate postings or something.
> CC'ing a closed list with open lists and sending email to people that
> are not on that closed list is bad form.

I've made an inquiry.

> Jens Axboe

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Richard Guy Briggs]

On 2023-01-27 16:03, Jens Axboe wrote:
> On 1/27/23 4:02 PM, Richard Guy Briggs wrote:
> > On 2023-01-27 15:45, Jens Axboe wrote:
> >> On 1/27/23 3:35?PM, Paul Moore wrote:
> >>> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>>>
> >>>> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> >>>> the audit_skip tags.
> >>>>
> >>>> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")
> >>>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >>>> ---
> >>>>  io_uring/opdef.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> >>>> index 3aa0d65c50e3..a2bf53b4a38a 100644
> >>>> --- a/io_uring/opdef.c
> >>>> +++ b/io_uring/opdef.c
> >>>> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> >>>>         },
> >>>>         [IORING_OP_FADVISE] = {
> >>>>                 .needs_file             = 1,
> >>>> -               .audit_skip             = 1,
> >>>>                 .name                   = "FADVISE",
> >>>>                 .prep                   = io_fadvise_prep,
> >>>>                 .issue                  = io_fadvise,
> >>>>         },
> >>>
> >>> I've never used posix_fadvise() or the associated fadvise64*()
> >>> syscalls, but from quickly reading the manpages and the
> >>> generic_fadvise() function in the kernel I'm missing where the fadvise
> >>> family of functions could be used to truncate a file, can you show me
> >>> where this happens?  The closest I can see is the manipulation of the
> >>> page cache, but that shouldn't actually modify the file ... right?
> >>
> >> Yeah, honestly not sure where that came from. Maybe it's being mixed up
> >> with fallocate? All fadvise (or madvise, for that matter) does is
> >> provide hints on the caching or access pattern. On second thought, both
> >> of these should be able to set audit_skip as far as I can tell.
> > 
> > That was one suspicion I had.  If this is the case, I'd agree both could
> > be skipped.
> 
> I'd be surprised if Steve didn't mix them up. Once he responds, can you
> send a v2 with the correction?

Gladly.

> Jens Axboe

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Paul Moore]

On Fri, Jan 27, 2023 at 6:02 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 3:53 PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/27/23 3:38 PM, Paul Moore wrote:
> >>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 1/27/23 12:42 PM, Paul Moore wrote:
> >>>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> >>>>>>> A couple of updates to the iouring ops audit bypass selections suggested in
> >>>>>>> consultation with Steve Grubb.
> >>>>>>>
> >>>>>>> Richard Guy Briggs (2):
> >>>>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> >>>>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> >>>>>>>
> >>>>>>>  io_uring/opdef.c | 4 +++-
> >>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> Look fine to me - we should probably add stable to both of them, just
> >>>>>> to keep things consistent across releases. I can queue them up for 6.3.
> >>>>>
> >>>>> Please hold off until I've had a chance to look them over ...
> >>>>
> >>>> I haven't taken anything yet, for things like this I always let it
> >>>> simmer until people have had a chance to do so.
> >>>
> >>> Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> >>> of different behaviors across subsystems and wanted to make sure we
> >>> were on the same page.
> >>
> >> Sounds fair. BTW, can we stop CC'ing closed lists on patch
> >> submissions? Getting these:
> >>
> >> Your message to Linux-audit awaits moderator approval
> >>
> >> on every reply is really annoying.
> >
> > We kinda need audit related stuff on the linux-audit list, that's our
> > mailing list for audit stuff.
>
> Sure, but then it should be open. Or do separate postings or something.
> CC'ing a closed list with open lists and sending email to people that
> are not on that closed list is bad form.

Agree, that's why I said in my reply that it was crap that the
linux-audit list is moderated and asked Richard/Steve to open it up.

-- 
paul-moore.com

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Jens Axboe]

On 1/27/23 4:08 PM, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 6:02 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/27/23 3:53 PM, Paul Moore wrote:
>>> On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/27/23 3:38 PM, Paul Moore wrote:
>>>>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 1/27/23 12:42 PM, Paul Moore wrote:
>>>>>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
>>>>>>>>> A couple of updates to the iouring ops audit bypass selections suggested in
>>>>>>>>> consultation with Steve Grubb.
>>>>>>>>>
>>>>>>>>> Richard Guy Briggs (2):
>>>>>>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
>>>>>>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
>>>>>>>>>
>>>>>>>>>  io_uring/opdef.c | 4 +++-
>>>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> Look fine to me - we should probably add stable to both of them, just
>>>>>>>> to keep things consistent across releases. I can queue them up for 6.3.
>>>>>>>
>>>>>>> Please hold off until I've had a chance to look them over ...
>>>>>>
>>>>>> I haven't taken anything yet, for things like this I always let it
>>>>>> simmer until people have had a chance to do so.
>>>>>
>>>>> Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
>>>>> of different behaviors across subsystems and wanted to make sure we
>>>>> were on the same page.
>>>>
>>>> Sounds fair. BTW, can we stop CC'ing closed lists on patch
>>>> submissions? Getting these:
>>>>
>>>> Your message to Linux-audit awaits moderator approval
>>>>
>>>> on every reply is really annoying.
>>>
>>> We kinda need audit related stuff on the linux-audit list, that's our
>>> mailing list for audit stuff.
>>
>> Sure, but then it should be open. Or do separate postings or something.
>> CC'ing a closed list with open lists and sending email to people that
>> are not on that closed list is bad form.
> 
> Agree, that's why I said in my reply that it was crap that the
> linux-audit list is moderated and asked Richard/Steve to open it up.

And thanks for that, I just skipped it in the reply as it wasn't for
me.

-- 
Jens Axboe

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Paul Moore]

On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2023-01-27 17:43, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  io_uring/opdef.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective.  As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
>
> The intent here is to lessen the impact of audit operations.  Read and
> Write were explicitly removed from io_uring auditing due to performance
> concerns coupled with the denial of service implications from sheer
> volume of records making other messages harder to locate.  Those
> operations are still possible for syscall auditing but they are strongly
> discouraged for normal use.

We need to balance security needs and performance needs.  You are
correct that general read() and write() operations are not audited,
and generally not checked from a LSM perspective as the auditing and
access control happens at open() time instead (access to fds is
revalidated when they are passed).  However, in the case of getxattr
and fgetxattr, these are not normal file read operations, and do not
go through the same code path in the kernel; there is a reason why we
have xattr_permission() and security_inode_getxattr().

We need to continue to audit IORING_OP_FGETXATTR and IORING_OP_GETXATTR.

-- 
paul-moore.com

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Paul Moore]

On Fri, Jan 27, 2023 at 6:05 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/27/23 4:01 PM, Richard Guy Briggs wrote:
> > On 2023-01-27 17:43, Paul Moore wrote:
> >> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>> Getting XATTRs is not particularly interesting security-wise.
> >>>
> >>> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> >>> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >>> ---
> >>>  io_uring/opdef.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>
> >> Depending on your security policy, fetching file data, including
> >> xattrs, can be interesting from a security perspective.  As an
> >> example, look at the SELinux file/getattr permission.
> >>
> >> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> >
> > The intent here is to lessen the impact of audit operations.  Read and
> > Write were explicitly removed from io_uring auditing due to performance
> > concerns coupled with the denial of service implications from sheer
> > volume of records making other messages harder to locate.  Those
> > operations are still possible for syscall auditing but they are strongly
> > discouraged for normal use.
> >
> > If the frequency of getxattr io_uring ops is so infrequent as to be no
> > distraction, then this patch may be more of a liability than a benefit.
>
> (audit list removed)
>
> Right now the xattr related functions are io-wq driven, and hence not
> super performance sensitive. But I'd greatly prefer to clean these up
> regardless, because once opcodes get upgraded from needing io-wq, then
> we don't have to go through the audit discussion at that point. Better
> to do it upfront, like now, regardless of expectation of frequency of
> calls.

See my reply to Richard, but unfortunately we need to continue to
audit the getxattr ops.

-- 
paul-moore.com

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Richard Guy Briggs]

On 2023-01-27 19:06, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2023-01-27 17:43, Paul Moore wrote:
> > > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Getting XATTRs is not particularly interesting security-wise.
> > > >
> > > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  io_uring/opdef.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > >
> > > Depending on your security policy, fetching file data, including
> > > xattrs, can be interesting from a security perspective.  As an
> > > example, look at the SELinux file/getattr permission.
> > >
> > > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> >
> > The intent here is to lessen the impact of audit operations.  Read and
> > Write were explicitly removed from io_uring auditing due to performance
> > concerns coupled with the denial of service implications from sheer
> > volume of records making other messages harder to locate.  Those
> > operations are still possible for syscall auditing but they are strongly
> > discouraged for normal use.
> 
> We need to balance security needs and performance needs.  You are
> correct that general read() and write() operations are not audited,
> and generally not checked from a LSM perspective as the auditing and
> access control happens at open() time instead (access to fds is
> revalidated when they are passed).  However, in the case of getxattr
> and fgetxattr, these are not normal file read operations, and do not
> go through the same code path in the kernel; there is a reason why we
> have xattr_permission() and security_inode_getxattr().
> 
> We need to continue to audit IORING_OP_FGETXATTR and IORING_OP_GETXATTR.

Fair enough.  This would be similar reasoning to send/recv vs
sendmsg/recvmsg.  I'll drop this patch.  Thanks for the reasoning and
feedback.

> paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Steve Grubb]

On Friday, January 27, 2023 5:53:24 PM EST Paul Moore wrote:
> On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
> > On 1/27/23 3:38 PM, Paul Moore wrote:
> > > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >> On 1/27/23 12:42 PM, Paul Moore wrote:
> > >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> > >>>>> A couple of updates to the iouring ops audit bypass selections
> > >>>>> suggested in consultation with Steve Grubb.
> > >>>>> 
> > >>>>> Richard Guy Briggs (2):
> > >>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> > >>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> > >>>>>  
> > >>>>>  io_uring/opdef.c | 4 +++-
> > >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>> 
> > >>>> Look fine to me - we should probably add stable to both of them,
> > >>>> just to keep things consistent across releases. I can queue them up
> > >>>> for 6.3.
> > >>> 
> > >>> Please hold off until I've had a chance to look them over ...
> > >> 
> > >> I haven't taken anything yet, for things like this I always let it
> > >> simmer until people have had a chance to do so.
> > > 
> > > Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> > > of different behaviors across subsystems and wanted to make sure we
> > > were on the same page.
> > 
> > Sounds fair. BTW, can we stop CC'ing closed lists on patch
> > submissions? Getting these:
> > 
> > Your message to Linux-audit awaits moderator approval
> > 
> > on every reply is really annoying.
> 
> We kinda need audit related stuff on the linux-audit list, that's our
> mailing list for audit stuff.
> 
> However, I agree that it is crap that the linux-audit list is
> moderated, but unfortunately that isn't something I control (I haven't
> worked for RH in years, and even then the list owner was really weird
> about managing the list).  Occasionally I grumble about moving the
> kernel audit development to a linux-audit list on vger but haven't
> bothered yet, perhaps this is as good a reason as any.
> 
> Richard, Steve - any chance of opening the linux-audit list?

Unfortunately, it really has to be this way. I deleted 10 spam emails 
yesterday. It seems like some people subscribed to this list are compromised. 
Because everytime there is a legit email, it's followed in a few seconds by a 
spam email.

Anyways, all legit email will be approved without needing to be subscribed.

-Steve

Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE

[Steve Grubb]

On Friday, January 27, 2023 5:57:30 PM EST Paul Moore wrote:
> On Fri, Jan 27, 2023 at 5:45 PM Jens Axboe <axboe@kernel.dk> wrote:
> > On 1/27/23 3:35?PM, Paul Moore wrote:
> > > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> 
wrote:
> > >> Since FADVISE can truncate files and MADVISE operates on memory,
> > >> reverse
> > >> the audit_skip tags.
> > >> 
> > >> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit
> > >> support to io_uring") Signed-off-by: Richard Guy Briggs
> > >> <rgb@redhat.com>
> > >> ---
> > >> 
> > >>  io_uring/opdef.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> 
> > >> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > >> index 3aa0d65c50e3..a2bf53b4a38a 100644
> > >> --- a/io_uring/opdef.c
> > >> +++ b/io_uring/opdef.c
> > >> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> > >> 
> > >>         },
> > >>         [IORING_OP_FADVISE] = {
> > >>         
> > >>                 .needs_file             = 1,
> > >> 
> > >> -               .audit_skip             = 1,
> > >> 
> > >>                 .name                   = "FADVISE",
> > >>                 .prep                   = io_fadvise_prep,
> > >>                 .issue                  = io_fadvise,
> > >>         
> > >>         },
> > > 
> > > I've never used posix_fadvise() or the associated fadvise64*()
> > > syscalls, but from quickly reading the manpages and the
> > > generic_fadvise() function in the kernel I'm missing where the fadvise
> > > family of functions could be used to truncate a file, can you show me
> > > where this happens?  The closest I can see is the manipulation of the
> > > page cache, but that shouldn't actually modify the file ... right?
> > 
> > Yeah, honestly not sure where that came from. Maybe it's being mixed up
> > with fallocate?
> 
> That was my thought too when I was looking at it.

Oh. Yeah. fallocate is the one that truncates. fadvise can be skipped.

-Steve

> > All fadvise (or madvise, for that matter) does is
> > provide hints on the caching or access pattern. On second thought, both
> > of these should be able to set audit_skip as far as I can tell.
> 
> Agreed on the fadvise side, and probably the madvise side too,
> although the latter has more options/code to sift through so I'm
> curious to hear what analysis Richard has done on that one.

Re: [PATCH v1 0/2] two suggested iouring op audit updates

[Paul Moore]

On Sat, Jan 28, 2023 at 11:48 AM Steve Grubb <sgrubb@redhat.com> wrote:
> On Friday, January 27, 2023 5:53:24 PM EST Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe <axboe@kernel.dk> wrote:
> > > On 1/27/23 3:38 PM, Paul Moore wrote:
> > > > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> > > >> On 1/27/23 12:42 PM, Paul Moore wrote:
> > > >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
> > > >>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote:
> > > >>>>> A couple of updates to the iouring ops audit bypass selections
> > > >>>>> suggested in consultation with Steve Grubb.
> > > >>>>>
> > > >>>>> Richard Guy Briggs (2):
> > > >>>>>   io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
> > > >>>>>   io_uring,audit: do not log IORING_OP_*GETXATTR
> > > >>>>>
> > > >>>>>  io_uring/opdef.c | 4 +++-
> > > >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>> Look fine to me - we should probably add stable to both of them,
> > > >>>> just to keep things consistent across releases. I can queue them up
> > > >>>> for 6.3.
> > > >>>
> > > >>> Please hold off until I've had a chance to look them over ...
> > > >>
> > > >> I haven't taken anything yet, for things like this I always let it
> > > >> simmer until people have had a chance to do so.
> > > >
> > > > Thanks.  FWIW, that sounds very reasonable to me, but I've seen lots
> > > > of different behaviors across subsystems and wanted to make sure we
> > > > were on the same page.
> > >
> > > Sounds fair. BTW, can we stop CC'ing closed lists on patch
> > > submissions? Getting these:
> > >
> > > Your message to Linux-audit awaits moderator approval
> > >
> > > on every reply is really annoying.
> >
> > We kinda need audit related stuff on the linux-audit list, that's our
> > mailing list for audit stuff.
> >
> > However, I agree that it is crap that the linux-audit list is
> > moderated, but unfortunately that isn't something I control (I haven't
> > worked for RH in years, and even then the list owner was really weird
> > about managing the list).  Occasionally I grumble about moving the
> > kernel audit development to a linux-audit list on vger but haven't
> > bothered yet, perhaps this is as good a reason as any.
> >
> > Richard, Steve - any chance of opening the linux-audit list?
>
> Unfortunately, it really has to be this way. I deleted 10 spam emails
> yesterday. It seems like some people subscribed to this list are compromised.
> Because everytime there is a legit email, it's followed in a few seconds by a
> spam email.
>
> Anyways, all legit email will be approved without needing to be subscribed.

The problem is that other subsystem developers who aren't subscribed
to the linux-audit list end up getting held mail notices (see the
comments from Jens).  The moderation of linux-audit, as permissive as
it may be for proper emails, is a problem for upstream linux audit
development, I would say much more so than 10/day mails.

If you are unable/unwilling to switch linux-audit over to an open
mailing list we should revisit moving over to a vger list; at least
for upstream kernel development, you are welcome to stick with the
existing redhat.com list for discussion of your userspace tools.

-- 
paul-moore.com

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Steve Grubb]

On Friday, January 27, 2023 5:43:02 PM EST Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Getting XATTRs is not particularly interesting security-wise.
> > 
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > io_uring/opdef.c | 2 ++
> > 1 file changed, 2 insertions(+)
> 
> Depending on your security policy, fetching file data, including
> xattrs, can be interesting from a security perspective.  As an
> example, look at the SELinux file/getattr permission.
> 
> https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_cla
> sses_permissions.md#common-file-permissions

We're mostly interested in setting attributes because that changes policy. 
Reading them is not interesting unless the access fails with EPERM.

I was updating the user space piece recently and saw there was a bunch of 
"new" operations. I was commenting that we need to audit 5 or 6 of the "new" 
operations such as IORING_OP_MKDIRATor IORING_OP_SETXATTR. But now that I see 
the patch, it looks like they are auditable and we can just let a couple be 
skipped. IORING_OP_MADVISE is not interesting as it just gives hiints about 
the expected access patterns of memory. If there were an equivalent of 
mprotect, that would be of interest, but not madvise.

There are some I'm not sure about such as IORING_OP_MSG_RING and 
IORING_OP_URING_CMD. What do they do?

-Steve

Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR

[Paul Moore]

On Sat, Jan 28, 2023 at 12:26 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Friday, January 27, 2023 5:43:02 PM EST Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > io_uring/opdef.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective.  As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_cla
> > sses_permissions.md#common-file-permissions
>
> We're mostly interested in setting attributes because that changes policy.
> Reading them is not interesting unless the access fails with EPERM.

See my earlier comments, SELinux does have provisions for caring about
reading xattrs, and now that I look at the rest of the LSMs it looks
like Smack cares about reading xattrs too.  Regardless of whether a
given security policy cares about xattr access, the LSMs support
enforcing access on reading xattrs so we need to ensure the audit is
setup properly in these cases.

> I was updating the user space piece recently and saw there was a bunch of
> "new" operations. I was commenting that we need to audit 5 or 6 of the "new"
> operations such as IORING_OP_MKDIRATor IORING_OP_SETXATTR. But now that I see
> the patch, it looks like they are auditable and we can just let a couple be
> skipped. IORING_OP_MADVISE is not interesting as it just gives hiints about
> the expected access patterns of memory. If there were an equivalent of
> mprotect, that would be of interest, but not madvise.

Once again, as discussed previously, it is likely that skipping
auditing for IORING_OP_MADVISE is okay, but given that several of the
changes in this patchset were incorrect, I'd like a little more
thorough investigation before we skip auditing on madvise.

> There are some I'm not sure about such as IORING_OP_MSG_RING and
> IORING_OP_URING_CMD. What do they do?

Look at 4f57f06ce218 ("io_uring: add support for IORING_OP_MSG_RING
command") for the patch which added IORING_OP_MSG_RING as it has a
decent commit description.  As for IORING_OP_URING_CMD, there were
lengthy discussions about it on the mailing lists (including audit)
back in March 2022 and then later in August on the LSM, SELinux, etc.
mailing lists when we landed some patches for it (there were no audit
changes).  I also covered the IORING_OP_URING_CMD, albeit briefly, in
a presentation at LSS-EU last year:

https://www.youtube.com/watch?v=AaaH6skUEI8
https://www.paul-moore.com/docs/2022-lss_eu-iouring_lsm-pcmoore-r3.pdf

--
paul-moore.com