| Message ID | 20190329171221.GA32456@altlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: fix syscall_get_arguments() and syscall_set_arguments() | expand |
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, ®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, 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, ®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 > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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
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.
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.
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
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
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
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)
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(-)