2018-12-10 04:32:36

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH v5 13/25] m68k: add asm/syscall.h

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 | 39 +++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 arch/m68k/include/asm/syscall.h

diff --git a/arch/m68k/include/asm/syscall.h b/arch/m68k/include/asm/syscall.h
new file mode 100644
index 000000000000..75a24cf90620
--- /dev/null
+++ b/arch/m68k/include/asm/syscall.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_M68K_SYSCALL_H
+#define _ASM_M68K_SYSCALL_H
+
+#include <uapi/linux/audit.h>
+
+static inline int
+syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+{
+ return regs->orig_d0;
+}
+
+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);
+ memcpy(args, &regs->d1 + i, n * sizeof(args[0]));
+}
+
+static inline long
+syscall_get_error(struct task_struct *task, struct pt_regs *regs)
+{
+ return IS_ERR_VALUE(regs->d0) ? regs->d0 : 0;
+}
+
+static inline long
+syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
+{
+ return regs->d0;
+}
+
+static inline int
+syscall_get_arch(void)
+{
+ return AUDIT_ARCH_M68K;
+}
+
+#endif /* _ASM_M68K_SYSCALL_H */
--
ldv


2018-12-10 09:18:32

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

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?
Perhaps you can return an error code instead?

> + memcpy(args, &regs->d1 + i, n * sizeof(args[0]));
> +}

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-10 12:55:24

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> Hi Dmitry,
>
> 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.

> 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.


--
ldv


Attachments:
(No filename) (1.76 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-10 13:45:00

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > 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.
> >
> > > 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.
>
> 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.


--
ldv


Attachments:
(No filename) (2.73 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-10 14:32:54

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > 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.
>
> > 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.

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.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-12 08:57:39

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > 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.
> > >
> > > > 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.
> >
> > 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.

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]));
}


--
ldv


Attachments:
(No filename) (3.59 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-12 09:02:57

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > 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.
> > > >
> > > > > 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.
> > >
> > > 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.
>
> 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;

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?

> }
> 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]));
> }

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-12 09:28:44

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> Hi Dmitry,
>
> On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > 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.
> > > > >
> > > > > > 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.
> > > >
> > > > 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.
> >
> > 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;
>
> 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]));
}
?


--
ldv


Attachments:
(No filename) (4.72 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-12 09:45:22

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > 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.
> > > > > >
> > > > > > > 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.
> > > > >
> > > > > 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.
> > >
> > > 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;
> >
> > 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.

> 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]));
> }
> ?

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.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-12 12:07:39

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > 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.
> > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > 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.
> > > >
> > > > 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;
> > >
> > > 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.
>
> > 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]));
> > }
> > ?
>
> 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.

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

Anyway, as asm-generic/syscall.h is used for documentation purposes only,
I can try to prepare it for inclusion in other files without risk of
starting a big refactoring that would affect all architectures -- a thing
I'd rather not include into this series which is all about adding
PTRACE_GET_SYSCALL_INFO API.

I suppose there is no rush and arch-specific asm/syscall.h can switch
to use asm-generic/syscall.h gradually.


--
ldv


Attachments:
(No filename) (6.28 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-12 12:28:52

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

On Wed, Dec 12, 2018 at 1:04 PM Dmitry V. Levin <[email protected]> wrote:
> On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> > On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > 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;
> > > >
> > > > 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.
> >
> > > 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]));
> > > }
> > > ?
> >
> > 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.
>
> 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.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-12 12:38:52

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Wed, Dec 12, 2018 at 01:27:14PM +0100, Geert Uytterhoeven wrote:
> On Wed, Dec 12, 2018 at 1:04 PM Dmitry V. Levin <[email protected]> wrote:
> > On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > > > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > 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;
> > > > >
> > > > > 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.
> > >
> > > > 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]));
> > > > }
> > > > ?
> > >
> > > 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.
> >
> > 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?


--
ldv


Attachments:
(No filename) (6.87 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-12 12:55:40

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Dmitry,

On Wed, Dec 12, 2018 at 1:37 PM Dmitry V. Levin <[email protected]> wrote:
> On Wed, Dec 12, 2018 at 01:27:14PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Dec 12, 2018 at 1:04 PM Dmitry V. Levin <[email protected]> wrote:
> > > On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > > > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > >
> > > > > > > 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;
> > > > > >
> > > > > > 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.
> > > >
> > > > > 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]));
> > > > > }
> > > > > ?
> > > >
> > > > 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.
> > >
> > > 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,

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

> 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.
And alternatively do the BUG() thing there.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2018-12-12 13:10:22

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Wed, Dec 12, 2018 at 01:54:05PM +0100, Geert Uytterhoeven wrote:
> On Wed, Dec 12, 2018 at 1:37 PM Dmitry V. Levin <[email protected]> wrote:
> > On Wed, Dec 12, 2018 at 01:27:14PM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Dec 12, 2018 at 1:04 PM Dmitry V. Levin <[email protected]> wrote:
> > > > On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> > > > > On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > >
> > > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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;
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > > 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]));
> > > > > > }
> > > > > > ?
> > > > >
> > > > > 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.
> > > >
> > > > 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,
>
> 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.

> > 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.

OK, I'll prepare the change, thanks.


--
ldv


Attachments:
(No filename) (8.06 kB)
signature.asc (817.00 B)
Download all attachments

2018-12-12 23:13:41

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

Hi Geert,

On Wed, Dec 12, 2018 at 04:07:11PM +0300, Dmitry V. Levin wrote:
> On Wed, Dec 12, 2018 at 01:54:05PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Dec 12, 2018 at 1:37 PM Dmitry V. Levin <[email protected]> wrote:
> > > On Wed, Dec 12, 2018 at 01:27:14PM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Dec 12, 2018 at 1:04 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > On Wed, Dec 12, 2018 at 10:43:33AM +0100, Geert Uytterhoeven wrote:
> > > > > > On Wed, Dec 12, 2018 at 10:27 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > On Wed, Dec 12, 2018 at 10:01:29AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > On Wed, Dec 12, 2018 at 9:55 AM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > > > > > > > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > > > > > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > >
> > > > > > > > > > > > > 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.
> > > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > 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;
> > > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > > 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]));
> > > > > > > }
> > > > > > > ?
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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,
> >
> > 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.
>
> > > 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.
>
> OK, I'll prepare the change, thanks.

I have the change ready, but I don't like it. The only architecture
that could benefit from being able of signalling an error condition to
syscall_get_arguments is MIPS, and even in that case the return code is
not suitable because it wouldn't help to distinguish between the first 4
syscall arguments that cannot cause an error and remaining arguments that
can. It looks like there is no need to make __syscall_get_arguments()
to return int after all.


--
ldv


Attachments:
(No filename) (8.88 kB)
signature.asc (817.00 B)
Download all attachments

2019-03-29 22:05:53

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

On Wed, Dec 12, 2018 at 11:55:16AM +0300, Dmitry V. Levin wrote:
> On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > 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.
> > > >
> > > > > 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.
> > >
> > > 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.
>
> 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]));
> }

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]/


--
ldv


Attachments:
(No filename) (4.29 kB)
signature.asc (817.00 B)
Download all attachments

2019-03-30 20:59:23

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v5 13/25] m68k: add asm/syscall.h

CC Steven

On Fri, Mar 29, 2019 at 11:04 PM Dmitry V. Levin <[email protected]> wrote:
> On Wed, Dec 12, 2018 at 11:55:16AM +0300, Dmitry V. Levin wrote:
> > On Mon, Dec 10, 2018 at 04:30:25PM +0300, Dmitry V. Levin wrote:
> > > On Mon, Dec 10, 2018 at 02:06:28PM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, Dec 10, 2018 at 1:41 PM Dmitry V. Levin <[email protected]> wrote:
> > > > > On Mon, Dec 10, 2018 at 09:45:42AM +0100, Geert Uytterhoeven wrote:
> > > > > > 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.
> > > > >
> > > > > > 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.
> > > >
> > > > 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.
> >
> > 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]));
> > }
>
> 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]/

Yeah, no longer a need for all these ugly checks, good.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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