Linux-Kernel Archive: Re: [PATCH 1/2] audit: add call argument to socketcall auditing Re: [PATCH 1/2] audit: add call argument to socketcall auditing From: Paul Moore Date: Tue May 03 2022 - 12:08:05 EST Next message: kernel test robot: "[asahilinux:bits/130-cpufreq 3/4] drivers/cpufreq/apple-soc-cpufreq.c:67:19: error: implicit declaration of function 'readq_relaxed'; did you mean 'readl_relaxed'?" Previous message: Paolo Bonzini: "Re: [PATCH] KVM: LAPIC: Fix advance timer expiration delta trace" In reply to: kernel test robot: "Re: [PATCH 1/2] audit: add call argument to socketcall auditing" Next in thread: Steve Grubb: "Re: [PATCH 1/2] audit: add call argument to socketcall auditing" Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] On Tue, May 3, 2022 at 5:02 AM Sven Schnelle wrote: > > socketcall auditing misses the call argument: > > type=SOCKETCALL msg=audit: nargs=3 a0=10 a1=3 a2=c > > which renders socketcall auditing (almost) useless. Add the call > argument so it is possible to decode the actual syscall from the > audit log: > > type=SOCKETCALL msg=audit: call=1 nargs=3 a0=10 a1=3 a2=c > > Signed-off-by: Sven Schnelle > --- > include/linux/audit.h | 10 +++++----- > kernel/audit.h | 1 + > kernel/auditsc.c | 6 ++++-- > net/compat.c | 2 +- > net/socket.c | 2 +- > 5 files changed, 12 insertions(+), 9 deletions(-) Hi Sven, Thanks for catching this, my only guess is that the original code assumed that a0 held the socket call number. In addition to the kernel test robot errors that need fixing, I've made some comments inline with the patch below ... > diff --git a/kernel/audit.h b/kernel/audit.h > index 58b66543b4d5..34e53b6f0ebb 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -153,6 +153,7 @@ struct audit_context { > int type; > union { > struct { > + int call; > int nargs; > long args[6]; Not your code, but while you are making changes, perhaps make @args[6] and unsigned long to match the network stack's code. > } socketcall; > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ea2ee1181921..c856893041c9 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -1399,8 +1399,9 @@ static void show_special(struct audit_context *context, int *call_panic) > switch (context->type) { > case AUDIT_SOCKETCALL: { > int nargs = context->socketcall.nargs; > + int call = context->socketcall.call; > > - audit_log_format(ab, "nargs=%d", nargs); > + audit_log_format(ab, "call=%d nargs=%d", call, nargs); > for (i = 0; i < nargs; i++) > audit_log_format(ab, " a%d=%lx", i, > context->socketcall.args[i]); The approach we take when adding new fields to existing audit records is to add them to the end of the record. Using your example in the patch description, we would want to see the following record format for SOCKETCALL: type=SOCKETCALL msg=audit: nargs=3 a0=10 a1=3 a2=c call=1 > @@ -2684,13 +2685,14 @@ void __audit_bprm(struct linux_binprm *bprm) > * @args: args array > * > */ > -int __audit_socketcall(int nargs, unsigned long *args) > +int __audit_socketcall(int call, int nargs, unsigned long *args) > { > struct audit_context *context = audit_context(); > > if (nargs <= 0 || nargs > AUDITSC_ARGS || !args) > return -EINVAL; > context->type = AUDIT_SOCKETCALL; > + context->socketcall.call = call; > context->socketcall.nargs = nargs; > memcpy(context->socketcall.args, args, nargs * sizeof(unsigned long)); > return 0; > diff --git a/net/compat.c b/net/compat.c > index 210fc3b4d0d8..0df955019ecc 100644 > --- a/net/compat.c > +++ b/net/compat.c > @@ -437,7 +437,7 @@ COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args) > if (copy_from_user(a, args, len)) > return -EFAULT; > > - ret = audit_socketcall_compat(len / sizeof(a[0]), a); > + ret = audit_socketcall_compat(call, len / sizeof(a[0]), a); See my note below for the non-compat version of socketcall(2). > diff --git a/net/socket.c b/net/socket.c > index 6887840682bb..ff71f28c96f7 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2921,7 +2921,7 @@ SYSCALL_DEFINE2(socketcall, int, call, unsigned long __user *, args) > if (copy_from_user(a, args, len)) > return -EFAULT; > > - err = audit_socketcall(nargs[call] / sizeof(unsigned long), a); > + err = audit_socketcall(call, nargs[call] / sizeof(unsigned long), a); I recognize that this isn't your code, but I think it might be better to cleanup the arg count calculation passed as the second parameter. Something like this not only looks cleaner, but it should be a bit more robust against other kernel changes: err = audit_socketcall(call, len / AL(1), a); ... it may also help resolve some of the kernel test robot errors you are seeing. -- paul-moore.com Next message: kernel test robot: "[asahilinux:bits/130-cpufreq 3/4] drivers/cpufreq/apple-soc-cpufreq.c:67:19: error: implicit declaration of function 'readq_relaxed'; did you mean 'readl_relaxed'?" Previous message: Paolo Bonzini: "Re: [PATCH] KVM: LAPIC: Fix advance timer expiration delta trace" In reply to: kernel test robot: "Re: [PATCH 1/2] audit: add call argument to socketcall auditing" Next in thread: Steve Grubb: "Re: [PATCH 1/2] audit: add call argument to socketcall auditing" Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
