Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220503090212.1322050-1-svens@linux.ibm.com>
In-Reply-To: <20220503090212.1322050-1-svens@linux.ibm.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 3 May 2022 12:07:45 -0400
Message-ID: <CAHC9VhQ=Lvvy=tOorGQA0FZwggCPkpEk0dFxWQZPkTLn0K2c4g@mail.gmail.com>
Subject: Re: [PATCH 1/2] audit: add call argument to socketcall auditing
To:     Sven Schnelle <svens@linux.ibm.com>
Cc:     Eric Paris <eparis@redhat.com>, linux-audit@redhat.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 3, 2022 at 5:02 AM Sven Schnelle <svens@linux.ibm.com> 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 <svens@linux.ibm.com>
> ---
>  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