linux-audit.redhat.com archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
@ 2021-04-16  7:55 He Zhe
  2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: He Zhe @ 2021-04-16  7:55 UTC (permalink / raw)
  To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis,
	linux-audit, linux-kernel, zhe.he

The general version of is_syscall_success does not handle 32-bit
compatible case, which would cause 32-bit negative return code to be
recoganized as a positive number later and seen as a "success".

Since is_compat_thread is defined in compat.h, implementing
is_syscall_success in ptrace.h would introduce build failure due to
recursive inclusion of some basic headers like mutex.h. We put the
implementation to ptrace.c

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/arm64/include/asm/ptrace.h |  3 +++
 arch/arm64/kernel/ptrace.c      | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..3c415e9e5d85 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
 	regs->regs[0] = rc;
 }
 
+extern inline int is_syscall_success(struct pt_regs *regs);
+#define is_syscall_success(regs) is_syscall_success(regs)
+
 /**
  * regs_get_kernel_argument() - get Nth function argument in kernel
  * @regs:	pt_regs of that context
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 170f42fd6101..3266201f8c60 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
 	else
 		return valid_native_regs(regs);
 }
+
+inline int is_syscall_success(struct pt_regs *regs)
+{
+	unsigned long val = regs->regs[0];
+
+	if (is_compat_thread(task_thread_info(current)))
+		val = sign_extend64(val, 31);
+
+	return !IS_ERR_VALUE(val);
+}
-- 
2.17.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
  2021-04-16  7:55 [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe
@ 2021-04-16  7:55 ` He Zhe
  2021-04-16  9:43   ` Oleg Nesterov
  2021-04-21 17:41   ` Mark Rutland
  2021-04-16  7:55 ` [PATCH 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe
  2021-04-16 12:33 ` [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Catalin Marinas
  2 siblings, 2 replies; 18+ messages in thread
From: He Zhe @ 2021-04-16  7:55 UTC (permalink / raw)
  To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis,
	linux-audit, linux-kernel, zhe.he

Add sign extension handling in syscall_get_return_value so that it can
handle 32-bit compatible case and can be used by for example audit, just
like what syscall_get_error does.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/arm64/include/asm/syscall.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..cd7a22787aeb 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -44,7 +44,12 @@ static inline long syscall_get_error(struct task_struct *task,
 static inline long syscall_get_return_value(struct task_struct *task,
 					    struct pt_regs *regs)
 {
-	return regs->regs[0];
+	long val = regs->regs[0];
+
+	if (is_compat_thread(task_thread_info(task)))
+		val = sign_extend64(val, 31);
+
+	return val;
 }
 
 static inline void syscall_set_return_value(struct task_struct *task,
-- 
2.17.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* [PATCH 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
  2021-04-16  7:55 [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe
  2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
@ 2021-04-16  7:55 ` He Zhe
  2021-04-16 12:33 ` [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Catalin Marinas
  2 siblings, 0 replies; 18+ messages in thread
From: He Zhe @ 2021-04-16  7:55 UTC (permalink / raw)
  To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis,
	linux-audit, linux-kernel, zhe.he

regs_return_value for some architectures like arm64 simply retrieve
register value from pt_regs without sign extension in 32-bit compatible
case and cause audit to have false syscall return code. For example,
32-bit -13 would be treated as 4294967283 below.

type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
success=yes exit=4294967283

We just added proper sign extension in syscall_get_return_value which
should be used instead.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 include/linux/audit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 82b7c1116a85..135adbe22c19 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
 {
 	if (unlikely(audit_context())) {
 		int success = is_syscall_success(pt_regs);
-		long return_code = regs_return_value(pt_regs);
+		long return_code = syscall_get_return_value(current, pt_regs);
 
 		__audit_syscall_exit(success, return_code);
 	}
-- 
2.17.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
  2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
@ 2021-04-16  9:43   ` Oleg Nesterov
  2021-04-20  8:38     ` He Zhe
  2021-04-21 17:41   ` Mark Rutland
  1 sibling, 1 reply; 18+ messages in thread
From: Oleg Nesterov @ 2021-04-16  9:43 UTC (permalink / raw)
  To: He Zhe; +Cc: catalin.marinas, linux-kernel, linux-audit, will, linux-arm-kernel

On 04/16, He Zhe wrote:
>
> --- a/arch/arm64/include/asm/syscall.h
> +++ b/arch/arm64/include/asm/syscall.h
> @@ -44,7 +44,12 @@ static inline long syscall_get_error(struct task_struct *task,
>  static inline long syscall_get_return_value(struct task_struct *task,
>  					    struct pt_regs *regs)
>  {
> -	return regs->regs[0];
> +	long val = regs->regs[0];
> +
> +	if (is_compat_thread(task_thread_info(task)))
> +		val = sign_extend64(val, 31);
> +
> +	return val;
>  }

I can't really review these arm-specific patches, but with this change both
syscall_get_error() and is_syscall_success() can use syscall_get_return_value()
to avoid the code duplication.

Oleg.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-16  7:55 [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe
  2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
  2021-04-16  7:55 ` [PATCH 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe
@ 2021-04-16 12:33 ` Catalin Marinas
  2021-04-16 13:34   ` Mark Rutland
  2021-04-20  8:42   ` He Zhe
  2 siblings, 2 replies; 18+ messages in thread
From: Catalin Marinas @ 2021-04-16 12:33 UTC (permalink / raw)
  To: He Zhe
  Cc: Mark Rutland, oleg, linux-kernel, linux-audit, will, linux-arm-kernel

On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
> The general version of is_syscall_success does not handle 32-bit
> compatible case, which would cause 32-bit negative return code to be
> recoganized as a positive number later and seen as a "success".
> 
> Since is_compat_thread is defined in compat.h, implementing
> is_syscall_success in ptrace.h would introduce build failure due to
> recursive inclusion of some basic headers like mutex.h. We put the
> implementation to ptrace.c
> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  arch/arm64/include/asm/ptrace.h |  3 +++
>  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index e58bca832dff..3c415e9e5d85 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>  	regs->regs[0] = rc;
>  }
>  
> +extern inline int is_syscall_success(struct pt_regs *regs);
> +#define is_syscall_success(regs) is_syscall_success(regs)
> +
>  /**
>   * regs_get_kernel_argument() - get Nth function argument in kernel
>   * @regs:	pt_regs of that context
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 170f42fd6101..3266201f8c60 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
>  	else
>  		return valid_native_regs(regs);
>  }
> +
> +inline int is_syscall_success(struct pt_regs *regs)
> +{
> +	unsigned long val = regs->regs[0];
> +
> +	if (is_compat_thread(task_thread_info(current)))
> +		val = sign_extend64(val, 31);
> +
> +	return !IS_ERR_VALUE(val);
> +}

It's better to use compat_user_mode(regs) here instead of
is_compat_thread(). It saves us from worrying whether regs are for the
current context.

I think we should change regs_return_value() instead. This function
seems to be called from several other places and it has the same
potential problems if called on compat pt_regs.

-- 
Catalin

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-16 12:33 ` [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Catalin Marinas
@ 2021-04-16 13:34   ` Mark Rutland
  2021-04-17 13:19     ` David Laight
  2021-04-19 12:19     ` Will Deacon
  2021-04-20  8:42   ` He Zhe
  1 sibling, 2 replies; 18+ messages in thread
From: Mark Rutland @ 2021-04-16 13:34 UTC (permalink / raw)
  To: Catalin Marinas, will
  Cc: He Zhe, oleg, linux-kernel, linux-audit, linux-arm-kernel

On Fri, Apr 16, 2021 at 01:33:22PM +0100, Catalin Marinas wrote:
> On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
> > The general version of is_syscall_success does not handle 32-bit
> > compatible case, which would cause 32-bit negative return code to be
> > recoganized as a positive number later and seen as a "success".
> > 
> > Since is_compat_thread is defined in compat.h, implementing
> > is_syscall_success in ptrace.h would introduce build failure due to
> > recursive inclusion of some basic headers like mutex.h. We put the
> > implementation to ptrace.c
> > 
> > Signed-off-by: He Zhe <zhe.he@windriver.com>
> > ---
> >  arch/arm64/include/asm/ptrace.h |  3 +++
> >  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> > index e58bca832dff..3c415e9e5d85 100644
> > --- a/arch/arm64/include/asm/ptrace.h
> > +++ b/arch/arm64/include/asm/ptrace.h
> > @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> >  	regs->regs[0] = rc;
> >  }
> >  
> > +extern inline int is_syscall_success(struct pt_regs *regs);
> > +#define is_syscall_success(regs) is_syscall_success(regs)
> > +
> >  /**
> >   * regs_get_kernel_argument() - get Nth function argument in kernel
> >   * @regs:	pt_regs of that context
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 170f42fd6101..3266201f8c60 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
> >  	else
> >  		return valid_native_regs(regs);
> >  }
> > +
> > +inline int is_syscall_success(struct pt_regs *regs)
> > +{
> > +	unsigned long val = regs->regs[0];
> > +
> > +	if (is_compat_thread(task_thread_info(current)))
> > +		val = sign_extend64(val, 31);
> > +
> > +	return !IS_ERR_VALUE(val);
> > +}
> 
> It's better to use compat_user_mode(regs) here instead of
> is_compat_thread(). It saves us from worrying whether regs are for the
> current context.
> 
> I think we should change regs_return_value() instead. This function
> seems to be called from several other places and it has the same
> potential problems if called on compat pt_regs.

I think this is a problem we created for ourselves back in commit:

  15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)

AFAICT, the perf regs samples are the only place this matters, since for
ptrace the compat regs are implicitly truncated to compat_ulong_t, and
audit expects the non-truncated return value. Other architectures don't
truncate here, so I think we're setting ourselves up for a game of
whack-a-mole to truncate and extend wherever we need to.

Given that, I suspect it'd be better to do something like the below.

Will, thoughts?

Mark.

---->8----
>From df0f7c160240d9ee6f20f87a180326d3253e80fb Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Fri, 16 Apr 2021 13:58:54 +0100
Subject: [PATCH] arm64: perf: truncate compat regs

For compat userspace, it doesn't generally make sense for the upper 32
bits of GPRs to be set, as these bits don't really exist in AArch32.
However, for structural reasons the kernel may transiently set the upper
32 bits of registers in pt_regs at points where a perf sample can be
taken.

We tried to avoid this happening in commit:

  15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return")

... by having invoke_syscall() truncate the return value for compat
tasks, with helpers in <asm/syscall.h> extending the return value when
required.

Unfortunately this is not complete, as there are other places where we
assign the return value, such as when el0_svc_common() sets up a return
of -ENOSYS.

Further, this approach breaks the audit code, which relies on the upper
32 bits of the return value.

Instead, let's have the perf code explicitly truncate the user regs to
32 bits, and otherwise preserve those within the kernel.

Fixes: 15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/syscall.h | 11 +----------
 arch/arm64/kernel/perf_regs.c    | 26 ++++++++++++++++----------
 arch/arm64/kernel/syscall.c      |  3 ---
 3 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..0ebeaf6dbd45 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -35,9 +35,6 @@ static inline long syscall_get_error(struct task_struct *task,
 {
 	unsigned long error = regs->regs[0];
 
-	if (is_compat_thread(task_thread_info(task)))
-		error = sign_extend64(error, 31);
-
 	return IS_ERR_VALUE(error) ? error : 0;
 }
 
@@ -51,13 +48,7 @@ static inline void syscall_set_return_value(struct task_struct *task,
 					    struct pt_regs *regs,
 					    int error, long val)
 {
-	if (error)
-		val = error;
-
-	if (is_compat_thread(task_thread_info(task)))
-		val = lower_32_bits(val);
-
-	regs->regs[0] = val;
+	regs->regs[0] = (long) error ? error : val;
 }
 
 #define SYSCALL_MAX_ARGS 6
diff --git a/arch/arm64/kernel/perf_regs.c b/arch/arm64/kernel/perf_regs.c
index f6f58e6265df..296f0c55b4e2 100644
--- a/arch/arm64/kernel/perf_regs.c
+++ b/arch/arm64/kernel/perf_regs.c
@@ -9,6 +9,17 @@
 #include <asm/perf_regs.h>
 #include <asm/ptrace.h>
 
+static u64 __perf_reg_value(struct pt_regs *regs, int idx)
+{
+	if ((u32)idx == PERF_REG_ARM64_SP)
+		return regs->sp;
+
+	if ((u32)idx == PERF_REG_ARM64_PC)
+		return regs->pc;
+
+	return regs->regs[idx];
+}
+
 u64 perf_reg_value(struct pt_regs *regs, int idx)
 {
 	if (WARN_ON_ONCE((u32)idx >= PERF_REG_ARM64_MAX))
@@ -38,20 +49,15 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 	 */
 	if (compat_user_mode(regs)) {
 		if ((u32)idx == PERF_REG_ARM64_SP)
-			return regs->compat_sp;
+			return lower_32_bits(regs->compat_sp);
 		if ((u32)idx == PERF_REG_ARM64_LR)
-			return regs->compat_lr;
+			return lower_32_bits(regs->compat_lr);
 		if (idx == 15)
-			return regs->pc;
+			return lower_32_bits(regs->pc);
+		return lower_32_bits(__perf_reg_value(regs, idx));
 	}
 
-	if ((u32)idx == PERF_REG_ARM64_SP)
-		return regs->sp;
-
-	if ((u32)idx == PERF_REG_ARM64_PC)
-		return regs->pc;
-
-	return regs->regs[idx];
+	return __perf_reg_value(regs, idx);
 }
 
 #define REG_RESERVED (~((1ULL << PERF_REG_ARM64_MAX) - 1))
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index b9cf12b271d7..84314fae4b5c 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -51,9 +51,6 @@ 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;
 }
 
-- 
2.11.0

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* RE: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-16 13:34   ` Mark Rutland
@ 2021-04-17 13:19     ` David Laight
  2021-04-19 12:19     ` Will Deacon
  1 sibling, 0 replies; 18+ messages in thread
From: David Laight @ 2021-04-17 13:19 UTC (permalink / raw)
  To: 'Mark Rutland', Catalin Marinas, will
  Cc: He Zhe, oleg, linux-kernel, linux-audit, linux-arm-kernel

From: Mark Rutland
> Sent: 16 April 2021 14:35
..
> @@ -51,13 +48,7 @@ static inline void syscall_set_return_value(struct task_struct *task,
>  					    struct pt_regs *regs,
>  					    int error, long val)
>  {
> -	if (error)
> -		val = error;
> -
> -	if (is_compat_thread(task_thread_info(task)))
> -		val = lower_32_bits(val);
> -
> -	regs->regs[0] = val;
> +	regs->regs[0] = (long) error ? error : val;

	= error ? (long)error : rval;

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-16 13:34   ` Mark Rutland
  2021-04-17 13:19     ` David Laight
@ 2021-04-19 12:19     ` Will Deacon
  2021-04-20  8:54       ` He Zhe
  2021-04-21 17:10       ` Mark Rutland
  1 sibling, 2 replies; 18+ messages in thread
From: Will Deacon @ 2021-04-19 12:19 UTC (permalink / raw)
  To: Mark Rutland
  Cc: He Zhe, Catalin Marinas, oleg, linux-kernel, linux-audit,
	linux-arm-kernel

On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
> On Fri, Apr 16, 2021 at 01:33:22PM +0100, Catalin Marinas wrote:
> > On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
> > > The general version of is_syscall_success does not handle 32-bit
> > > compatible case, which would cause 32-bit negative return code to be
> > > recoganized as a positive number later and seen as a "success".
> > > 
> > > Since is_compat_thread is defined in compat.h, implementing
> > > is_syscall_success in ptrace.h would introduce build failure due to
> > > recursive inclusion of some basic headers like mutex.h. We put the
> > > implementation to ptrace.c
> > > 
> > > Signed-off-by: He Zhe <zhe.he@windriver.com>
> > > ---
> > >  arch/arm64/include/asm/ptrace.h |  3 +++
> > >  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
> > >  2 files changed, 13 insertions(+)
> > > 
> > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> > > index e58bca832dff..3c415e9e5d85 100644
> > > --- a/arch/arm64/include/asm/ptrace.h
> > > +++ b/arch/arm64/include/asm/ptrace.h
> > > @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> > >  	regs->regs[0] = rc;
> > >  }
> > >  
> > > +extern inline int is_syscall_success(struct pt_regs *regs);
> > > +#define is_syscall_success(regs) is_syscall_success(regs)
> > > +
> > >  /**
> > >   * regs_get_kernel_argument() - get Nth function argument in kernel
> > >   * @regs:	pt_regs of that context
> > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > index 170f42fd6101..3266201f8c60 100644
> > > --- a/arch/arm64/kernel/ptrace.c
> > > +++ b/arch/arm64/kernel/ptrace.c
> > > @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
> > >  	else
> > >  		return valid_native_regs(regs);
> > >  }
> > > +
> > > +inline int is_syscall_success(struct pt_regs *regs)
> > > +{
> > > +	unsigned long val = regs->regs[0];
> > > +
> > > +	if (is_compat_thread(task_thread_info(current)))
> > > +		val = sign_extend64(val, 31);
> > > +
> > > +	return !IS_ERR_VALUE(val);
> > > +}
> > 
> > It's better to use compat_user_mode(regs) here instead of
> > is_compat_thread(). It saves us from worrying whether regs are for the
> > current context.
> > 
> > I think we should change regs_return_value() instead. This function
> > seems to be called from several other places and it has the same
> > potential problems if called on compat pt_regs.
> 
> I think this is a problem we created for ourselves back in commit:
> 
>   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
> 
> AFAICT, the perf regs samples are the only place this matters, since for
> ptrace the compat regs are implicitly truncated to compat_ulong_t, and
> audit expects the non-truncated return value. Other architectures don't
> truncate here, so I think we're setting ourselves up for a game of
> whack-a-mole to truncate and extend wherever we need to.
> 
> Given that, I suspect it'd be better to do something like the below.
> 
> Will, thoughts?

I think perf is one example, but this is also visible to userspace via the
native ptrace interface and I distinctly remember needing this for some
versions of arm64 strace to work correctly when tracing compat tasks.

So I do think that clearing the upper bits on the return path is the right
approach, but it sounds like we need some more work to handle syscall(-1)
and audit (what exactly is the problem here after these patches have been
applied?)

Will

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
  2021-04-16  9:43   ` Oleg Nesterov
@ 2021-04-20  8:38     ` He Zhe
  0 siblings, 0 replies; 18+ messages in thread
From: He Zhe @ 2021-04-20  8:38 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: catalin.marinas, linux-kernel, linux-audit, will, linux-arm-kernel



On 4/16/21 5:43 PM, Oleg Nesterov wrote:
> On 04/16, He Zhe wrote:
>> --- a/arch/arm64/include/asm/syscall.h
>> +++ b/arch/arm64/include/asm/syscall.h
>> @@ -44,7 +44,12 @@ static inline long syscall_get_error(struct task_struct *task,
>>  static inline long syscall_get_return_value(struct task_struct *task,
>>  					    struct pt_regs *regs)
>>  {
>> -	return regs->regs[0];
>> +	long val = regs->regs[0];
>> +
>> +	if (is_compat_thread(task_thread_info(task)))
>> +		val = sign_extend64(val, 31);
>> +
>> +	return val;
>>  }
> I can't really review these arm-specific patches, but with this change both
> syscall_get_error() and is_syscall_success() can use syscall_get_return_value()
> to avoid the code duplication.

Thanks. I'll improve this if v2 is needed.

Regards,
Zhe

>
> Oleg.
>

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-16 12:33 ` [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Catalin Marinas
  2021-04-16 13:34   ` Mark Rutland
@ 2021-04-20  8:42   ` He Zhe
  2021-04-21 17:17     ` Mark Rutland
  1 sibling, 1 reply; 18+ messages in thread
From: He Zhe @ 2021-04-20  8:42 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Mark Rutland, oleg, linux-kernel, linux-audit, will, linux-arm-kernel



On 4/16/21 8:33 PM, Catalin Marinas wrote:
> On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
>> The general version of is_syscall_success does not handle 32-bit
>> compatible case, which would cause 32-bit negative return code to be
>> recoganized as a positive number later and seen as a "success".
>>
>> Since is_compat_thread is defined in compat.h, implementing
>> is_syscall_success in ptrace.h would introduce build failure due to
>> recursive inclusion of some basic headers like mutex.h. We put the
>> implementation to ptrace.c
>>
>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>> ---
>>  arch/arm64/include/asm/ptrace.h |  3 +++
>>  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index e58bca832dff..3c415e9e5d85 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>>  	regs->regs[0] = rc;
>>  }
>>  
>> +extern inline int is_syscall_success(struct pt_regs *regs);
>> +#define is_syscall_success(regs) is_syscall_success(regs)
>> +
>>  /**
>>   * regs_get_kernel_argument() - get Nth function argument in kernel
>>   * @regs:	pt_regs of that context
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index 170f42fd6101..3266201f8c60 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
>>  	else
>>  		return valid_native_regs(regs);
>>  }
>> +
>> +inline int is_syscall_success(struct pt_regs *regs)
>> +{
>> +	unsigned long val = regs->regs[0];
>> +
>> +	if (is_compat_thread(task_thread_info(current)))
>> +		val = sign_extend64(val, 31);
>> +
>> +	return !IS_ERR_VALUE(val);
>> +}
> It's better to use compat_user_mode(regs) here instead of
> is_compat_thread(). It saves us from worrying whether regs are for the
> current context.

Thanks. I'll use this for v2.

>
> I think we should change regs_return_value() instead. This function
> seems to be called from several other places and it has the same
> potential problems if called on compat pt_regs.

IMHO, now that we have had specific function, syscall_get_return_value, to get
syscall return code, we might as well use it. regs_return_value may be left for
where we want internal return code. I found such places below and haven't found
other places that syscall sign extension is concerned about.

kernel/test_kprobes.c
kernel/trace/trace_kprobe.c
samples/kprobes/kretprobe_example.c


Regards,
Zhe

>

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-19 12:19     ` Will Deacon
@ 2021-04-20  8:54       ` He Zhe
  2021-04-21 17:10       ` Mark Rutland
  1 sibling, 0 replies; 18+ messages in thread
From: He Zhe @ 2021-04-20  8:54 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland
  Cc: Catalin Marinas, oleg, linux-kernel, linux-audit, linux-arm-kernel



On 4/19/21 8:19 PM, Will Deacon wrote:
> On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
>> On Fri, Apr 16, 2021 at 01:33:22PM +0100, Catalin Marinas wrote:
>>> On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
>>>> The general version of is_syscall_success does not handle 32-bit
>>>> compatible case, which would cause 32-bit negative return code to be
>>>> recoganized as a positive number later and seen as a "success".
>>>>
>>>> Since is_compat_thread is defined in compat.h, implementing
>>>> is_syscall_success in ptrace.h would introduce build failure due to
>>>> recursive inclusion of some basic headers like mutex.h. We put the
>>>> implementation to ptrace.c
>>>>
>>>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>>>> ---
>>>>  arch/arm64/include/asm/ptrace.h |  3 +++
>>>>  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
>>>>  2 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>>>> index e58bca832dff..3c415e9e5d85 100644
>>>> --- a/arch/arm64/include/asm/ptrace.h
>>>> +++ b/arch/arm64/include/asm/ptrace.h
>>>> @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>>>>  	regs->regs[0] = rc;
>>>>  }
>>>>  
>>>> +extern inline int is_syscall_success(struct pt_regs *regs);
>>>> +#define is_syscall_success(regs) is_syscall_success(regs)
>>>> +
>>>>  /**
>>>>   * regs_get_kernel_argument() - get Nth function argument in kernel
>>>>   * @regs:	pt_regs of that context
>>>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>>>> index 170f42fd6101..3266201f8c60 100644
>>>> --- a/arch/arm64/kernel/ptrace.c
>>>> +++ b/arch/arm64/kernel/ptrace.c
>>>> @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
>>>>  	else
>>>>  		return valid_native_regs(regs);
>>>>  }
>>>> +
>>>> +inline int is_syscall_success(struct pt_regs *regs)
>>>> +{
>>>> +	unsigned long val = regs->regs[0];
>>>> +
>>>> +	if (is_compat_thread(task_thread_info(current)))
>>>> +		val = sign_extend64(val, 31);
>>>> +
>>>> +	return !IS_ERR_VALUE(val);
>>>> +}
>>> It's better to use compat_user_mode(regs) here instead of
>>> is_compat_thread(). It saves us from worrying whether regs are for the
>>> current context.
>>>
>>> I think we should change regs_return_value() instead. This function
>>> seems to be called from several other places and it has the same
>>> potential problems if called on compat pt_regs.
>> I think this is a problem we created for ourselves back in commit:
>>
>>   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
>>
>> AFAICT, the perf regs samples are the only place this matters, since for
>> ptrace the compat regs are implicitly truncated to compat_ulong_t, and
>> audit expects the non-truncated return value. Other architectures don't
>> truncate here, so I think we're setting ourselves up for a game of
>> whack-a-mole to truncate and extend wherever we need to.
>>
>> Given that, I suspect it'd be better to do something like the below.
>>
>> Will, thoughts?
> I think perf is one example, but this is also visible to userspace via the
> native ptrace interface and I distinctly remember needing this for some
> versions of arm64 strace to work correctly when tracing compat tasks.
>
> So I do think that clearing the upper bits on the return path is the right
> approach, but it sounds like we need some more work to handle syscall(-1)
> and audit (what exactly is the problem here after these patches have been
> applied?)

IIUC, IS_ERR_VALUE could handle -1, did I miss something? Thanks.

Regards,
Zhe

>
> Will

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-19 12:19     ` Will Deacon
  2021-04-20  8:54       ` He Zhe
@ 2021-04-21 17:10       ` Mark Rutland
  2021-04-22 16:07         ` Will Deacon
  1 sibling, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2021-04-21 17:10 UTC (permalink / raw)
  To: Will Deacon
  Cc: He Zhe, Catalin Marinas, oleg, linux-kernel, linux-audit,
	linux-arm-kernel

On Mon, Apr 19, 2021 at 01:19:33PM +0100, Will Deacon wrote:
> On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
> > I think this is a problem we created for ourselves back in commit:
> > 
> >   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
> > 
> > AFAICT, the perf regs samples are the only place this matters, since for
> > ptrace the compat regs are implicitly truncated to compat_ulong_t, and
> > audit expects the non-truncated return value. Other architectures don't
> > truncate here, so I think we're setting ourselves up for a game of
> > whack-a-mole to truncate and extend wherever we need to.
> > 
> > Given that, I suspect it'd be better to do something like the below.
> > 
> > Will, thoughts?
> 
> I think perf is one example, but this is also visible to userspace via the
> native ptrace interface and I distinctly remember needing this for some
> versions of arm64 strace to work correctly when tracing compat tasks.

FWIW, you've convinced me on your approach (more on that below), but
when I went digging here this didn't seem to be exposed via ptrace --
for any task tracing a compat task, the GPRs are exposed via
compat_gpr_{get,set}(), which always truncate to compat_ulong_t, giving
the lower 32 bits. See task_user_regset_view() for where we get the
regset.

Am I missing something, or are you thinking of another issue you fixed
at the same time?

> So I do think that clearing the upper bits on the return path is the right
> approach, but it sounds like we need some more work to handle syscall(-1)
> and audit (what exactly is the problem here after these patches have been
> applied?)

>From digging a bit more, I think I agree, and I think these patches are
sufficient for audit. I have some comments I'll leave separately.

The remaining issues are wherever we assign a signed value to a compat
GPR without explicit truncation. That'll leak via perf sampling the user
regs, but I haven't managed to convince myself whether that causes any
functional change in behaviour for audit, seccomp, or syscall tracing.

Since we mostly use compat_ulong_t for intermediate values in compat
code, it does look like this is only an issue for x0 where we assign an
error value, e.g. the -ENOSYS case in el0_svc_common. I'll go see if I
can find any more.

With those fixed up we can remove the x0 truncation from entry.S,
which'd be nice too.

Thanks,
Mark.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-20  8:42   ` He Zhe
@ 2021-04-21 17:17     ` Mark Rutland
  0 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2021-04-21 17:17 UTC (permalink / raw)
  To: He Zhe
  Cc: Catalin Marinas, oleg, linux-kernel, linux-audit, will, linux-arm-kernel

On Tue, Apr 20, 2021 at 04:42:53PM +0800, He Zhe wrote:
> On 4/16/21 8:33 PM, Catalin Marinas wrote:
> > On Fri, Apr 16, 2021 at 03:55:31PM +0800, He Zhe wrote:
> >> The general version of is_syscall_success does not handle 32-bit
> >> compatible case, which would cause 32-bit negative return code to be
> >> recoganized as a positive number later and seen as a "success".
> >>
> >> Since is_compat_thread is defined in compat.h, implementing
> >> is_syscall_success in ptrace.h would introduce build failure due to
> >> recursive inclusion of some basic headers like mutex.h. We put the
> >> implementation to ptrace.c
> >>
> >> Signed-off-by: He Zhe <zhe.he@windriver.com>
> >> ---
> >>  arch/arm64/include/asm/ptrace.h |  3 +++
> >>  arch/arm64/kernel/ptrace.c      | 10 ++++++++++
> >>  2 files changed, 13 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> >> index e58bca832dff..3c415e9e5d85 100644
> >> --- a/arch/arm64/include/asm/ptrace.h
> >> +++ b/arch/arm64/include/asm/ptrace.h
> >> @@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> >>  	regs->regs[0] = rc;
> >>  }
> >>  
> >> +extern inline int is_syscall_success(struct pt_regs *regs);
> >> +#define is_syscall_success(regs) is_syscall_success(regs)
> >> +
> >>  /**
> >>   * regs_get_kernel_argument() - get Nth function argument in kernel
> >>   * @regs:	pt_regs of that context
> >> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> >> index 170f42fd6101..3266201f8c60 100644
> >> --- a/arch/arm64/kernel/ptrace.c
> >> +++ b/arch/arm64/kernel/ptrace.c
> >> @@ -1909,3 +1909,13 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
> >>  	else
> >>  		return valid_native_regs(regs);
> >>  }
> >> +
> >> +inline int is_syscall_success(struct pt_regs *regs)
> >> +{
> >> +	unsigned long val = regs->regs[0];
> >> +
> >> +	if (is_compat_thread(task_thread_info(current)))
> >> +		val = sign_extend64(val, 31);
> >> +
> >> +	return !IS_ERR_VALUE(val);
> >> +}
> > It's better to use compat_user_mode(regs) here instead of
> > is_compat_thread(). It saves us from worrying whether regs are for the
> > current context.
> 
> Thanks. I'll use this for v2.
> 
> >
> > I think we should change regs_return_value() instead. This function
> > seems to be called from several other places and it has the same
> > potential problems if called on compat pt_regs.
> 
> IMHO, now that we have had specific function, syscall_get_return_value, to get
> syscall return code, we might as well use it. regs_return_value may be left for
> where we want internal return code. I found such places below and haven't found
> other places that syscall sign extension is concerned about.
> 
> kernel/test_kprobes.c
> kernel/trace/trace_kprobe.c
> samples/kprobes/kretprobe_example.c

FWIW, I agree that we should use syscall_get_return_value(). If we make
the common implementation of is_syscall_success() use
syscall_get_return_value(), we shouldn't need to write our own
implementation, so I'd prefer if we could do that if possible.

IIUC regs_get_return_value() was originally meant to be used for kernel
regs, and is trying to do something quite different, so having the core
code use syscall_get_return_value() makes sense to allow architectures
to handle those cases differently.

Thanks,
Mark.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
  2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
  2021-04-16  9:43   ` Oleg Nesterov
@ 2021-04-21 17:41   ` Mark Rutland
  2021-04-22 16:55     ` Mark Rutland
  1 sibling, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2021-04-21 17:41 UTC (permalink / raw)
  To: He Zhe
  Cc: catalin.marinas, oleg, linux-kernel, linux-audit, will, linux-arm-kernel

On Fri, Apr 16, 2021 at 03:55:32PM +0800, He Zhe wrote:
> Add sign extension handling in syscall_get_return_value so that it can
> handle 32-bit compatible case and can be used by for example audit, just
> like what syscall_get_error does.

If a compat syscall can ever legitimately return a non-error value with
bit 31 set, and this sign-extends it, is that ever going to reach
userspace as a 64-bit value?

IIUC things like mmap() can return pointers above 2GiB for a compat
task, so I'm a bit uneasy that we'd handle those wrong. I can't see a
way of preventing that unless we keep the upper 32 bits for errors.

Mark.

> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  arch/arm64/include/asm/syscall.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
> index cfc0672013f6..cd7a22787aeb 100644
> --- a/arch/arm64/include/asm/syscall.h
> +++ b/arch/arm64/include/asm/syscall.h
> @@ -44,7 +44,12 @@ static inline long syscall_get_error(struct task_struct *task,
>  static inline long syscall_get_return_value(struct task_struct *task,
>  					    struct pt_regs *regs)
>  {
> -	return regs->regs[0];
> +	long val = regs->regs[0];
> +
> +	if (is_compat_thread(task_thread_info(task)))
> +		val = sign_extend64(val, 31);
> +
> +	return val;
>  }
>  
>  static inline void syscall_set_return_value(struct task_struct *task,
> -- 
> 2.17.1
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-21 17:10       ` Mark Rutland
@ 2021-04-22 16:07         ` Will Deacon
  2021-04-22 16:42           ` Mark Rutland
  0 siblings, 1 reply; 18+ messages in thread
From: Will Deacon @ 2021-04-22 16:07 UTC (permalink / raw)
  To: Mark Rutland
  Cc: He Zhe, Catalin Marinas, oleg, linux-kernel, linux-audit,
	linux-arm-kernel

Hi Mark,

On Wed, Apr 21, 2021 at 06:10:05PM +0100, Mark Rutland wrote:
> On Mon, Apr 19, 2021 at 01:19:33PM +0100, Will Deacon wrote:
> > On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
> > > I think this is a problem we created for ourselves back in commit:
> > > 
> > >   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
> > > 
> > > AFAICT, the perf regs samples are the only place this matters, since for
> > > ptrace the compat regs are implicitly truncated to compat_ulong_t, and
> > > audit expects the non-truncated return value. Other architectures don't
> > > truncate here, so I think we're setting ourselves up for a game of
> > > whack-a-mole to truncate and extend wherever we need to.
> > > 
> > > Given that, I suspect it'd be better to do something like the below.
> > > 
> > > Will, thoughts?
> > 
> > I think perf is one example, but this is also visible to userspace via the
> > native ptrace interface and I distinctly remember needing this for some
> > versions of arm64 strace to work correctly when tracing compat tasks.
> 
> FWIW, you've convinced me on your approach (more on that below), but
> when I went digging here this didn't seem to be exposed via ptrace --
> for any task tracing a compat task, the GPRs are exposed via
> compat_gpr_{get,set}(), which always truncate to compat_ulong_t, giving
> the lower 32 bits. See task_user_regset_view() for where we get the
> regset.
> 
> Am I missing something, or are you thinking of another issue you fixed
> at the same time?

I think it may depend on whether strace pokes at the GPRs or instead issues
a PTRACE_GET_SYSCALL_INFO request but I've forgotten the details,
unfortunately. I do remember seeing an issue though, and it was only last
year.

> > So I do think that clearing the upper bits on the return path is the right
> > approach, but it sounds like we need some more work to handle syscall(-1)
> > and audit (what exactly is the problem here after these patches have been
> > applied?)
> 
> From digging a bit more, I think I agree, and I think these patches are
> sufficient for audit. I have some comments I'll leave separately.
> 
> The remaining issues are wherever we assign a signed value to a compat
> GPR without explicit truncation. That'll leak via perf sampling the user
> regs, but I haven't managed to convince myself whether that causes any
> functional change in behaviour for audit, seccomp, or syscall tracing.
> 
> Since we mostly use compat_ulong_t for intermediate values in compat
> code, it does look like this is only an issue for x0 where we assign an
> error value, e.g. the -ENOSYS case in el0_svc_common. I'll go see if I
> can find any more.
> 
> With those fixed up we can remove the x0 truncation from entry.S,
> which'd be nice too.

If we remove that then we should probably have a (debug?) check on the
return-to-user path just to make sure.

Will

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-22 16:07         ` Will Deacon
@ 2021-04-22 16:42           ` Mark Rutland
  2021-04-22 18:57             ` Dmitry V. Levin
  0 siblings, 1 reply; 18+ messages in thread
From: Mark Rutland @ 2021-04-22 16:42 UTC (permalink / raw)
  To: Will Deacon
  Cc: He Zhe, Catalin Marinas, oleg, linux-kernel, linux-audit,
	linux-arm-kernel

On Thu, Apr 22, 2021 at 05:07:53PM +0100, Will Deacon wrote:
> On Wed, Apr 21, 2021 at 06:10:05PM +0100, Mark Rutland wrote:
> > On Mon, Apr 19, 2021 at 01:19:33PM +0100, Will Deacon wrote:
> > > On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
> > > > I think this is a problem we created for ourselves back in commit:
> > > > 
> > > >   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
> > > > 
> > > > AFAICT, the perf regs samples are the only place this matters, since for
> > > > ptrace the compat regs are implicitly truncated to compat_ulong_t, and
> > > > audit expects the non-truncated return value. Other architectures don't
> > > > truncate here, so I think we're setting ourselves up for a game of
> > > > whack-a-mole to truncate and extend wherever we need to.
> > > > 
> > > > Given that, I suspect it'd be better to do something like the below.
> > > > 
> > > > Will, thoughts?
> > > 
> > > I think perf is one example, but this is also visible to userspace via the
> > > native ptrace interface and I distinctly remember needing this for some
> > > versions of arm64 strace to work correctly when tracing compat tasks.
> > 
> > FWIW, you've convinced me on your approach (more on that below), but
> > when I went digging here this didn't seem to be exposed via ptrace --
> > for any task tracing a compat task, the GPRs are exposed via
> > compat_gpr_{get,set}(), which always truncate to compat_ulong_t, giving
> > the lower 32 bits. See task_user_regset_view() for where we get the
> > regset.
> > 
> > Am I missing something, or are you thinking of another issue you fixed
> > at the same time?
> 
> I think it may depend on whether strace pokes at the GPRs or instead issues
> a PTRACE_GET_SYSCALL_INFO request but I've forgotten the details,
> unfortunately. I do remember seeing an issue though, and it was only last
> year.

Ah; I hadn't spotted PTRACE_GET_SYSCALL_INFO, thanks for the pointer. I
see that gets at the regs via syscall_get_arguments(), which doesn't
truncate them.

That makes me wonder whether x86 and others do the right thing here...

> > > So I do think that clearing the upper bits on the return path is the right
> > > approach, but it sounds like we need some more work to handle syscall(-1)
> > > and audit (what exactly is the problem here after these patches have been
> > > applied?)
> > 
> > From digging a bit more, I think I agree, and I think these patches are
> > sufficient for audit. I have some comments I'll leave separately.
> > 
> > The remaining issues are wherever we assign a signed value to a compat
> > GPR without explicit truncation. That'll leak via perf sampling the user
> > regs, but I haven't managed to convince myself whether that causes any
> > functional change in behaviour for audit, seccomp, or syscall tracing.
> > 
> > Since we mostly use compat_ulong_t for intermediate values in compat
> > code, it does look like this is only an issue for x0 where we assign an
> > error value, e.g. the -ENOSYS case in el0_svc_common. I'll go see if I
> > can find any more.
> > 
> > With those fixed up we can remove the x0 truncation from entry.S,
> > which'd be nice too.
> 
> If we remove that then we should probably have a (debug?) check on the
> return-to-user path just to make sure.

I've hacked that up locally; I'd certainly like to run that along with
some other sanity checks (e.g. PSTATE) while fuzzing, but agree that's
probably not something to enable in any defconfig build.

I'm happy to add that later as and when I remove the bit from entry.S.

Thanks,
Mark.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
  2021-04-21 17:41   ` Mark Rutland
@ 2021-04-22 16:55     ` Mark Rutland
  0 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2021-04-22 16:55 UTC (permalink / raw)
  To: He Zhe
  Cc: catalin.marinas, oleg, linux-kernel, linux-audit, will, linux-arm-kernel

On Wed, Apr 21, 2021 at 06:41:05PM +0100, Mark Rutland wrote:
> On Fri, Apr 16, 2021 at 03:55:32PM +0800, He Zhe wrote:
> > Add sign extension handling in syscall_get_return_value so that it can
> > handle 32-bit compatible case and can be used by for example audit, just
> > like what syscall_get_error does.
> 
> If a compat syscall can ever legitimately return a non-error value with
> bit 31 set, and this sign-extends it, is that ever going to reach
> userspace as a 64-bit value?
> 
> IIUC things like mmap() can return pointers above 2GiB for a compat
> task, so I'm a bit uneasy that we'd handle those wrong. I can't see a
> way of preventing that unless we keep the upper 32 bits for errors.

Looking at this with fresh eyes, I think we can more closely mirror
syscall_get_error(), and do something like:

static inline long syscall_get_return_value(struct task_struct *task,
					    struct pt_regs *regs)
{
	long val = regs->regs[0];
	long error = val;

	if (is_compat_thread(task_thread_info(task)))
		error = sign_extend64(error, 31);
	
	return IS_ERR_VALUE(error) ? error : val;
}

Thanks,
Mark.

> 
> Mark.
> 
> > 
> > Signed-off-by: He Zhe <zhe.he@windriver.com>
> > ---
> >  arch/arm64/include/asm/syscall.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
> > index cfc0672013f6..cd7a22787aeb 100644
> > --- a/arch/arm64/include/asm/syscall.h
> > +++ b/arch/arm64/include/asm/syscall.h
> > @@ -44,7 +44,12 @@ static inline long syscall_get_error(struct task_struct *task,
> >  static inline long syscall_get_return_value(struct task_struct *task,
> >  					    struct pt_regs *regs)
> >  {
> > -	return regs->regs[0];
> > +	long val = regs->regs[0];
> > +
> > +	if (is_compat_thread(task_thread_info(task)))
> > +		val = sign_extend64(val, 31);
> > +
> > +	return val;
> >  }
> >  
> >  static inline void syscall_set_return_value(struct task_struct *task,
> > -- 
> > 2.17.1
> > 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

* Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
  2021-04-22 16:42           ` Mark Rutland
@ 2021-04-22 18:57             ` Dmitry V. Levin
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2021-04-22 18:57 UTC (permalink / raw)
  To: Mark Rutland
  Cc: He Zhe, Catalin Marinas, oleg, linux-kernel, linux-audit,
	Will Deacon, linux-arm-kernel

On Thu, Apr 22, 2021 at 05:42:28PM +0100, Mark Rutland wrote:
> On Thu, Apr 22, 2021 at 05:07:53PM +0100, Will Deacon wrote:
> > On Wed, Apr 21, 2021 at 06:10:05PM +0100, Mark Rutland wrote:
> > > On Mon, Apr 19, 2021 at 01:19:33PM +0100, Will Deacon wrote:
> > > > On Fri, Apr 16, 2021 at 02:34:41PM +0100, Mark Rutland wrote:
> > > > > I think this is a problem we created for ourselves back in commit:
> > > > > 
> > > > >   15956689a0e60aa0 ("arm64: compat: Ensure upper 32 bits of x0 are zero on syscall return)
> > > > > 
> > > > > AFAICT, the perf regs samples are the only place this matters, since for
> > > > > ptrace the compat regs are implicitly truncated to compat_ulong_t, and
> > > > > audit expects the non-truncated return value. Other architectures don't
> > > > > truncate here, so I think we're setting ourselves up for a game of
> > > > > whack-a-mole to truncate and extend wherever we need to.
> > > > > 
> > > > > Given that, I suspect it'd be better to do something like the below.
> > > > > 
> > > > > Will, thoughts?
> > > > 
> > > > I think perf is one example, but this is also visible to userspace via the
> > > > native ptrace interface and I distinctly remember needing this for some
> > > > versions of arm64 strace to work correctly when tracing compat tasks.
> > > 
> > > FWIW, you've convinced me on your approach (more on that below), but
> > > when I went digging here this didn't seem to be exposed via ptrace --
> > > for any task tracing a compat task, the GPRs are exposed via
> > > compat_gpr_{get,set}(), which always truncate to compat_ulong_t, giving
> > > the lower 32 bits. See task_user_regset_view() for where we get the
> > > regset.
> > > 
> > > Am I missing something, or are you thinking of another issue you fixed
> > > at the same time?
> > 
> > I think it may depend on whether strace pokes at the GPRs or instead issues
> > a PTRACE_GET_SYSCALL_INFO request but I've forgotten the details,
> > unfortunately. I do remember seeing an issue though, and it was only last
> > year.
> 
> Ah; I hadn't spotted PTRACE_GET_SYSCALL_INFO, thanks for the pointer. I
> see that gets at the regs via syscall_get_arguments(), which doesn't
> truncate them.
> 
> That makes me wonder whether x86 and others do the right thing here...

Yes, some architectures had to be fixed, but they mostly do the right
thing nowadays.

Feel free to use tools/testing/selftests/ptrace/get_syscall_info.c for
testing, or indeed use strace test suite.


-- 
ldv

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


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

end of thread, other threads:[~2021-04-22 19:07 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-16  7:55 [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe
2021-04-16  7:55 ` [PATCH 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe
2021-04-16  9:43   ` Oleg Nesterov
2021-04-20  8:38     ` He Zhe
2021-04-21 17:41   ` Mark Rutland
2021-04-22 16:55     ` Mark Rutland
2021-04-16  7:55 ` [PATCH 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe
2021-04-16 12:33 ` [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat Catalin Marinas
2021-04-16 13:34   ` Mark Rutland
2021-04-17 13:19     ` David Laight
2021-04-19 12:19     ` Will Deacon
2021-04-20  8:54       ` He Zhe
2021-04-21 17:10       ` Mark Rutland
2021-04-22 16:07         ` Will Deacon
2021-04-22 16:42           ` Mark Rutland
2021-04-22 18:57             ` Dmitry V. Levin
2021-04-20  8:42   ` He Zhe
2021-04-21 17:17     ` Mark Rutland

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