Re: [PATCH v5 2/2] m68k: add kernel seccomp support

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Michael,

On Thu, Jun 17, 2021 at 7:39 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
> Add secure_computing() call to syscall_trace_enter to actually
> filter system calls.
>
> Add necessary arch Kconfig options, define TIF_SECCOMP trace
> flag and provide basic seccomp filter support in asm/syscall.h
>
> syscall_get_nr currently uses the syscall nr stored in orig_d0
> because we change d0 to a default return code before starting a
> syscall trace. This may be inconsistent with syscall_rollback
> copying orig_d0 to d0 (which we never check upon return from
> trace). We use d0 for the return code from syscall_trace_enter
> in entry.S currently, and could perhaps expand that to store
> a new syscall number returned by the seccomp filter before
> executing the syscall. This clearly needs some discussion.
>
> Compiles (for Atari) and boots on ARAnyM, otherwise untested.
>
> Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>

Thanks for your patch!

> --- a/arch/m68k/include/asm/syscall.h
> +++ b/arch/m68k/include/asm/syscall.h
> @@ -4,6 +4,39 @@
>
>  #include <uapi/linux/audit.h>
>
> +#include <asm/unistd.h>
> +
> +extern const unsigned long sys_call_table[];
> +
> +static inline int syscall_get_nr(struct task_struct *task,
> +                                struct pt_regs *regs)
> +{
> +       return regs->orig_d0;
> +}
> +
> +static inline void syscall_rollback(struct task_struct *task,
> +                                   struct pt_regs *regs)
> +{
> +       regs->d0 = regs->orig_d0;
> +}
> +
> +static inline void syscall_set_return_value(struct task_struct *task,
> +                                           struct pt_regs *regs,
> +                                           int error, long val)
> +{
> +       regs->d0 = (long) error ? error : val;
> +}
> +
> +static inline void syscall_get_arguments(struct task_struct *task,
> +                                        struct pt_regs *regs,
> +                                        unsigned long *args)
> +{
> +       args[0] = regs->orig_d0;
> +       args++;
> +
> +       memcpy(args, &regs->d0 + 1, 5 * sizeof(args[0]));

This doesn't look right to me: "&regs->d0 + 1" is "&regs->orig_d0"
again, and there are no registers after that.
Perhaps you meant "&regs->d1"?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds