ptrace: add PTRACE_GET_SYSCALL_INFO request

syscall_get_arch() is required to be implemented on all architectures in addition to already implemented syscall_get_nr(), syscall_get_arguments(), syscall_get_error(), and syscall_get_return_value() functions in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

Cc Mark Salter <[email protected]>

Cc Aurelien Jacquiot <[email protected]>

Cc Paul Moore <[email protected]>

Cc Eric Paris <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added Cc

 arch/c6x/include/asm/syscall.h | 7 +++++++
 include/uapi/linux/audit.h     | 2 ++
 2 files changed, 9 insertions(+)

arch/c6x/include/asm/syscall.h +7 -0

index ae2be315ee9c..39dbd1ef994c 100644
@@ -11,6 +11,7 @@
 #ifndef __ASM_C6X_SYSCALL_H
 #define __ASM_C6X_SYSCALL_H
 
+#include <uapi/linux/audit.h>
 #include <linux/err.h>
 #include <linux/sched.h>
 

@@ -120,4 +121,10 @@ static inline void syscall_set_arguments(struct task_struct *task,
 	}
 }
 
+static inline int syscall_get_arch(void)
+{
+	return IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)
+		? AUDIT_ARCH_C6XBE : AUDIT_ARCH_C6X;
+}
+
 #endif /* __ASM_C6X_SYSCALLS_H */

include/uapi/linux/audit.h +0 -0

index bedf3bf54c3a..72aeea0a740d 100644

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

Hi Dmitry,

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

Perhaps you can return an error code instead?

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
        }
        BUILD_BUG_ON(sizeof(regs->d1) != sizeof(args[0]));
        memcpy(args, &regs->d1 + i, n * sizeof(args[0]));
}

Hi Dmitry,

Quoted 70 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

Quoted 9 lines

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

?

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

And make the latter return int, so it can indicate other failures.

Quoted 114 lines · Geert Uytterhoeven

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

Quoted 4 lines

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

And make the latter return int, so it can indicate other failures.

Other failures? What syscall_get_arguments is expected to do if __syscall_get_arguments returned, say, -1?

Quoted 118 lines · Dmitry V. Levin

Quoted 117 lines · Geert Uytterhoeven

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

Quoted 4 lines

Quoted 1 line

And make the latter return int, so it can indicate other failures.

Other failures? What syscall_get_arguments is expected to do if __syscall_get_arguments returned, say, -1?

Fail. Just like in case of other generic ill conditions it can detect itself.

Quoted 119 lines · Geert Uytterhoeven

Quoted 118 lines · Dmitry V. Levin

Quoted 117 lines · Geert Uytterhoeven

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

And make the latter return int, so it can indicate other failures.

Other failures? What syscall_get_arguments is expected to do if __syscall_get_arguments returned, say, -1?

Fail. Just like in case of other generic ill conditions it can detect itself.

Sorry, I don't quite follow. syscall_get_arguments() has no return code,

so all it can possibly do is to zero out args[], e.g.

        if (unlikely(__syscall_get_arguments(task, regs, i, n, args) < 0)) {
                memset(args, 0, n * sizeof(args[0]));
                return;
        }

Do you mean this?

Quoted 125 lines · Dmitry V. Levin

Quoted 124 lines · Geert Uytterhoeven

Quoted 122 lines · Dmitry V. Levin

Quoted 111 lines · Geert Uytterhoeven

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all
architectures in order to extend the generic ptrace API with
PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_*
functions as documented in asm-generic/syscall.h: syscall_get_nr,
syscall_get_arguments, syscall_get_error, syscall_get_return_value,
and syscall_get_arch.

Cc: Geert Uytterhoeven <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

--- /dev/null
+++ b/arch/m68k/include/asm/syscall.h
@@ -0,0 +1,39 @@

Quoted 7 lines

Quoted 5 lines

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+ unsigned int i, unsigned int n, unsigned long *args)
+{
+ BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

Quoted 7 lines

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

And make the latter return int, so it can indicate other failures.

Other failures? What syscall_get_arguments is expected to do if __syscall_get_arguments returned, say, -1?

Quoted 3 lines

Quoted 1 line

Fail. Just like in case of other generic ill conditions it can detect itself.

Sorry, I don't quite follow. syscall_get_arguments() has no return code,

Which may be an indicator for a different problem. What is e.g. populate_seccomp_data() supposed to do if syscall_get_arguments() fails?

Quoted 8 lines

so all it can possibly do is to zero out args[], e.g.

        if (unlikely(__syscall_get_arguments(task, regs, i, n, args) < 0)) {
                memset(args, 0, n * sizeof(args[0]));
                return;
        }

Do you mean this?

Exactly.

Quoted 126 lines · Geert Uytterhoeven

Quoted 125 lines · Dmitry V. Levin

Quoted 124 lines · Geert Uytterhoeven

Quoted 122 lines · Dmitry V. Levin

Quoted 111 lines · Geert Uytterhoeven

Quoted 82 lines · Dmitry V. Levin

Quoted 81 lines · Geert Uytterhoeven

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 35 lines · Geert Uytterhoeven

On Mon, Dec 10, 2018 at 5:30 AM Dmitry V. Levin <[email protected]> wrote:
syscall_get_* functions are required to be implemented on all
architectures in order to extend the generic ptrace API with
PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_*
functions as documented in asm-generic/syscall.h: syscall_get_nr,
syscall_get_arguments, syscall_get_error, syscall_get_return_value,
and syscall_get_arch.

Cc: Geert Uytterhoeven <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

--- /dev/null
+++ b/arch/m68k/include/asm/syscall.h
@@ -0,0 +1,39 @@

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+ unsigned int i, unsigned int n, unsigned long *args)
+{
+ BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
        /*
         * Ideally there should have been
         * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
         * instead of these checks.
         */
        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                return;

Quoted 7 lines

Quoted 3 lines

Does this have security implications, as args is an output parameter? I.e. if you don't fill the array, the caller will use whatever is on the stack. Can this ever be passed to userspace, leaking data?

In the current kernel code n is always less or equal to 6, but in theory future changes can potentially break the assertion and this could lead to leaking data to userspace.

OK.

Quoted 14 lines

Do you think we should rather be defensive and add some memsets, e.g.

        if (unlikely(i > SYSCALL_MAX_ARGS)) {
                WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
                memset(args, 0, n * sizeof(args[0]));
                return;
        }
        if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
                unsigned int extra = n - (SYSCALL_MAX_ARGS - i);

                WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
                n = SYSCALL_MAX_ARGS - i;
                memset(&args[n], 0, extra * sizeof(args[0]));
        }

Quoted 1 line

?

Yes please.

Quoted 7 lines

But please handle all of that in the generic code, so it doesn't have to be replicated across all architectures.

E.g. make syscall_get_arguments() a wrapper in generic code, calling __syscall_get_arguments() in architecture-specific code.

And make the latter return int, so it can indicate other failures.

Other failures? What syscall_get_arguments is expected to do if __syscall_get_arguments returned, say, -1?

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Fail. Just like in case of other generic ill conditions it can detect itself.

Sorry, I don't quite follow. syscall_get_arguments() has no return code,

Which may be an indicator for a different problem. What is e.g. populate_seccomp_data() supposed to do if syscall_get_arguments() fails?

Well, syscall_get_arguments() is not supposed to fail if invoked properly.

Currently populate_seccomp_data() does this:

	struct task_struct *task = current;
	struct pt_regs *regs = task_pt_regs(task);
	unsigned long args[6];
	...
	syscall_get_arguments(task, regs, 0, 6, args);

I don't see how this could fail.

Quoted 7 lines

Quoted 7 lines

so all it can possibly do is to zero out args[], e.g.

        if (unlikely(__syscall_get_arguments(task, regs, i, n, args) < 0)) {
                memset(args, 0, n * sizeof(args[0]));
                return;
        }

Quoted 3 lines

Quoted 1 line

Do you mean this?

Exactly.

OK, I'll prepare the change, thanks.

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 17

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
	/*
	 * Ideally there should have been
	 * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
	 * instead of these checks.
	 */
	if (unlikely(i > SYSCALL_MAX_ARGS)) {
		WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
		return;

	}
	if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
		WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
		n = SYSCALL_MAX_ARGS - i;
	}
	BUILD_BUG_ON(sizeof(regs->d1) != sizeof(args[0]));
	memcpy(args, &regs->d1 + i, n * sizeof(args[0]));
}

Quoted 79 lines · Dmitry V. Levin

Quoted 53 lines · Dmitry V. Levin

Quoted 43 lines · Geert Uytterhoeven

Quoted 42 lines · Dmitry V. Levin

Quoted 28 lines · Geert Uytterhoeven

Quoted 26 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This introduces asm/syscall.h on m68k implementing all 5 syscall_get_* functions as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

Cc Geert Uytterhoeven <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Elvira Khabirova <[email protected]>

Cc Eugene Syromyatnikov <[email protected]>

Cc [email protected]

Signed-off-by Dmitry V. Levin <[email protected]>

---

Notes:
v5: added syscall_get_nr, syscall_get_arguments, syscall_get_error,
and syscall_get_return_value
v1: added syscall_get_arch

arch/m68k/include/asm/syscall.h new file +0 -0

@@ -0,0 +1,39 @@

Quoted 7 lines

arch/m68k/include/asm/syscall.h line 5

Quoted 5 lines

arch/m68k/include/asm/syscall.h new file +5 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       BUG_ON(i + n > 6);

Does this have to crash the kernel?

This is what most of other architectures do, but we could choose a softer approach, e.g. use WARN_ON_ONCE instead.

Quoted 1 line

Perhaps you can return an error code instead?

Quoted 8 lines

Quoted 2 lines

That would be problematic given the signature of this function and the nature of the potential bug which would most likely be a usage error.

Of course to handle that, the function's signature need to be changed. Changing it has the advantage that the error handling can be done at the caller, in common code, instead of duplicating it for all architectures, possibly
leading to different semantics.

Quoted 8 lines

Given that *all* current users of syscall_get_arguments specify i == 0 (and there is an architecture that has BUG_ON(i)),
it should be really a usage error to get into situation where i + n > 6, I wish a BUILD_BUG_ON could be used here instead.

I don't think it worths pushing the change of API just to convert a "cannot happen" assertion into an error that would have to be dealt with on the caller side.

I suggest the following BUG_ON replacement for syscall_get_arguments:

#define SYSCALL_MAX_ARGS 6

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n, unsigned long *args)

{
      /*
       * Ideally there should have been
       * BUILD_BUG_ON(i + n > SYSCALL_MAX_ARGS);
       * instead of these checks.
       */
      if (unlikely(i > SYSCALL_MAX_ARGS)) {
              WARN_ONCE(1, "i > SYSCALL_MAX_ARGS");
              return;

Quoted 8 lines

      }
      if (unlikely(n > SYSCALL_MAX_ARGS - i)) {
              WARN_ONCE(1, "i + n > SYSCALL_MAX_ARGS");
              n = SYSCALL_MAX_ARGS - i;
      }
      BUILD_BUG_ON(sizeof(regs->d1) != sizeof(args[0]));
      memcpy(args, &regs->d1 + i, n * sizeof(args[0]));
}

There seems to be a more straightforward approach to this issue.

Assuming there is a general consensus [1] to get rid of "i" and "n" arguments of syscall_get_arguments(), the implementation could be simplified to

static inline void
syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned long *args)

{
        memcpy(args, &regs->d1, 6 * sizeof(args[0]));
}

[1] https://lore.kernel.org/lkml/[email protected]/

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

This adds all 5 syscall_get_* functions on xtensa as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

arch/xtensa/include/asm/syscall.h +0 -0

index 3673ff1f1bc5..d529c855a144 100644

arch/xtensa/include/asm/syscall.h +31 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       switch (i) {
+       case 0:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[6];
+               /* fall through */
+       case 1:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[3];
+               /* fall through */
+       case 2:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[4];
+               /* fall through */
+       case 3:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[5];
+               /* fall through */
+       case 4:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[8];
+               /* fall through */
+       case 5:

Hello,

Quoted 4 lines · Dmitry V. Levin

syscall_get_* functions are required to be implemented on all architectures in order to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request.

Quoted 3 lines

This adds all 5 syscall_get_* functions on xtensa as documented in asm-generic/syscall.h: syscall_get_nr, syscall_get_arguments, syscall_get_error, syscall_get_return_value, and syscall_get_arch.

I have this set of functions plus syscall_set_arguments implemented for syscall tracing here:
https://github.com/jcmvbkbc/linux-xtensa/commit/0023f56298cc92ce47e61b1b5dd1038f7be4f826

How should we synchronize our changes?

Quoted 4 lines

arch/xtensa/include/asm/syscall.h +0 -0

index 3673ff1f1bc5..d529c855a144 100644

[...]

Quoted 35 lines

arch/xtensa/include/asm/syscall.h +35 -0

+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+                     unsigned int i, unsigned int n, unsigned long *args)
+{
+       switch (i) {
+       case 0:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[6];
+               /* fall through */
+       case 1:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[3];
+               /* fall through */
+       case 2:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[4];
+               /* fall through */
+       case 3:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[5];
+               /* fall through */
+       case 4:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[8];
+               /* fall through */
+       case 5:
+               if (!n--)
+                       break;
+               *args++ = regs->areg[9];
+               /* fall through */

arch/xtensa/include/asm/syscall.h line 59

Quoted 6 lines

arch/xtensa/include/asm/syscall.h +6 -0

+       case 6:
+               if (!n--)
+                       break;
+               /* fall through */
+       default:
+               BUG();

A WARN should be enough.

Quoted 1 line · Max Filippov

How should we synchronize our changes?

No problem, I can revert to the previous edition of this patch that just adds syscall_get_arch.
Alternatively, you can just take that couple of patches (v5 18/25 and v2 15/15) into your tree.

Quoted 7 lines · Dmitry V. Levin

Quoted 1 line · Max Filippov

How should we synchronize our changes?

No problem, I can revert to the previous edition of this patch that just adds syscall_get_arch.
Alternatively, you can just take that couple of patches (v5 18/25 and v2 15/15) into your tree.

Sure I can do the second. Will it work for v2 16/15 that changes syscall_get_arch adding an argument to it?

Quoted 9 lines · Max Filippov

Quoted 7 lines · Dmitry V. Levin

Quoted 1 line · Max Filippov

How should we synchronize our changes?

No problem, I can revert to the previous edition of this patch that just adds syscall_get_arch.
Alternatively, you can just take that couple of patches (v5 18/25 and v2 15/15) into your tree.

Quoted 2 lines

Sure I can do the second. Will it work for v2 16/15 that changes syscall_get_arch adding an argument to it?

No, I'm afraid it won't work for v2 16/15 (aka v5 22/25), which means I'd have to keep them in the series.

Quoted 13 lines · Dmitry V. Levin

Quoted 11 lines · Max Filippov

Quoted 7 lines · Dmitry V. Levin

Quoted 1 line · Max Filippov

How should we synchronize our changes?

No problem, I can revert to the previous edition of this patch that just adds syscall_get_arch.
Alternatively, you can just take that couple of patches (v5 18/25 and v2 15/15) into your tree.

Sure I can do the second. Will it work for v2 16/15 that changes syscall_get_arch adding an argument to it?

Quoted 2 lines

No, I'm afraid it won't work for v2 16/15 (aka v5 22/25), which means I'd have to keep them in the series.

You can surely take them into your tree, but I'll have to keep them in the series because of that change of syscall_get_arch signature. Sorry for confusion.