Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On 2023-01-27 19:06, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 6: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
> > > wrote:
> > > > Getting XATTRs is not particularly interesting security-wise.
> > > >
> > > > Suggested-by: Steve Grubb
> > > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > 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
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 6: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 wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 6:02 PM Jens Axboe wrote: > On 1/27/23 3:53 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe wrote: > >> On 1/27/23 3:38 PM, Paul Moore wrote: > >>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > On 1/27/23 12:42 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
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
> >>> 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
> ---
> 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
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
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 wrote: > >> On 1/27/23 3:38 PM, Paul Moore wrote: > >>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > On 1/27/23 12:42 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 5:55 PM Richard Guy Briggs wrote:
> On 2023-01-27 17:35, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs 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
> > > ---
> > > 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
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 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
> >> ---
> >> 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
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On 2023-01-27 17:43, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > Getting XATTRs is not particularly interesting security-wise.
> >
> > Suggested-by: Steve Grubb
> > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > Signed-off-by: Richard Guy Briggs
> > ---
> > 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
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 5:45 PM 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 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
> >> ---
> >> 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On 2023-01-27 17:35, Paul Moore wrote:
> On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs 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
> > ---
> > 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
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe wrote: > On 1/27/23 3:38 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > >> On 1/27/23 12:42 PM, Paul Moore wrote: > >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
>
> Getting XATTRs is not particularly interesting security-wise.
>
> Suggested-by: Steve Grubb
> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> Signed-off-by: Richard Guy Briggs
> ---
> 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > On 1/27/23 12:42 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs 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
> ---
> 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify, audit: Allow audit to use the full permission event response
On Friday, January 27, 2023 3:00:37 PM EST Paul Moore wrote:
> On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs wrote:
> > On 2023-01-20 13:52, Paul Moore wrote:
> > > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb wrote:
> > > > Hello Richard,
> > > >
> > > > I built a new kernel and tested this with old and new user space. It
> > > > is
> > > > working as advertised. The only thing I'm wondering about is why we
> > > > have 3F as the default value when no additional info was sent? Would
> > > > it be better to just make it 0?
> > >
> > > ...
> > >
> > > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote:
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index d1fb821de104..3133c4175c15 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > > >
> > > > > context->type = AUDIT_KERN_MODULE;
> > > > >
> > > > > }
> > > > >
> > > > > -void __audit_fanotify(u32 response)
> > > > > +void __audit_fanotify(u32 response, struct
> > > > > fanotify_response_info_audit_rule *friar) {
> > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > AUDIT_FANOTIFY,
> > > > > + "resp=%u fan_type=%u fan_info=3F
> > > > > subj_trust=2
> > > >
> > > > obj_trust=2",
> > > >
> > > > > + response, FAN_RESPONSE_INFO_NONE);
> > > > > + return;
> > > > > + }
> > >
> > > (I'm working under the assumption that the "fan_info=3F" in the record
> > > above is what Steve was referring to in his comment.)
> > >
> > > I vaguely recall Richard commenting on this in the past, although
> > > maybe not ... my thought is that the "3F" is simply the hex encoded
> > > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the
> > > question is what to do in the FAN_RESPONSE_INFO_NONE case.
> > >
> > > Historically when we had a missing field we would follow the "field=?"
> > > pattern, but I don't recall doing that for a field which was
> > > potentially hex encoded, is there an existing case where we use "?"
> > > for a field that is hex encoded? If so, we can swap out the "3F" for
> > > a more obvious "?".
> >
> > I was presuming encoding the zero: "30"
>
> I'm sorry, but you've lost me here.
>
> > > However, another option might be to simply output the current
> > > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g.
> > > only "resp=%u". This is a little against the usual guidance of
> > > "fields should not disappear from a record", but considering that
> > > userspace will always need to support the original resp-only format
> > > for compatibility reasons this may be an option.
> >
> > I don't have a strong opinion.
>
> I'm not sure I care too much either. I will admit that the "3F" seems
> to be bordering on the "bit too clever" side of things, but it's easy
> to argue it is in keeping with the general idea of using "?" to denote
> absent/unknown fields.
The translation will be from %X to %u. In that case, someone might think 63
has some meaning. It would be better to leave it as 0 so there's less to
explain.
-Steve
> As Steve was the one who raised the question in this latest round, and
> he knows his userspace tools the best, it seems wise to get his input
> on this.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Wed, Jan 25, 2023 at 5:11 PM Richard Guy Briggs wrote:
>
> On 2023-01-20 13:58, Paul Moore wrote:
> > On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs wrote:
> > >
> > > This patch passes the full response so that the audit function can use all
> > > of it. The audit function was updated to log the additional information in
> > > the AUDIT_FANOTIFY record.
> > >
> > > Currently the only type of fanotify info that is defined is an audit
> > > rule number, but convert it to hex encoding to future-proof the field.
> > > Hex encoding suggested by Paul Moore .
> > >
> > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes,
> > > unknown.
> > >
> > > Sample records:
> > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> > > fan_info=3137 subj_trust=3 obj_trust=5
> > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0
> > > fan_info=3F subj_trust=2 obj_trust=2
> > >
> > > Suggested-by: Steve Grubb
> > > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > fs/notify/fanotify/fanotify.c | 3 ++-
> > > include/linux/audit.h | 9 +
> > > kernel/auditsc.c | 16 +---
> > > 3 files changed, 20 insertions(+), 8 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d1fb821de104..3133c4175c15 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > context->type = AUDIT_KERN_MODULE;
> > > }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, struct
> > > fanotify_response_info_audit_rule *friar)
> > > {
> > > - audit_log(audit_context(), GFP_KERNEL,
> > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> > > obj_trust=2",
> > > + response, FAN_RESPONSE_INFO_NONE);
> > > + return;
> > > + }
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u
> > > obj_trust=%u",
> > > + response, friar->hdr.type, friar->rule_number,
> > > + friar->subj_trust, friar->obj_trust);
> > > }
> >
> > The only thing that comes to mind might be to convert the if-return
> > into a switch statement to make it a bit cleaner and easier to patch
> > in the future, but that is s far removed from any real concern
> > that I debated even mentioning it. I only bring it up in case the
> > "3F" discussion results in a respin, and even then I'm not going to
> > hold my ACK over something as silly as a if-return vs switch.
> >
> > For clarity, this is what I was thinking:
> >
> > void __audit_fanontify(...)
> > {
> > switch (type) {
> > case FAN_RESPONSE_INFO_NONE:
> > audit_log(...);
> > break;
> > default:
> > audit_log(...);
> > }
> > }
>
> I agree that would be cleaner ...
As I said, the "3F" concern of Steve is really the only thing I would
bother respinning for, my other comments were just passing
observations.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs wrote:
> On 2023-01-20 13:52, Paul Moore wrote:
> > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb wrote:
> > > Hello Richard,
> > >
> > > I built a new kernel and tested this with old and new user space. It is
> > > working as advertised. The only thing I'm wondering about is why we have
> > > 3F
> > > as the default value when no additional info was sent? Would it be better
> > > to
> > > just make it 0?
> >
> > ...
> >
> > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index d1fb821de104..3133c4175c15 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > > context->type = AUDIT_KERN_MODULE;
> > > > }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, struct
> > > > fanotify_response_info_audit_rule *friar) {
> > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> > > obj_trust=2",
> > > > + response, FAN_RESPONSE_INFO_NONE);
> > > > + return;
> > > > + }
> >
> > (I'm working under the assumption that the "fan_info=3F" in the record
> > above is what Steve was referring to in his comment.)
> >
> > I vaguely recall Richard commenting on this in the past, although
> > maybe not ... my thought is that the "3F" is simply the hex encoded
> > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the
> > question is what to do in the FAN_RESPONSE_INFO_NONE case.
> >
> > Historically when we had a missing field we would follow the "field=?"
> > pattern, but I don't recall doing that for a field which was
> > potentially hex encoded, is there an existing case where we use "?"
> > for a field that is hex encoded? If so, we can swap out the "3F" for
> > a more obvious "?".
>
> I was presuming encoding the zero: "30"
I'm sorry, but you've lost me here.
> > However, another option might be to simply output the current
> > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g.
> > only "resp=%u". This is a little against the usual guidance of
> > "fields should not disappear from a record", but considering that
> > userspace will always need to support the original resp-only format
> > for compatibility reasons this may be an option.
>
> I don't have a strong opinion.
I'm not sure I care too much either. I will admit that the "3F" seems
to be bordering on the "bit too clever" side of things, but it's easy
to argue it is in keeping with the general idea of using "?" to denote
absent/unknown fields.
As Steve was the one who raised the question in this latest round, and
he knows his userspace tools the best, it seems wise to get his input
on this.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On 1/27/23 12:42 PM, Paul Moore wrote: > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
Getting XATTRs is not particularly interesting security-wise.
Suggested-by: Steve Grubb
Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
Signed-off-by: Richard Guy Briggs
---
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v1 0/2] two suggested iouring op audit updates
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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_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
---
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 02/31] Documentation: Add binding for kalray,kv3-1-core-intc
On 26/01/2023 17:10, Jules Maselbas wrote:
>>> + reg:
>>> +maxItems: 0
>>
>> ??? No way... What's this?
> This (per CPU) interrupt controller is not memory mapped at all, it is
> controlled and configured through system registers.
>
> I do not have found existing .yaml bindings for such devices, only the
> file snps,archs-intc.txt has something similar.
>
> I do not know what is the best way to represent such devices in the
> device-tree. Any suggestions are welcome.
You cannot have an array property with 0 items. How would it look like
in DTS? There are many, many bindings which are expressing it. Just drop
the reg.
>
>>
>>> + "kalray,intc-nr-irqs":
>>
>> Drop quotes.
>>
>>> +description: Number of irqs handled by the controller.
>>
>> Why this is variable per board? Why do you need it ?
> This property is not even used in our device-tree, this will be removed
> from the documentation and from the driver as well.
>
>>> +
>>> +required:
>>> + - compatible
>>> + - "#interrupt-cells"
>>> + - interrupt-controller
>>
>> missing additionalProperties: false
>>
>> This binding looks poor, like you started from something odd. Please
>> don't. Take the newest reviewed binding or better example-schema and use
>> it to build yours. This would solve several trivial mistakes and style
>> issues.
> I am starting over from the example-schema.
>
>>> +
>>> +examples:
>>> + - |
>>> +intc: interrupt-controller {
>>
>> What's the IO address space?
> As said above, this is not a memory mapped device, but is accessed
> through system registers.
Sure, but then you cannot define a reg which was confusing...
Best regards,
Krzysztof
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
