Re: [PATCH 03/13] arm: pass all six syscall args to audit_syscall_entry()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch series changes the signature of `audit_syscall_entry()` globally but fails to update numerous architectures, breaking their builds.

Pre-existing issues:
- [High] A tracer can bypass seccomp filters on ARM by modifying `ARM_r0` via ptrace, as seccomp incorrectly evaluates the unmodified `ARM_ORIG_r0`.
--

--- Patch [3]: [PATCH 03/13] arm: pass all six syscall args to audit_syscall_entry() ---
commit 5e37711667e9af11aeb7334ef9ee7ed66cadd89d
Author: Ricardo Robaina <rrobaina@redhat.com>

arm: pass all six syscall args to audit_syscall_entry()
    
Update the audit_syscall_entry() call to forward the 5th and 6th
syscall arguments (ARM_r4, ARM_r5) now that the audit core logs
all six.

> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 7951b2c06fec6..ace74207738d0 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -869,7 +869,7 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs)

[Severity: High]
This is a pre-existing issue, but can a tracer bypass seccomp filters here?

The ptrace intercept occurs before seccomp:

arch/arm/kernel/ptrace.c:syscall_trace_enter() {
    ...
    if (test_thread_flag(TIF_SYSCALL_TRACE))
        report_syscall(regs, PTRACE_SYSCALL_ENTER);

#ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
    if (secure_computing() == -1)
        return -1;
    ...
}

If the tracer modifies the tracee's first syscall argument (ARM_r0) via
ptrace, does seccomp evaluate the unmodified ARM_ORIG_r0 instead?

arch/arm/include/asm/syscall.h:syscall_get_arguments() {
    ...
    args[0] = regs->ARM_ORIG_r0;
    ...
}

Could this allow restricted system calls to be executed with arbitrary first
arguments?

>  		trace_sys_enter(regs, scno);
>  
>  	audit_syscall_entry(scno, regs->ARM_r0, regs->ARM_r1, regs->ARM_r2,
> -			    regs->ARM_r3);
> +			    regs->ARM_r3, regs->ARM_r4, regs->ARM_r5);

[Severity: High]
Does this patch series break the build for other architectures?

The series changes the signature of audit_syscall_entry() globally to require
7 arguments instead of 5:

include/linux/audit.h:
static inline void audit_syscall_entry(int major, unsigned long a0,
                                       unsigned long a1, unsigned long a2,
                                       unsigned long a3, unsigned long a4,
                                       unsigned long a5)

Are other architectures like arm64 still calling it with 5 arguments?

arch/arm64/kernel/ptrace.c:syscall_trace_enter() {
    ...
    audit_syscall_entry(regs->syscallno, regs->orig_x0, regs->regs[1],
                        regs->regs[2], regs->regs[3]);
    ...
}

Will this result in too few arguments to function compiler errors for
architectures that do not use generic entry?

>  
>  	return scno;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786626275.git.rrobaina@redhat.com?part=3