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 <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Will Drewry <[email protected]>
Cc: [email protected]
Cc: [email protected] # v4.15+
Signed-off-by: Dmitry V. Levin <[email protected]>
---
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, ®s->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
+ memcpy(args, ®s->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(®s->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
+ } else {
+ i--;
+ }
+ memcpy(®s->a1 + i, args, n * sizeof(regs->a1));
}
static inline int syscall_get_arch(void)
--
ldv
On Fri, 29 Mar 2019 20:12:21 +0300
"Dmitry V. Levin" <[email protected]> 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 <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Will Drewry <[email protected]>
> Cc: [email protected]
> Cc: [email protected] # v4.15+
> Signed-off-by: Dmitry V. Levin <[email protected]>
> ---
> 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, ®s->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
> + memcpy(args, ®s->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(®s->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
> + } else {
> + i--;
> + }
> + memcpy(®s->a1 + i, args, n * sizeof(regs->a1));
> }
>
> static inline int syscall_get_arch(void)
On Fri, 29 Mar 2019 18:52:18 +0100
David Abdurachmanov <[email protected]> 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
On Fri, Mar 29, 2019 at 6:15 PM Steven Rostedt <[email protected]> wrote:
>
> On Fri, 29 Mar 2019 20:12:21 +0300
> "Dmitry V. Levin" <[email protected]> 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 <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Andy Lutomirski <[email protected]>
> > Cc: Will Drewry <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected] # v4.15+
> > Signed-off-by: Dmitry V. Levin <[email protected]>
> > ---
> > 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, ®s->a1 + i * sizeof(regs->a1), n * sizeof(args[0]));
> > + memcpy(args, ®s->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(®s->a1 + i * sizeof(regs->a1), args, n * sizeof(regs->a0));
> > + } else {
> > + i--;
> > + }
> > + memcpy(®s->a1 + i, args, n * sizeof(regs->a1));
> > }
> >
> > static inline int syscall_get_arch(void)
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, Mar 29, 2019 at 01:56:35PM -0400, Steven Rostedt wrote:
> On Fri, 29 Mar 2019 18:52:18 +0100
> David Abdurachmanov <[email protected]> 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
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" <[email protected]> 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
On Fri, 29 Mar 2019 21:11:09 +0300
"Dmitry V. Levin" <[email protected]> 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 <[email protected]> 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
On Fri, 29 Mar 2019 21:16:19 +0300
"Dmitry V. Levin" <[email protected]> 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
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" <[email protected]> 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