linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
@ 2019-03-29 17:12 Dmitry V. Levin
  2019-03-29 17:15 ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2019-03-29 17:12 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Will Drewry, Kees Cook, linux-kernel, Steven Rostedt,
	Andy Lutomirski, Ingo Molnar, linux-riscv

RISC-V syscall arguments are located in orig_a0,a1..a5 fields
of struct pt_regs.

Due to an off-by-one bug and a bug in pointer arithmetic
syscall_get_arguments() was reading s3..s7 fields instead of a1..a5.
Likewise, syscall_set_arguments() was writing s3..s7 fields
instead of a1..a5.

Fixes: e2c0cdfba7f69 ("RISC-V: User-facing API")
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: linux-riscv@lists.infradead.org
Cc: stable@vger.kernel.org # v4.15+
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 arch/riscv/include/asm/syscall.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/include/asm/syscall.h b/arch/riscv/include/asm/syscall.h
index bba3da6ef157..6ea9e1804233 100644
--- a/arch/riscv/include/asm/syscall.h
+++ b/arch/riscv/include/asm/syscall.h
@@ -79,10 +79,11 @@ static inline void syscall_get_arguments(struct task_struct *task,
 	if (i == 0) {
 		args[0] = regs->orig_a0;
 		args++;
-		i++;
 		n--;
+	} else {
+		i--;
 	}
-	memcpy(args, &regs->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
+	memcpy(args, &regs->a1 + i, n * sizeof(args[0]));
 }
 
 static inline void syscall_set_arguments(struct task_struct *task,
@@ -94,10 +95,11 @@ static inline void syscall_set_arguments(struct task_struct *task,
         if (i == 0) {
                 regs->orig_a0 = args[0];
                 args++;
-                i++;
                 n--;
-        }
-	memcpy(&regs->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
+	} else {
+		i--;
+	}
+	memcpy(&regs->a1 + i, args, n * sizeof(regs->a1));
 }
 
 static inline int syscall_get_arch(void)
-- 
ldv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 17:12 [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments() Dmitry V. Levin
@ 2019-03-29 17:15 ` Steven Rostedt
  2019-03-29 17:52   ` David Abdurachmanov
  2019-03-29 18:16   ` Dmitry V. Levin
  0 siblings, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2019-03-29 17:15 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, linux-riscv

On Fri, 29 Mar 2019 20:12:21 +0300
"Dmitry V. Levin" <ldv@altlinux.org> wrote:

> RISC-V syscall arguments are located in orig_a0,a1..a5 fields
> of struct pt_regs.
> 
> Due to an off-by-one bug and a bug in pointer arithmetic
> syscall_get_arguments() was reading s3..s7 fields instead of a1..a5.
> Likewise, syscall_set_arguments() was writing s3..s7 fields
> instead of a1..a5.

Should I add this to my series? And then rebase on top of it?

-- Steve

> 
> Fixes: e2c0cdfba7f69 ("RISC-V: User-facing API")
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: linux-riscv@lists.infradead.org
> Cc: stable@vger.kernel.org # v4.15+
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>  arch/riscv/include/asm/syscall.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/syscall.h b/arch/riscv/include/asm/syscall.h
> index bba3da6ef157..6ea9e1804233 100644
> --- a/arch/riscv/include/asm/syscall.h
> +++ b/arch/riscv/include/asm/syscall.h
> @@ -79,10 +79,11 @@ static inline void syscall_get_arguments(struct task_struct *task,
>  	if (i == 0) {
>  		args[0] = regs->orig_a0;
>  		args++;
> -		i++;
>  		n--;
> +	} else {
> +		i--;
>  	}
> -	memcpy(args, &regs->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
> +	memcpy(args, &regs->a1 + i, n * sizeof(args[0]));
>  }
>  
>  static inline void syscall_set_arguments(struct task_struct *task,
> @@ -94,10 +95,11 @@ static inline void syscall_set_arguments(struct task_struct *task,
>          if (i == 0) {
>                  regs->orig_a0 = args[0];
>                  args++;
> -                i++;
>                  n--;
> -        }
> -	memcpy(&regs->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
> +	} else {
> +		i--;
> +	}
> +	memcpy(&regs->a1 + i, args, n * sizeof(regs->a1));
>  }
>  
>  static inline int syscall_get_arch(void)


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 17:15 ` Steven Rostedt
@ 2019-03-29 17:52   ` David Abdurachmanov
  2019-03-29 17:56     ` Steven Rostedt
  2019-03-29 18:16   ` Dmitry V. Levin
  1 sibling, 1 reply; 9+ messages in thread
From: David Abdurachmanov @ 2019-03-29 17:52 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, linux-riscv, Dmitry V. Levin

On Fri, Mar 29, 2019 at 6:15 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 29 Mar 2019 20:12:21 +0300
> "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
> > RISC-V syscall arguments are located in orig_a0,a1..a5 fields
> > of struct pt_regs.
> >
> > Due to an off-by-one bug and a bug in pointer arithmetic
> > syscall_get_arguments() was reading s3..s7 fields instead of a1..a5.
> > Likewise, syscall_set_arguments() was writing s3..s7 fields
> > instead of a1..a5.
>
> Should I add this to my series? And then rebase on top of it?

I have alternative version posted in December part of SECCOMP
patchset which is based on arm64 implementation.

http://lists.infradead.org/pipermail/linux-riscv/2018-December/002450.html

I noticed that SECCOMP wasn't working properly if filters were
checking syscall arguments, because populated arguments were wrong.

Btw, I plan to send v2 of SECCOMP patchset soonish.

david

>
> -- Steve
>
> >
> > Fixes: e2c0cdfba7f69 ("RISC-V: User-facing API")
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: linux-riscv@lists.infradead.org
> > Cc: stable@vger.kernel.org # v4.15+
> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> > ---
> >  arch/riscv/include/asm/syscall.h | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/syscall.h b/arch/riscv/include/asm/syscall.h
> > index bba3da6ef157..6ea9e1804233 100644
> > --- a/arch/riscv/include/asm/syscall.h
> > +++ b/arch/riscv/include/asm/syscall.h
> > @@ -79,10 +79,11 @@ static inline void syscall_get_arguments(struct task_struct *task,
> >       if (i == 0) {
> >               args[0] = regs->orig_a0;
> >               args++;
> > -             i++;
> >               n--;
> > +     } else {
> > +             i--;
> >       }
> > -     memcpy(args, &regs->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
> > +     memcpy(args, &regs->a1 + i, n * sizeof(args[0]));
> >  }
> >
> >  static inline void syscall_set_arguments(struct task_struct *task,
> > @@ -94,10 +95,11 @@ static inline void syscall_set_arguments(struct task_struct *task,
> >          if (i == 0) {
> >                  regs->orig_a0 = args[0];
> >                  args++;
> > -                i++;
> >                  n--;
> > -        }
> > -     memcpy(&regs->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
> > +     } else {
> > +             i--;
> > +     }
> > +     memcpy(&regs->a1 + i, args, n * sizeof(regs->a1));
> >  }
> >
> >  static inline int syscall_get_arch(void)
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 17:52   ` David Abdurachmanov
@ 2019-03-29 17:56     ` Steven Rostedt
  2019-03-29 18:11       ` Dmitry V. Levin
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2019-03-29 17:56 UTC (permalink / raw)
  To: David Abdurachmanov
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, linux-riscv, Dmitry V. Levin

On Fri, 29 Mar 2019 18:52:18 +0100
David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:

> I have alternative version posted in December part of SECCOMP
> patchset which is based on arm64 implementation.
> 
> http://lists.infradead.org/pipermail/linux-riscv/2018-December/002450.html
> 
> I noticed that SECCOMP wasn't working properly if filters were
> checking syscall arguments, because populated arguments were wrong.
> 
> Btw, I plan to send v2 of SECCOMP patchset soonish.

Please do. I want to get my patch series out, which will require these
changes.

-- Steve

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 17:56     ` Steven Rostedt
@ 2019-03-29 18:11       ` Dmitry V. Levin
  2019-03-29 20:32         ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2019-03-29 18:11 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Will Drewry, Kees Cook, David Abdurachmanov, Palmer Dabbelt,
	linux-kernel, Andy Lutomirski, Ingo Molnar, linux-riscv


[-- Attachment #1.1: Type: text/plain, Size: 1092 bytes --]

On Fri, Mar 29, 2019 at 01:56:35PM -0400, Steven Rostedt wrote:
> On Fri, 29 Mar 2019 18:52:18 +0100
> David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> 
> > I have alternative version posted in December part of SECCOMP
> > patchset which is based on arm64 implementation.
> > 
> > http://lists.infradead.org/pipermail/linux-riscv/2018-December/002450.html
> > 
> > I noticed that SECCOMP wasn't working properly if filters were
> > checking syscall arguments, because populated arguments were wrong.
> > 
> > Btw, I plan to send v2 of SECCOMP patchset soonish.
> 
> Please do. I want to get my patch series out, which will require these
> changes.

Sorry, I haven't seen the alternative patch posted by David before.
Apparently, besides fixing the bug it also introduces new sanity checks
of "i" and "n" arguments in syscall_get_arguments() and
syscall_set_arguments().

Given that your patchset removes these arguments completely,
I see little sense in adding new checks that are going to be removed
by the subsequent commit in the series.


-- 
ldv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 17:15 ` Steven Rostedt
  2019-03-29 17:52   ` David Abdurachmanov
@ 2019-03-29 18:16   ` Dmitry V. Levin
  2019-03-29 20:33     ` Steven Rostedt
  1 sibling, 1 reply; 9+ messages in thread
From: Dmitry V. Levin @ 2019-03-29 18:16 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, Guo Ren, linux-riscv


[-- Attachment #1.1: Type: text/plain, Size: 669 bytes --]

On Fri, Mar 29, 2019 at 01:15:14PM -0400, Steven Rostedt wrote:
> On Fri, 29 Mar 2019 20:12:21 +0300
> "Dmitry V. Levin" <ldv@altlinux.org> wrote:
> 
> > RISC-V syscall arguments are located in orig_a0,a1..a5 fields
> > of struct pt_regs.
> > 
> > Due to an off-by-one bug and a bug in pointer arithmetic
> > syscall_get_arguments() was reading s3..s7 fields instead of a1..a5.
> > Likewise, syscall_set_arguments() was writing s3..s7 fields
> > instead of a1..a5.
> 
> Should I add this to my series? And then rebase on top of it?

This is fine with me.  If you are adding the fix for riscv,
please consider adding the fix for csky, too.


-- 
ldv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 18:11       ` Dmitry V. Levin
@ 2019-03-29 20:32         ` Steven Rostedt
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2019-03-29 20:32 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Will Drewry, Kees Cook, David Abdurachmanov, Palmer Dabbelt,
	linux-kernel, Andy Lutomirski, Ingo Molnar, linux-riscv

On Fri, 29 Mar 2019 21:11:09 +0300
"Dmitry V. Levin" <ldv@altlinux.org> wrote:

> On Fri, Mar 29, 2019 at 01:56:35PM -0400, Steven Rostedt wrote:
> > On Fri, 29 Mar 2019 18:52:18 +0100
> > David Abdurachmanov <david.abdurachmanov@gmail.com> wrote:
> >   
> > > I have alternative version posted in December part of SECCOMP
> > > patchset which is based on arm64 implementation.
> > > 
> > > http://lists.infradead.org/pipermail/linux-riscv/2018-December/002450.html
> > > 
> > > I noticed that SECCOMP wasn't working properly if filters were
> > > checking syscall arguments, because populated arguments were wrong.
> > > 
> > > Btw, I plan to send v2 of SECCOMP patchset soonish.  
> > 
> > Please do. I want to get my patch series out, which will require these
> > changes.  
> 
> Sorry, I haven't seen the alternative patch posted by David before.
> Apparently, besides fixing the bug it also introduces new sanity checks
> of "i" and "n" arguments in syscall_get_arguments() and
> syscall_set_arguments().
> 
> Given that your patchset removes these arguments completely,
> I see little sense in adding new checks that are going to be removed
> by the subsequent commit in the series.

I agree. I'm going to pull in Dmitry's patches as my patches are going
to rewrite most the code anyway, and no need for the extra churn of the
sanity checks that are going to become irrelevant immediately afterward.

-- Steve

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 18:16   ` Dmitry V. Levin
@ 2019-03-29 20:33     ` Steven Rostedt
  2019-03-30  0:26       ` Guo Ren
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2019-03-29 20:33 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, Guo Ren, linux-riscv

On Fri, 29 Mar 2019 21:16:19 +0300
"Dmitry V. Levin" <ldv@altlinux.org> wrote:

> This is fine with me.  If you are adding the fix for riscv,
> please consider adding the fix for csky, too.

Yes of course. I mentioned both of these fixes in my reply to Linus.

-- Steve

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments()
  2019-03-29 20:33     ` Steven Rostedt
@ 2019-03-30  0:26       ` Guo Ren
  0 siblings, 0 replies; 9+ messages in thread
From: Guo Ren @ 2019-03-30  0:26 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Will Drewry, Kees Cook, Palmer Dabbelt, linux-kernel,
	Andy Lutomirski, Ingo Molnar, linux-riscv, Dmitry V. Levin

Thx Dmitry & Steve,

On Fri, Mar 29, 2019 at 04:33:37PM -0400, Steven Rostedt wrote:
> On Fri, 29 Mar 2019 21:16:19 +0300
> "Dmitry V. Levin" <ldv@altlinux.org> wrote:
> 
> > This is fine with me.  If you are adding the fix for riscv,
> > please consider adding the fix for csky, too.
> 
> Yes of course. I mentioned both of these fixes in my reply to Linus.
The BUG is from this commit before upstream:
https://github.com/c-sky/csky-linux/commit/b167422869e6a76b31bda639413efa2ba7e60432#diff-cdc97efef2ab02d6828fa545698f9311

I reference the riscv code without my mind, thx for all.

Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-03-30  0:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 17:12 [PATCH] riscv: fix syscall_get_arguments() and syscall_set_arguments() Dmitry V. Levin
2019-03-29 17:15 ` Steven Rostedt
2019-03-29 17:52   ` David Abdurachmanov
2019-03-29 17:56     ` Steven Rostedt
2019-03-29 18:11       ` Dmitry V. Levin
2019-03-29 20:32         ` Steven Rostedt
2019-03-29 18:16   ` Dmitry V. Levin
2019-03-29 20:33     ` Steven Rostedt
2019-03-30  0:26       ` Guo Ren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).