Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb wrote:
> On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > On 2022-08-31 17:25, Steve Grubb wrote:
> > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index 433418d73584..f000fec52360 100644
> > > > > > --- a/kernel/auditsc.c
> > > > > > +++ b/kernel/auditsc.c
> > > > > > @@ -64,6 +64,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include // struct open_how
> > > > > > +#include
> > > > > >
> > > > > > #include "audit.h"
> > > > > >
> > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > }
> > > > > >
> > > > > > -void __audit_fanotify(u32 response)
> > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > {
> > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > + size_t c = len;
> > > > > > + char *ib = buf;
> > > > > > +
> > > > > > + if (!(len && buf)) {
> > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > AUDIT_FANOTIFY,
> > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > response);
> > > > > > + return;
> > > > > > + }
> > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > *)buf;
> > > > >
> > > > > Since the only use of this at the moment is the
> > > > > fanotify_response_info_rule, why not pass the
> > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > can always change it if we need to in the future without affecting
> > > > > userspace, and it would simplify the code.
> > > >
> > > > Steve, would it make any sense for there to be more than one
> > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > than one rule that contributes to a notify reason? If not, would it be
> > > > reasonable to return -EINVAL if there is more than one?
> > >
> > > I don't see a reason for sending more than one header. What is more
> > > probable is the need to send additional data in that header. I was
> > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > padding the struct just in case it needs expanding some day.
> >
> > This doesn't exactly answer my question about multiple rules
> > contributing to one decision.
>
> I don't forsee that.
>
> > The need for more as yet undefined information sounds like a good reason
> > to define a new header if that happens.
>
> It's much better to pad the struct so that the size doesn't change.
>
> > At this point, is it reasonable to throw an error if more than one RULE
> > header appears in a message?
>
> It is a write syscall. I'd silently discard everything else and document that
> in the man pages. But the fanotify maintainers should really weigh in on
> this.
>
> > The way I had coded this last patchset was to allow for more than one RULE
> > header and each one would get its own record in the event.
>
> I do not forsee a need for this.
>
> > How many rules total are likely to exist?
>
> Could be a thousand. But I already know some missing information we'd like to
> return to user space in an audit event, so the bit mapping on the rule number
> might happen. I'd suggest padding one u32 for future use.
A better way to handle an expansion like that would be to have a
length/version field at the top of the struct that could be used to
determine the size and layout of the struct.
However, to be clear, my original suggestion of passing the
fanotify_response_info_rule struct internally didn't require any
additional future proofing as it is an internal implementation detail
and not something that is exposed to userspace; the function arguments
could be changed in the future and not break userspace. I'm not quite
sure how we ended up on this topic ...
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> On 2022-08-31 17:25, Steve Grubb wrote:
> > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 433418d73584..f000fec52360 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -64,6 +64,7 @@
> > > > > #include
> > > > > #include
> > > > > #include // struct open_how
> > > > > +#include
> > > > >
> > > > > #include "audit.h"
> > > > >
> > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > context->type = AUDIT_KERN_MODULE;
> > > > > }
> > > > >
> > > > > -void __audit_fanotify(u32 response)
> > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > {
> > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > + size_t c = len;
> > > > > + char *ib = buf;
> > > > > +
> > > > > + if (!(len && buf)) {
> > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > AUDIT_FANOTIFY,
> > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > response);
> > > > > + return;
> > > > > + }
> > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > *)buf;
> > > >
> > > > Since the only use of this at the moment is the
> > > > fanotify_response_info_rule, why not pass the
> > > > fanotify_response_info_rule struct directly into this function? We
> > > > can always change it if we need to in the future without affecting
> > > > userspace, and it would simplify the code.
> > >
> > > Steve, would it make any sense for there to be more than one
> > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > than one rule that contributes to a notify reason? If not, would it be
> > > reasonable to return -EINVAL if there is more than one?
> >
> > I don't see a reason for sending more than one header. What is more
> > probable is the need to send additional data in that header. I was
> > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > padding the struct just in case it needs expanding some day.
>
> This doesn't exactly answer my question about multiple rules
> contributing to one decision.
I don't forsee that.
> The need for more as yet undefined information sounds like a good reason
> to define a new header if that happens.
It's much better to pad the struct so that the size doesn't change.
> At this point, is it reasonable to throw an error if more than one RULE
> header appears in a message?
It is a write syscall. I'd silently discard everything else and document that
in the man pages. But the fanotify maintainers should really weigh in on
this.
> The way I had coded this last patchset was to allow for more than one RULE
> header and each one would get its own record in the event.
I do not forsee a need for this.
> How many rules total are likely to exist?
Could be a thousand. But I already know some missing information we'd like to
return to user space in an audit event, so the bit mapping on the rule number
might happen. I'd suggest padding one u32 for future use.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-08-31 17:25, Steve Grubb wrote:
> On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 433418d73584..f000fec52360 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -64,6 +64,7 @@
> > > > #include
> > > > #include
> > > > #include // struct open_how
> > > > +#include
> > > >
> > > > #include "audit.h"
> > > >
> > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > context->type = AUDIT_KERN_MODULE;
> > > > }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > {
> > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > + struct fanotify_response_info_audit_rule *friar;
> > > > + size_t c = len;
> > > > + char *ib = buf;
> > > > +
> > > > + if (!(len && buf)) {
> > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > + "resp=%u fan_type=0 fan_info=?", response);
> > > > + return;
> > > > + }
> > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > + friar = (struct fanotify_response_info_audit_rule
> > > > *)buf;
> > >
> > > Since the only use of this at the moment is the
> > > fanotify_response_info_rule, why not pass the
> > > fanotify_response_info_rule struct directly into this function? We
> > > can always change it if we need to in the future without affecting
> > > userspace, and it would simplify the code.
> >
> > Steve, would it make any sense for there to be more than one
> > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > than one rule that contributes to a notify reason? If not, would it be
> > reasonable to return -EINVAL if there is more than one?
>
> I don't see a reason for sending more than one header. What is more probable
> is the need to send additional data in that header. I was thinking of maybe
> bit mapping it in the rule number. But I'd suggest padding the struct just in
> case it needs expanding some day.
This doesn't exactly answer my question about multiple rules
contributing to one decision.
The need for more as yet undefined information sounds like a good reason
to define a new header if that happens.
At this point, is it reasonable to throw an error if more than one RULE
header appears in a message? The way I had coded this last patchset was
to allow for more than one RULE header and each one would get its own
record in the event.
How many rules total are likely to exist?
> -Steev
- 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 v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-08-15 20:22, Paul Moore wrote:
> On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs wrote:
> >
> > This patch passes the full value 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. The following is an example of the new record
> > format:
> >
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=17
> >
> > 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 | 31 ---
> > 3 files changed, 35 insertions(+), 8 deletions(-)
>
> You've hopefully already seen the kernel test robot build warning, so
> I won't bring that up again, but a few comments below ...
Yes, dealt with...
...
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 433418d73584..f000fec52360 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -64,6 +64,7 @@
> > #include
> > #include
> > #include // struct open_how
> > +#include
> >
> > #include "audit.h"
> >
> > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > context->type = AUDIT_KERN_MODULE;
> > }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > {
> > - audit_log(audit_context(), GFP_KERNEL,
> > - AUDIT_FANOTIFY, "resp=%u", response);
> > + struct fanotify_response_info_audit_rule *friar;
> > + size_t c = len;
> > + char *ib = buf;
> > +
> > + if (!(len && buf)) {
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=0 fan_info=?", response);
> > + return;
> > + }
> > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > + friar = (struct fanotify_response_info_audit_rule *)buf;
>
> Since the only use of this at the moment is the
> fanotify_response_info_rule, why not pass the
> fanotify_response_info_rule struct directly into this function? We
> can always change it if we need to in the future without affecting
> userspace, and it would simplify the code.
Steve, would it make any sense for there to be more than one
FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
than one rule that contributes to a notify reason? If not, would it be
reasonable to return -EINVAL if there is more than one?
> > + switch (friar->hdr.type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + if (friar->hdr.len < sizeof(*friar)) {
> > + audit_log(audit_context(), GFP_KERNEL,
> > AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u
> > fan_info=(incomplete)",
> > + response, friar->hdr.type);
> > + return;
> > + }
> > + audit_log(audit_context(), GFP_KERNEL,
> > AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_info=%u",
> > + response, friar->hdr.type,
> > friar->audit_rule);
> > + }
> > + c -= friar->hdr.len;
> > + ib += friar->hdr.len;
> > + }
> > }
> >
> > void __audit_tk_injoffset(struct timespec64 offset)
>
> 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 v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 433418d73584..f000fec52360 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -64,6 +64,7 @@
> > > #include
> > > #include
> > > #include // struct open_how
> > > +#include
> > >
> > > #include "audit.h"
> > >
> > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > context->type = AUDIT_KERN_MODULE;
> > > }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > {
> > > - audit_log(audit_context(), GFP_KERNEL,
> > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > + struct fanotify_response_info_audit_rule *friar;
> > > + size_t c = len;
> > > + char *ib = buf;
> > > +
> > > + if (!(len && buf)) {
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=0 fan_info=?", response);
> > > + return;
> > > + }
> > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > + friar = (struct fanotify_response_info_audit_rule
> > > *)buf;
> >
> > Since the only use of this at the moment is the
> > fanotify_response_info_rule, why not pass the
> > fanotify_response_info_rule struct directly into this function? We
> > can always change it if we need to in the future without affecting
> > userspace, and it would simplify the code.
>
> Steve, would it make any sense for there to be more than one
> FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> than one rule that contributes to a notify reason? If not, would it be
> reasonable to return -EINVAL if there is more than one?
I don't see a reason for sending more than one header. What is more probable
is the need to send additional data in that header. I was thinking of maybe
bit mapping it in the rule number. But I'd suggest padding the struct just in
case it needs expanding some day.
-Steev
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/2] fs/xattr: wire up syscalls
On 2022-08-30 17:28, Christian Göttsche wrote: > Enable the new added extended attribute related syscalls. > > Signed-off-by: Christian Göttsche I can't speak to the completeness of the arch list, but I'm glad to see the audit attr change bits in there. > --- > TODO: > - deprecate traditional syscalls (setxattr, ...)? > - resolve possible conflicts with proposed readfile syscall > --- > arch/alpha/kernel/syscalls/syscall.tbl | 4 > arch/arm/tools/syscall.tbl | 4 > arch/arm64/include/asm/unistd.h | 2 +- > arch/arm64/include/asm/unistd32.h | 8 > arch/ia64/kernel/syscalls/syscall.tbl | 4 > arch/m68k/kernel/syscalls/syscall.tbl | 4 > arch/microblaze/kernel/syscalls/syscall.tbl | 4 > arch/mips/kernel/syscalls/syscall_n32.tbl | 4 > arch/mips/kernel/syscalls/syscall_n64.tbl | 4 > arch/mips/kernel/syscalls/syscall_o32.tbl | 4 > arch/parisc/kernel/syscalls/syscall.tbl | 4 > arch/powerpc/kernel/syscalls/syscall.tbl| 4 > arch/s390/kernel/syscalls/syscall.tbl | 4 > arch/sh/kernel/syscalls/syscall.tbl | 4 > arch/sparc/kernel/syscalls/syscall.tbl | 4 > arch/x86/entry/syscalls/syscall_32.tbl | 4 > arch/x86/entry/syscalls/syscall_64.tbl | 4 > arch/xtensa/kernel/syscalls/syscall.tbl | 4 > include/asm-generic/audit_change_attr.h | 6 ++ > include/linux/syscalls.h| 8 > include/uapi/asm-generic/unistd.h | 12 +++- > 21 files changed, 98 insertions(+), 2 deletions(-) > > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl > b/arch/alpha/kernel/syscalls/syscall.tbl > index 3515bc4f16a4..826a8a36da81 100644 > --- a/arch/alpha/kernel/syscalls/syscall.tbl > +++ b/arch/alpha/kernel/syscalls/syscall.tbl > @@ -490,3 +490,7 @@ > 558 common process_mreleasesys_process_mrelease > 559 common futex_waitv sys_futex_waitv > 560 common set_mempolicy_home_node sys_ni_syscall > +561 common setxattrat sys_setxattrat > +562 common getxattrat sys_getxattrat > +563 common listxattrat sys_listxattrat > +564 common removexattrat sys_removexattrat > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl > index ac964612d8b0..f0e9d9d487f0 100644 > --- a/arch/arm/tools/syscall.tbl > +++ b/arch/arm/tools/syscall.tbl > @@ -464,3 +464,7 @@ > 448 common process_mreleasesys_process_mrelease > 449 common futex_waitv sys_futex_waitv > 450 common set_mempolicy_home_node sys_set_mempolicy_home_node > +451 common setxattrat sys_setxattrat > +452 common getxattrat sys_getxattrat > +453 common listxattrat sys_listxattrat > +454 common removexattrat sys_removexattrat > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h > index 037feba03a51..63a8a9c4abc1 100644 > --- a/arch/arm64/include/asm/unistd.h > +++ b/arch/arm64/include/asm/unistd.h > @@ -39,7 +39,7 @@ > #define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5) > #define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800) > > -#define __NR_compat_syscalls 451 > +#define __NR_compat_syscalls 455 > #endif > > #define __ARCH_WANT_SYS_CLONE > diff --git a/arch/arm64/include/asm/unistd32.h > b/arch/arm64/include/asm/unistd32.h > index 604a2053d006..cd6ac63376d1 100644 > --- a/arch/arm64/include/asm/unistd32.h > +++ b/arch/arm64/include/asm/unistd32.h > @@ -907,6 +907,14 @@ __SYSCALL(__NR_process_mrelease, sys_process_mrelease) > __SYSCALL(__NR_futex_waitv, sys_futex_waitv) > #define __NR_set_mempolicy_home_node 450 > __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) > +#define __NR_setxattrat 451 > +__SYSCALL(__NR_setxattrat, sys_setxattrat) > +#define __NR_getxattrat 452 > +__SYSCALL(__NR_getxattrat, sys_getxattrat) > +#define __NR_listxattrat 453 > +__SYSCALL(__NR_listxattrat, sys_listxattrat) > +#define __NR_removexattrat 454 > +__SYSCALL(__NR_removexattrat, sys_removexattrat) > > /* > * Please add new compat syscalls above this comment and update > diff --git a/arch/ia64/kernel/syscalls/syscall.tbl > b/arch/ia64/kernel/syscalls/syscall.tbl > index 78b1d03e86e1..6e942a935a27 100644 > --- a/arch/ia64/kernel/syscalls/syscall.tbl > +++ b/arch/ia64/kernel/syscalls/syscall.tbl > @@ -371,3 +371,7 @@ > 448 common process_mreleasesys_process_mrelease > 449 common futex_waitv sys_futex_waitv > 450 common set_mempolicy_home_node sys_set_mempolicy_home_node > +451 common setxattrat sys_setxattrat > +452 common getxattrat sys_getxattrat > +453
Re: [PATCH] audit: remove obvious unnecessary header files
Paul Moore 于2022年8月31日周三 01:04写道： > > > Hi Wuchi, can you explain what process you used to determine that > these header file includes were unnecessary? When reading the code, if I don't found the user of the *.h in the *.c file,I will think that is unnecessary. For example, #include in the audit.c, I don't found the use of kthread* in the file. But, I just build that without "W=1 " , the after test robot show that I was wrong. and I don't sure that if it is true to remove some header files. thanks wuchi > > -- > paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: remove obvious unnecessary header files
Paul Moore 于2022年8月31日周三 08:49写道： > > On Tue, Aug 30, 2022 at 8:20 PM chi wu wrote: > > Paul Moore 于2022年8月31日周三 01:04写道： > > > > > > > > > Hi Wuchi, can you explain what process you used to determine that > > > these header file includes were unnecessary? > > > > When reading the code, if I don't found the user of the *.h in the *.c > > file,I will think that is unnecessary. For example, #include > > in the audit.c, I don't found the use of kthread* in > > the file. > > But, I just build that without "W=1 " , the after test robot show that I > > was wrong. and I don't sure that if it is true to remove some header > > files. > > Yes, I would recommend that you focus your time and energy on other > tasks within the Linux Kernel. I'm very happy to see patches which > improve the audit subsystem, but I don't believe verifying the header > file usage is a good use of time at this point. > thanks very much > -- > paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
