From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Yuchen Wei <weiyuchen3@huawei.com>,
catalin.marinas@arm.com, vincenzo.frascino@arm.com,
keescook@chromium.org, pcc@google.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, He Zhe <zhe.he@windriver.com>
Subject: Re: [PATCH] arm64: audit: fix return value high 32bit truncation problem
Date: Fri, 30 Jul 2021 15:23:36 +0100 [thread overview]
Message-ID: <20210730142336.GA19569@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210730122236.GE23589@willie-the-truck>
Hi,
[adding He Zhe]
On Fri, Jul 30, 2021 at 01:22:37PM +0100, Will Deacon wrote:
> On Thu, Jul 22, 2021 at 02:07:07PM +0800, Yuchen Wei wrote:
> > From: weiyuchen <weiyuchen3@huawei.com>
> >
> > Add error code judgment in invoke_syscall() to prevent kernel
> > components such as audit and tracepoint from obtaining incorrect
> > return values. For example:
> >
> > type=SYSCALL msg=audit(342.780:69): arch=40000028 syscall=235
> > success=yes exit=4294967235
> >
> > The syscall return value is -61, but due to the following process in
> > invoke_syscall():
> >
> > if (is_compat_task())
> > ret = lower_32_bits(ret);
> > regs->regs[0] = ret;
> >
> > The return value audit or tracepoint get from regs[0] is 4294967235,
> > which is an incorrect return value.
> >
> > Signed-off-by: weiyuchen <weiyuchen3@huawei.com>
> > ---
> > arch/arm64/kernel/syscall.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index 263d6c1a525f..f9f042d9a088 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -54,7 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
> > ret = do_ni_syscall(regs, scno);
> > }
> >
> > - if (is_compat_task())
> > + if (is_compat_task() && !IS_ERR_VALUE(ret))
> > ret = lower_32_bits(ret);
>
> Hmm, I'm worried this might break other users who don't expect to see
> non-zero bits for the upper 32-bits of a compat task.
>
> Mark -- I remember you looking into this relatively recently. Where did you
> get to with it?
Sorry, I've been meaning to chase this up for a bit.
There are a few problems here, but I think we can solve them all (patch
below).
Generally, syscall_get_return_value() should be used to get syscall
return values, and this *should* always sign-extend compat values to 64
bits (but arm64's implementation currently doesn't).
Audit currently uses regs_return_value() rather than
syscall_get_return_value(). That's mostly for historical reasons, but
some architectures don't implement syscall_get_return_value() at the
moment, so we'll have to bodge regs_return_value() on arm64 for now. On
32-bit arm regs_return_value() returns a long, and so is sign-extended.
On 32-bit arm, since syscall_get_return_value() returns a long, it'll
get sign-extended (whether returning an errno or a pointer in the high
2GiB), and we should do the same for compat. We can shuffle
syscall_get_return_value() and syscall_get_error() to handle that.
There are a few places we directly assign to the regs where we need to
truncate the assignment. We can use syscall_set_return_value() to do
that for us.
I think for now, the below should be sufficient for arm64, and we can
chase that up with some cleanup to the core audit code to use
syscall_get_return_value().
Thanks,
Mark.
---->8----
From 20131e3e51e4b6034e11df044ae916a853be43de Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Fri, 30 Jul 2021 15:12:41 +0100
Subject: [PATCH] arm64: fix compat syscall return truncation
TODO: explain this.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: He Zhe <zhe.he@windriver.com>
Cc: Will Deacon <will@kernel.org>
Cc: weiyuchen <weiyuchen3@huawei.com>
---
arch/arm64/include/asm/ptrace.h | 12 +++++++++++-
arch/arm64/include/asm/syscall.h | 19 ++++++++++---------
arch/arm64/kernel/ptrace.c | 2 +-
arch/arm64/kernel/signal.c | 3 ++-
arch/arm64/kernel/syscall.c | 9 +++------
5 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..41b332c054ab 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -320,7 +320,17 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
static inline unsigned long regs_return_value(struct pt_regs *regs)
{
- return regs->regs[0];
+ unsigned long val = regs->regs[0];
+
+ /*
+ * Audit currently uses regs_return_value() instead of
+ * syscall_get_return_value(). Apply the same sign-extension here until
+ * audit is updated to use syscall_get_return_value().
+ */
+ if (compat_user_mode(regs))
+ val = sign_extend64(val, 31);
+
+ return val;
}
static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..03e20895453a 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -29,22 +29,23 @@ static inline void syscall_rollback(struct task_struct *task,
regs->regs[0] = regs->orig_x0;
}
-
-static inline long syscall_get_error(struct task_struct *task,
- struct pt_regs *regs)
+static inline long syscall_get_return_value(struct task_struct *task,
+ struct pt_regs *regs)
{
- unsigned long error = regs->regs[0];
+ unsigned long val = regs->regs[0];
if (is_compat_thread(task_thread_info(task)))
- error = sign_extend64(error, 31);
+ val = sign_extend64(val, 31);
- return IS_ERR_VALUE(error) ? error : 0;
+ return val;
}
-static inline long syscall_get_return_value(struct task_struct *task,
- struct pt_regs *regs)
+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
{
- return regs->regs[0];
+ unsigned long error = syscall_get_return_value(task, regs);
+
+ return IS_ERR_VALUE(error) ? error : 0;
}
static inline void syscall_set_return_value(struct task_struct *task,
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 499b6b2f9757..b381a1ee9ea7 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1862,7 +1862,7 @@ void syscall_trace_exit(struct pt_regs *regs)
audit_syscall_exit(regs);
if (flags & _TIF_SYSCALL_TRACEPOINT)
- trace_sys_exit(regs, regs_return_value(regs));
+ trace_sys_exit(regs, syscall_get_return_value(current, regs));
if (flags & (_TIF_SYSCALL_TRACE | _TIF_SINGLESTEP))
tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 53c2c85efb34..62e273b2d83f 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -29,6 +29,7 @@
#include <asm/unistd.h>
#include <asm/fpsimd.h>
#include <asm/ptrace.h>
+#include <asm/syscall.h>
#include <asm/signal32.h>
#include <asm/traps.h>
#include <asm/vdso.h>
@@ -890,7 +891,7 @@ static void do_signal(struct pt_regs *regs)
retval == -ERESTART_RESTARTBLOCK ||
(retval == -ERESTARTSYS &&
!(ksig.ka.sa.sa_flags & SA_RESTART)))) {
- regs->regs[0] = -EINTR;
+ syscall_set_return_value(current, regs, -EINTR, 0);
regs->pc = continue_addr;
}
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 263d6c1a525f..50a0f1a38e84 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -54,10 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
ret = do_ni_syscall(regs, scno);
}
- if (is_compat_task())
- ret = lower_32_bits(ret);
-
- regs->regs[0] = ret;
+ syscall_set_return_value(current, regs, 0, ret);
/*
* Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
@@ -115,7 +112,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* syscall. do_notify_resume() will send a signal to userspace
* before the syscall is restarted.
*/
- regs->regs[0] = -ERESTARTNOINTR;
+ syscall_set_return_value(current, regs, -ERESTARTNOINTR, 0);
return;
}
@@ -136,7 +133,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* anyway.
*/
if (scno == NO_SYSCALL)
- regs->regs[0] = -ENOSYS;
+ syscall_set_return_value(current, regs, -ENOSYS, 0);
scno = syscall_trace_enter(regs);
if (scno == NO_SYSCALL)
goto trace_exit;
--
2.11.0
WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Yuchen Wei <weiyuchen3@huawei.com>,
catalin.marinas@arm.com, vincenzo.frascino@arm.com,
keescook@chromium.org, pcc@google.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, He Zhe <zhe.he@windriver.com>
Subject: Re: [PATCH] arm64: audit: fix return value high 32bit truncation problem
Date: Fri, 30 Jul 2021 15:23:36 +0100 [thread overview]
Message-ID: <20210730142336.GA19569@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210730122236.GE23589@willie-the-truck>
Hi,
[adding He Zhe]
On Fri, Jul 30, 2021 at 01:22:37PM +0100, Will Deacon wrote:
> On Thu, Jul 22, 2021 at 02:07:07PM +0800, Yuchen Wei wrote:
> > From: weiyuchen <weiyuchen3@huawei.com>
> >
> > Add error code judgment in invoke_syscall() to prevent kernel
> > components such as audit and tracepoint from obtaining incorrect
> > return values. For example:
> >
> > type=SYSCALL msg=audit(342.780:69): arch=40000028 syscall=235
> > success=yes exit=4294967235
> >
> > The syscall return value is -61, but due to the following process in
> > invoke_syscall():
> >
> > if (is_compat_task())
> > ret = lower_32_bits(ret);
> > regs->regs[0] = ret;
> >
> > The return value audit or tracepoint get from regs[0] is 4294967235,
> > which is an incorrect return value.
> >
> > Signed-off-by: weiyuchen <weiyuchen3@huawei.com>
> > ---
> > arch/arm64/kernel/syscall.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > index 263d6c1a525f..f9f042d9a088 100644
> > --- a/arch/arm64/kernel/syscall.c
> > +++ b/arch/arm64/kernel/syscall.c
> > @@ -54,7 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
> > ret = do_ni_syscall(regs, scno);
> > }
> >
> > - if (is_compat_task())
> > + if (is_compat_task() && !IS_ERR_VALUE(ret))
> > ret = lower_32_bits(ret);
>
> Hmm, I'm worried this might break other users who don't expect to see
> non-zero bits for the upper 32-bits of a compat task.
>
> Mark -- I remember you looking into this relatively recently. Where did you
> get to with it?
Sorry, I've been meaning to chase this up for a bit.
There are a few problems here, but I think we can solve them all (patch
below).
Generally, syscall_get_return_value() should be used to get syscall
return values, and this *should* always sign-extend compat values to 64
bits (but arm64's implementation currently doesn't).
Audit currently uses regs_return_value() rather than
syscall_get_return_value(). That's mostly for historical reasons, but
some architectures don't implement syscall_get_return_value() at the
moment, so we'll have to bodge regs_return_value() on arm64 for now. On
32-bit arm regs_return_value() returns a long, and so is sign-extended.
On 32-bit arm, since syscall_get_return_value() returns a long, it'll
get sign-extended (whether returning an errno or a pointer in the high
2GiB), and we should do the same for compat. We can shuffle
syscall_get_return_value() and syscall_get_error() to handle that.
There are a few places we directly assign to the regs where we need to
truncate the assignment. We can use syscall_set_return_value() to do
that for us.
I think for now, the below should be sufficient for arm64, and we can
chase that up with some cleanup to the core audit code to use
syscall_get_return_value().
Thanks,
Mark.
---->8----
From 20131e3e51e4b6034e11df044ae916a853be43de Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Fri, 30 Jul 2021 15:12:41 +0100
Subject: [PATCH] arm64: fix compat syscall return truncation
TODO: explain this.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: He Zhe <zhe.he@windriver.com>
Cc: Will Deacon <will@kernel.org>
Cc: weiyuchen <weiyuchen3@huawei.com>
---
arch/arm64/include/asm/ptrace.h | 12 +++++++++++-
arch/arm64/include/asm/syscall.h | 19 ++++++++++---------
arch/arm64/kernel/ptrace.c | 2 +-
arch/arm64/kernel/signal.c | 3 ++-
arch/arm64/kernel/syscall.c | 9 +++------
5 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..41b332c054ab 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -320,7 +320,17 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
static inline unsigned long regs_return_value(struct pt_regs *regs)
{
- return regs->regs[0];
+ unsigned long val = regs->regs[0];
+
+ /*
+ * Audit currently uses regs_return_value() instead of
+ * syscall_get_return_value(). Apply the same sign-extension here until
+ * audit is updated to use syscall_get_return_value().
+ */
+ if (compat_user_mode(regs))
+ val = sign_extend64(val, 31);
+
+ return val;
}
static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..03e20895453a 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -29,22 +29,23 @@ static inline void syscall_rollback(struct task_struct *task,
regs->regs[0] = regs->orig_x0;
}
-
-static inline long syscall_get_error(struct task_struct *task,
- struct pt_regs *regs)
+static inline long syscall_get_return_value(struct task_struct *task,
+ struct pt_regs *regs)
{
- unsigned long error = regs->regs[0];
+ unsigned long val = regs->regs[0];
if (is_compat_thread(task_thread_info(task)))
- error = sign_extend64(error, 31);
+ val = sign_extend64(val, 31);
- return IS_ERR_VALUE(error) ? error : 0;
+ return val;
}
-static inline long syscall_get_return_value(struct task_struct *task,
- struct pt_regs *regs)
+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
{
- return regs->regs[0];
+ unsigned long error = syscall_get_return_value(task, regs);
+
+ return IS_ERR_VALUE(error) ? error : 0;
}
static inline void syscall_set_return_value(struct task_struct *task,
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 499b6b2f9757..b381a1ee9ea7 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1862,7 +1862,7 @@ void syscall_trace_exit(struct pt_regs *regs)
audit_syscall_exit(regs);
if (flags & _TIF_SYSCALL_TRACEPOINT)
- trace_sys_exit(regs, regs_return_value(regs));
+ trace_sys_exit(regs, syscall_get_return_value(current, regs));
if (flags & (_TIF_SYSCALL_TRACE | _TIF_SINGLESTEP))
tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 53c2c85efb34..62e273b2d83f 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -29,6 +29,7 @@
#include <asm/unistd.h>
#include <asm/fpsimd.h>
#include <asm/ptrace.h>
+#include <asm/syscall.h>
#include <asm/signal32.h>
#include <asm/traps.h>
#include <asm/vdso.h>
@@ -890,7 +891,7 @@ static void do_signal(struct pt_regs *regs)
retval == -ERESTART_RESTARTBLOCK ||
(retval == -ERESTARTSYS &&
!(ksig.ka.sa.sa_flags & SA_RESTART)))) {
- regs->regs[0] = -EINTR;
+ syscall_set_return_value(current, regs, -EINTR, 0);
regs->pc = continue_addr;
}
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 263d6c1a525f..50a0f1a38e84 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -54,10 +54,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int scno,
ret = do_ni_syscall(regs, scno);
}
- if (is_compat_task())
- ret = lower_32_bits(ret);
-
- regs->regs[0] = ret;
+ syscall_set_return_value(current, regs, 0, ret);
/*
* Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
@@ -115,7 +112,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* syscall. do_notify_resume() will send a signal to userspace
* before the syscall is restarted.
*/
- regs->regs[0] = -ERESTARTNOINTR;
+ syscall_set_return_value(current, regs, -ERESTARTNOINTR, 0);
return;
}
@@ -136,7 +133,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
* anyway.
*/
if (scno == NO_SYSCALL)
- regs->regs[0] = -ENOSYS;
+ syscall_set_return_value(current, regs, -ENOSYS, 0);
scno = syscall_trace_enter(regs);
if (scno == NO_SYSCALL)
goto trace_exit;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-30 14:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-22 6:07 [PATCH] arm64: audit: fix return value high 32bit truncation problem Yuchen Wei
2021-07-22 6:07 ` Yuchen Wei
2021-07-30 12:22 ` Will Deacon
2021-07-30 12:22 ` Will Deacon
2021-07-30 14:23 ` Mark Rutland [this message]
2021-07-30 14:23 ` Mark Rutland
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210730142336.GA19569@C02TD0UTHF1T.local \
--to=mark.rutland@arm.com \
--cc=catalin.marinas@arm.com \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pcc@google.com \
--cc=vincenzo.frascino@arm.com \
--cc=weiyuchen3@huawei.com \
--cc=will@kernel.org \
--cc=zhe.he@windriver.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.