| Message ID | 68eb0c2dd50bca1af91203669f7f1f8312331f38.1674682056.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | two suggested iouring op audit updates | expand |
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.
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.
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
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.
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
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?
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.
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
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.
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,
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(-)