Re: What does "---" in audit.log timestamp / event-id field mean?
Hello, On Thursday, May 12, 2022 4:01:34 AM EDT Sam Pinkus wrote: > I'm using auditd=1:2.8.4-3 on Debian. I got this event in my audit.log: > > > ... > type=SYSCALL msg=audit(16523210---): arch=c03e syscall=87 success=yes > exit=0 a0=7f867d66a3ed a1=7f867d66a3ed a2=0 a3=792f18 items=2 ppid=2275 > pid=16746 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 fsuid=1000 > egid=1000 sgid=1000 fsgid=1000 tty=(none) ses=2 comm=4C5320546872656164 > exe="/usr/lib/firefox-esr/firefox-esr" subj==unconfined key="delete" > type=CWD msg=audit(1652321038.100:23444): cwd="/home/sam" > type=PATH msg=audit(1652321038.100:23444): item=0 > name="/home/sam/.mozilla/firefox/baey2He4.default/" inode=15861713 > dev=fe:01 mode=040700 ouid=1000 ogid=1000 rdev=00:00 nametype=PARENT > cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0 cap_frootid=0 type=PATH > msg=audit(1652321038.100:23444): item=1 > name="/home/sam/.mozilla/firefox/baey2He4.default/webappsstore.sqlite-wal" > inode=15860647 dev=fe:01 mode=0100644 ouid=1000 ogid=1000 rdev=00:00 > nametype=DELETE cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0 cap_frootid=0 > type=PROCTITLE msg=audit(1652321038.100:23444): > proctitle="/usr/lib/firefox-esr/firefox-esr" > > I.e. there is an incomplete timestamp and no event ID in the first line of > the event "16523210---". I have never seen such a problem. Looking at both the kernel and userspace code, I do not see what could prossibly do this. There is no code with exactly 3 dashes in the audit user space or kernel. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/1] audit,io_uring,io-wq: call __audit_uring_exit for dummy contexts
On Tue, May 17, 2022 at 9:12 AM Paul Moore wrote:
> On Tue, May 17, 2022 at 6:33 AM Julian Orth wrote:
> >
> > Not calling the function for dummy contexts will cause the context to
> > not be reset. During the next syscall, this will cause an error in
> > __audit_syscall_entry:
> >
> > WARN_ON(context->context != AUDIT_CTX_UNUSED);
> > WARN_ON(context->name_count);
> > if (context->context != AUDIT_CTX_UNUSED || context->name_count) {
> > audit_panic("unrecoverable error in audit_syscall_entry()");
> > return;
> > }
> >
> > These problematic dummy contexts are created via the following call
> > chain:
> >
> >exit_to_user_mode_prepare
> > -> arch_do_signal_or_restart
> > -> get_signal
> > -> task_work_run
> > -> tctx_task_work
> > -> io_req_task_submit
> > -> io_issue_sqe
> > -> audit_uring_entry
> >
> > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
> > io_uring")
> > Signed-off-by: Julian Orth
> > ---
> > include/linux/audit.h | 2 +-
> > kernel/auditsc.c | 6 ++
> > 2 files changed, 7 insertions(+), 1 deletion(-)
>
> Hi Julian,
>
> Thanks for the report and the patch too! I agree that it does seem a
> little odd that we haven't seen this before, let me dig into this a
> bit more today and respond back.
The patch looks good to me, thanks again. I just merged this into the
audit/stable-5.18 branch and added a stable tag; assuming the test
runs go okay I'll send this up to Linus tomorrow.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 1/1] audit,io_uring,io-wq: call __audit_uring_exit for dummy contexts
On Tue, May 17, 2022 at 6:33 AM Julian Orth wrote:
>
> Not calling the function for dummy contexts will cause the context to
> not be reset. During the next syscall, this will cause an error in
> __audit_syscall_entry:
>
> WARN_ON(context->context != AUDIT_CTX_UNUSED);
> WARN_ON(context->name_count);
> if (context->context != AUDIT_CTX_UNUSED || context->name_count) {
> audit_panic("unrecoverable error in audit_syscall_entry()");
> return;
> }
>
> These problematic dummy contexts are created via the following call
> chain:
>
>exit_to_user_mode_prepare
> -> arch_do_signal_or_restart
> -> get_signal
> -> task_work_run
> -> tctx_task_work
> -> io_req_task_submit
> -> io_issue_sqe
> -> audit_uring_entry
>
> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
> io_uring")
> Signed-off-by: Julian Orth
> ---
> include/linux/audit.h | 2 +-
> kernel/auditsc.c | 6 ++
> 2 files changed, 7 insertions(+), 1 deletion(-)
Hi Julian,
Thanks for the report and the patch too! I agree that it does seem a
little odd that we haven't seen this before, let me dig into this a
bit more today and respond back.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH 1/1] audit, io_uring, io-wq: call __audit_uring_exit for dummy contexts
Not calling the function for dummy contexts will cause the context to
not be reset. During the next syscall, this will cause an error in
__audit_syscall_entry:
WARN_ON(context->context != AUDIT_CTX_UNUSED);
WARN_ON(context->name_count);
if (context->context != AUDIT_CTX_UNUSED || context->name_count) {
audit_panic("unrecoverable error in audit_syscall_entry()");
return;
}
These problematic dummy contexts are created via the following call
chain:
exit_to_user_mode_prepare
-> arch_do_signal_or_restart
-> get_signal
-> task_work_run
-> tctx_task_work
-> io_req_task_submit
-> io_issue_sqe
-> audit_uring_entry
Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
io_uring")
Signed-off-by: Julian Orth
---
include/linux/audit.h | 2 +-
kernel/auditsc.c | 6 ++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index d06134ac6245..cece70231138 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -339,7 +339,7 @@ static inline void audit_uring_entry(u8 op)
}
static inline void audit_uring_exit(int success, long code)
{
- if (unlikely(!audit_dummy_context()))
+ if (unlikely(audit_context()))
__audit_uring_exit(success, code);
}
static inline void audit_syscall_entry(int major, unsigned long a0,
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ea2ee1181921..f3a2abd6d1a1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1959,6 +1959,12 @@ void __audit_uring_exit(int success, long code)
{
struct audit_context *ctx = audit_context();
+ if (ctx->dummy) {
+ if (ctx->context != AUDIT_CTX_URING)
+ return;
+ goto out;
+ }
+
if (ctx->context == AUDIT_CTX_SYSCALL) {
/*
* NOTE: See the note in __audit_uring_entry() about the case
--
2.36.1
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
fsnotify
config: m68k-defconfig
(https://download.01.org/0day-ci/archive/20220517/202205171541.x3kcgj83-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1
O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of
>> type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned
>> int'} [-Wformat=]
5 | #define KERN_SOH"\001" /* ASCII Start Of Header */
| ^~
include/linux/printk.h:418:25: note: in definition of macro
'printk_index_wrap'
418 | _p_func(_fmt, ##__VA_ARGS__);
\
| ^~~~
include/linux/printk.h:132:17: note: in expansion of macro 'printk'
132 | printk(fmt, ##__VA_ARGS__); \
| ^~
include/linux/printk.h:576:9: note: in expansion of macro 'no_printk'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^
include/linux/kern_levels.h:15:25: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7"/* debug-level messages */
| ^~~~
include/linux/printk.h:576:19: note: in expansion of macro 'KERN_DEBUG'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro
'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u
size=%lu\n", __func__,
| ^~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001"
/* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII'\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://01.org/lkp
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
On Tue, May 17, 2022 at 2:31 PM Amir Goldstein wrote:
>
> On Tue, May 17, 2022 at 1:32 PM Jan Kara wrote:
> >
> > On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs
> > > wrote:
> > > >
> > > > This patch adds 2 structure members to the response returned from user
> > > > space on a permission event. The first field is 32 bits for the context
> > > > type. The context type will describe what the meaning is of the second
> > > > field. The default is none. The patch defines one additional context
> > > > type which means that the second field is a union containing a 32-bit
> > > > rule number. This will allow for the creation of other context types in
> > > > the future if other users of the API identify different needs. The
> > > > second field size is defined by the context type and can be used to pass
> > > > along the data described by the context.
> > > >
> > > > To support this, there is a macro for user space to check that the data
> > > > being sent is valid. Of course, without this check, anything that
> > > > overflows the bit field will trigger an EINVAL based on the use of
> > > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > > >
> > > > Suggested-by: Steve Grubb
> > > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > > Suggested-by: Jan Kara
> > > > Link: https://lore.kernel.org/r/20201001101219.ge17...@quack2.suse.cz
> > > > Signed-off-by: Richard Guy Briggs
> >
> > ...
> > > > static int process_access_response(struct fsnotify_group *group,
> > > > - struct fanotify_response
> > > > *response_struct)
> > > > + struct fanotify_response
> > > > *response_struct,
> > > > + size_t count)
> > > > {
> > > > struct fanotify_perm_event *event;
> > > > int fd = response_struct->fd;
> > > > u32 response = response_struct->response;
> > > >
> > > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > > -fd, response);
> > > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n",
> > > > __func__,
> > > > +group, fd, response, response_struct->extra_info_type,
> > > > count);
> > > > + if (fd < 0)
> > > > + return -EINVAL;
> > > > /*
> > > > * make sure the response is valid, if invalid we do nothing
> > > > and either
> > > > * userspace can send a valid response or we will clean it up
> > > > after the
> > > > * timeout
> > > > */
> > > > - switch (response & ~FAN_AUDIT) {
> > > > - case FAN_ALLOW:
> > > > - case FAN_DENY:
> > > > - break;
> > > > - default:
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - if (fd < 0)
> > > > + if (FAN_INVALID_RESPONSE_MASK(response))
> > >
> > > That is a logic change, because now the response value of 0 becomes valid.
> > >
> > > Since you did not document this change in the commit message I assume
> > > this was
> > > non intentional?
> > > However, this behavior change is something that I did ask for, but it
> > > should be
> > > done is a separate commit:
> > >
> > > /* These are NOT bitwise flags. Both bits can be used together. */
> > > #define FAN_TEST 0x00
> > > #define FAN_ALLOW 0x01
> > > #define FAN_DENY0x02
> > > #define FANOTIFY_RESPONSE_ACCESS \
> > > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> > >
> > > ...
> > > int access = response & FANOTIFY_RESPONSE_ACCESS;
> > >
> > > 1. Do return EINVAL for access == 0
> > > 2. Let all the rest of the EINVAL checks run (including extra type)
> > > 3. Move if (fd < 0) to last check
> > > 4. Add if (!access) return 0 before if (fd < 0)
> > >
> > > That will provide a mechanism for userspace to probe the
> > > kernel support for extra types in general and specific types
> > > that it may respond with.
> >
> > I have to admit I didn't quite grok your suggestion here although I
> > understand (and agree with) the general direction of the proposal :). Maybe
> > code would explain it better what you have in mind?
> >
>
> +/* These are NOT bitwise flags. Both bits can be used together. */
I realize when reading this that this comment is weird, because
0x01 and 0x02 cannot currently be used together.
The comment was copied from above FAN_MARK_INODE where it
has the same weirdness.
The meaning is that (response & FANOTIFY_RESPONSE_ACCESS)
is an enum. I am sure that a less confusing phrasing for this comment
can be found.
> +#define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY0x02
> #define FAN_AUDIT 0x10/* Bit mask to create audit record for result
> */
> +#define FANOTIFY_RESPONSE_ACCESS \
> +(FAN_TEST|FAN_ALLOW | FAN_DENY)
Thanks,
Amir.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listm
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs wrote:
>
> This patch adds 2 structure members to the response returned from user
> space on a permission event. The first field is 32 bits for the context
> type. The context type will describe what the meaning is of the second
> field. The default is none. The patch defines one additional context
> type which means that the second field is a union containing a 32-bit
> rule number. This will allow for the creation of other context types in
> the future if other users of the API identify different needs. The
> second field size is defined by the context type and can be used to pass
> along the data described by the context.
>
> To support this, there is a macro for user space to check that the data
> being sent is valid. Of course, without this check, anything that
> overflows the bit field will trigger an EINVAL based on the use of
> FAN_INVALID_RESPONSE_MASK in process_access_response().
>
> Suggested-by: Steve Grubb
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara
> Link: https://lore.kernel.org/r/20201001101219.ge17...@quack2.suse.cz
> Signed-off-by: Richard Guy Briggs
> ---
> fs/notify/fanotify/fanotify.c | 2 +-
> fs/notify/fanotify/fanotify.h | 2 +
> fs/notify/fanotify/fanotify_user.c | 74 +++---
> include/linux/fanotify.h | 3 ++
> include/uapi/linux/fanotify.h | 22 -
> 5 files changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..ea0e60488f12 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group
> *group,
> }
>
> /* userspace responded, convert to something usable */
> - switch (event->response & ~FAN_AUDIT) {
> + switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
> case FAN_ALLOW:
> ret = 0;
> break;
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index d8e06bee..eb7ec1f2a26e 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> struct fanotify_event fae;
> struct path path;
> u32 response; /* userspace answer to the event */
> + u32 extra_info_type;
> unsigned short state; /* state of the event */
> int fd; /* fd we passed to userspace for this event */
> + union fanotify_response_extra extra_info;
> };
>
> static inline struct fanotify_perm_event *
> diff --git a/fs/notify/fanotify/fanotify_user.c
> b/fs/notify/fanotify/fanotify_user.c
> index 721e777ea90b..1c4067e29f2e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,22 @@ static int create_fd(struct fsnotify_group *group,
> struct path *path,
> */
> static void finish_permission_event(struct fsnotify_group *group,
> struct fanotify_perm_event *event,
> - u32 response)
> + struct fanotify_response *response)
> __releases(&group->notification_lock)
> {
> bool destroy = false;
>
> assert_spin_locked(&group->notification_lock);
> - event->response = response;
> + event->response = response->response & ~FAN_EXTRA;
> + if (response->response & FAN_EXTRA) {
> + event->extra_info_type = response->extra_info_type;
> + switch (event->extra_info_type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + event->extra_info.audit_rule =
> response->extra_info.audit_rule;
> + }
> + } else {
> + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> + }
> if (event->state == FAN_EVENT_CANCELED)
> destroy = true;
> else
> @@ -306,33 +315,40 @@ static void finish_permission_event(struct
> fsnotify_group *group,
> }
>
> static int process_access_response(struct fsnotify_group *group,
> - struct fanotify_response *response_struct)
> + struct fanotify_response *response_struct,
> + size_t count)
> {
> struct fanotify_perm_event *event;
> int fd = response_struct->fd;
> u32 response = response_struct->response;
>
> - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> -fd, response);
> + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n",
> __func__,
> +group, fd, response, response_struct->extra_info_type,
> count);
> + if (fd < 0)
> + return -EINVAL;
> /*
>
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linux/master linus/master
v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
fsnotify
config: m68k-allmodconfig
(https://download.01.org/0day-ci/archive/20220517/202205171508.anzwewlm-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1
O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> fs/notify/fanotify/fanotify_user.c:325:18: warning: format '%lu' expects
>> argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka
>> 'unsigned int'} [-Wformat=]
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u
size=%lu\n", __func__,
| ^~~
include/linux/printk.h:336:21: note: in definition of macro 'pr_fmt'
336 | #define pr_fmt(fmt) fmt
| ^~~
include/linux/dynamic_debug.h:152:9: note: in expansion of macro
'__dynamic_func_call'
152 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func,
##__VA_ARGS__)
| ^~~
include/linux/dynamic_debug.h:162:9: note: in expansion of macro
'_dynamic_func_call'
162 | _dynamic_func_call(fmt, __dynamic_pr_debug, \
| ^~
include/linux/printk.h:570:9: note: in expansion of macro 'dynamic_pr_debug'
570 | dynamic_pr_debug(fmt, ##__VA_ARGS__)
| ^~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro
'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u
size=%lu\n", __func__,
| ^~~~
fs/notify/fanotify/fanotify_user.c:325:65: note: format string is defined
here
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u
size=%lu\n", __func__,
| ~~^
| |
| long
unsigned int
| %u
vim +325 fs/notify/fanotify/fanotify_user.c
316
317 static int process_access_response(struct fsnotify_group *group,
318 struct fanotify_response
*response_struct,
319 size_t count)
320 {
321 struct fanotify_perm_event *event;
322 int fd = response_struct->fd;
323
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
On Tue, May 17, 2022 at 1:32 PM Jan Kara wrote:
>
> On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs wrote:
> > >
> > > This patch adds 2 structure members to the response returned from user
> > > space on a permission event. The first field is 32 bits for the context
> > > type. The context type will describe what the meaning is of the second
> > > field. The default is none. The patch defines one additional context
> > > type which means that the second field is a union containing a 32-bit
> > > rule number. This will allow for the creation of other context types in
> > > the future if other users of the API identify different needs. The
> > > second field size is defined by the context type and can be used to pass
> > > along the data described by the context.
> > >
> > > To support this, there is a macro for user space to check that the data
> > > being sent is valid. Of course, without this check, anything that
> > > overflows the bit field will trigger an EINVAL based on the use of
> > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > >
> > > Suggested-by: Steve Grubb
> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > Suggested-by: Jan Kara
> > > Link: https://lore.kernel.org/r/20201001101219.ge17...@quack2.suse.cz
> > > Signed-off-by: Richard Guy Briggs
>
> ...
> > > static int process_access_response(struct fsnotify_group *group,
> > > - struct fanotify_response
> > > *response_struct)
> > > + struct fanotify_response
> > > *response_struct,
> > > + size_t count)
> > > {
> > > struct fanotify_perm_event *event;
> > > int fd = response_struct->fd;
> > > u32 response = response_struct->response;
> > >
> > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > -fd, response);
> > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n",
> > > __func__,
> > > +group, fd, response, response_struct->extra_info_type,
> > > count);
> > > + if (fd < 0)
> > > + return -EINVAL;
> > > /*
> > > * make sure the response is valid, if invalid we do nothing and
> > > either
> > > * userspace can send a valid response or we will clean it up
> > > after the
> > > * timeout
> > > */
> > > - switch (response & ~FAN_AUDIT) {
> > > - case FAN_ALLOW:
> > > - case FAN_DENY:
> > > - break;
> > > - default:
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (fd < 0)
> > > + if (FAN_INVALID_RESPONSE_MASK(response))
> >
> > That is a logic change, because now the response value of 0 becomes valid.
> >
> > Since you did not document this change in the commit message I assume this
> > was
> > non intentional?
> > However, this behavior change is something that I did ask for, but it
> > should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I have to admit I didn't quite grok your suggestion here although I
> understand (and agree with) the general direction of the proposal :). Maybe
> code would explain it better what you have in mind?
>
+/* These are NOT bitwise flags. Both bits can be used together. */
+#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY0x02
#define FAN_AUDIT 0x10/* Bit mask to create audit record for result */
+#define FANOTIFY_RESPONSE_ACCESS \
+(FAN_TEST|FAN_ALLOW | FAN_DENY)
...
@@ -311,6 +314,7 @@ static int process_access_response(struct
fsnotify_group *group,
struct fanotify_perm_event *event;
int fd = response_struct->fd;
int response = response_struct->response;
+ int access = response & FANOTIFY_RESPONSE_ACCESS;
pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
fd, response);
@@ -319,18 +323,33 @@ static int process_access_response(struct
fsnotify_group *group,
* userspace can send a valid response or we will clean it up after the
* timeout
*/
- switch (response & ~FAN_AUDIT) {
+ if (!response)
+ return -EINVAL;
+
+ switch
Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs wrote:
> >
> > This patch adds 2 structure members to the response returned from user
> > space on a permission event. The first field is 32 bits for the context
> > type. The context type will describe what the meaning is of the second
> > field. The default is none. The patch defines one additional context
> > type which means that the second field is a union containing a 32-bit
> > rule number. This will allow for the creation of other context types in
> > the future if other users of the API identify different needs. The
> > second field size is defined by the context type and can be used to pass
> > along the data described by the context.
> >
> > To support this, there is a macro for user space to check that the data
> > being sent is valid. Of course, without this check, anything that
> > overflows the bit field will trigger an EINVAL based on the use of
> > FAN_INVALID_RESPONSE_MASK in process_access_response().
> >
> > Suggested-by: Steve Grubb
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara
> > Link: https://lore.kernel.org/r/20201001101219.ge17...@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs
...
> > static int process_access_response(struct fsnotify_group *group,
> > - struct fanotify_response
> > *response_struct)
> > + struct fanotify_response
> > *response_struct,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > -fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n",
> > __func__,
> > +group, fd, response, response_struct->extra_info_type,
> > count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * make sure the response is valid, if invalid we do nothing and
> > either
> > * userspace can send a valid response or we will clean it up after
> > the
> > * timeout
> > */
> > - switch (response & ~FAN_AUDIT) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
> However, this behavior change is something that I did ask for, but it should
> be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I have to admit I didn't quite grok your suggestion here although I
understand (and agree with) the general direction of the proposal :). Maybe
code would explain it better what you have in mind?
> > +/*
> > + * User space may need to record additional information about its decision.
> > + * The extra information type records what kind of information is included.
> > + * The default is none. We also define an extra informaion buffer whose
>
> typo: informaion
>
> > + * size is determined by the extra information type.
> > + *
> > + * If the context type is Rule, then the context following is the rule
> > number
> > + * that triggered the user space decision.
> > + */
> > +
> > +#define FAN_RESPONSE_INFO_NONE 0
> > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and
> info_len.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
Yes, please no unions in the API, that is always painful with the
alignment, size etc. What I had in mind was:
K
[PATCH 0/1] audit, io_uring, io-wq: call __audit_uring_exit for dummy contexts
After porting my wayland compositor to io_uring, I noticed that my logs were getting spammed with the following messages (tested with 5.17.7 and 5.18.0-rc7): WARNING: CPU: 10 PID: 983 at kernel/auditsc.c:2041 __audit_syscall_entry+0x1> Modules linked in: vrf wireguard curve25519_x86_64 libchacha20poly1305 chach> ipmi_msghandler crypto_user ip_tables x_tables ext4 crc32c_generic crc16 mb> CPU: 10 PID: 983 Comm: jay Tainted: GW 5.18.0-rc7-dirty #5 1> Hardware name: Gigabyte Technology Co., Ltd. B450M DS3H/B450M DS3H-CF, BIOS > RIP: 0010:__audit_syscall_entry+0x111/0x140 Code: e8 24 69 ff ff 48 8b 34 24 48 8b 54 24 08 85 c0 48 8b 4c 24 10 4c 8b 4> RSP: 0018:a6e480887de8 EFLAGS: 00010202 RAX: RBX: 969c92f22400 RCX: 0001 RDX: 0001 RSI: 0007 RDI: 969c8fa4c080 RBP: 01aa R08: 0001 R09: R10: 0002 R11: 0001 R12: 01aa R13: a6e480887f58 R14: 01aa R15: FS: 7fefe020f040() GS:96a39ea8() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 7fefac429000 CR3: 000103674000 CR4: 003506e0 Call Trace: ? get_signal+0x8d/0x990 syscall_trace_enter.constprop.0+0x121/0x1a0 do_syscall_64+0x36/0x80 ? arch_do_signal_or_restart+0x44/0x750 ? syscall_exit_to_user_mode+0x22/0x40 ? exit_to_user_mode_prepare+0xd3/0x140 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7fefe0d0b67d Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 4> RSP: 002b:7fff383a6c58 EFLAGS: 0246 ORIG_RAX: 01aa RAX: ffda RBX: 5595f1c25e00 RCX: 7fefe0d0b67d RDX: 0001 RSI: 0001 RDI: 0007 RBP: 5595f1c1b700 R08: R09: R10: 0001 R11: 0246 R12: 0001 R13: 0005 R14: 5595f1c21f60 R15: 0001 ---[ end trace ]--- audit: unrecoverable error in audit_syscall_entry() I traced this to the context not being reset after audit_uring_entry if the created context was a dummy context. I am surprised that I would be the first one to find this problem but maybe io_uring is rarely used on systems where auditing is enabled or a recent kernel change elsewhere caused this problem to surface. Julian Orth (1): audit,io_uring,io-wq: call __audit_uring_exit for dummy contexts include/linux/audit.h | 2 +- kernel/auditsc.c | 6 ++ 2 files changed, 7 insertions(+), 1 deletion(-) -- 2.36.1 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
