Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
---
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
Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
>> ---
>> 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
>
Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
> > > ---
> > > 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
RE: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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)
Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
> > ---
> > 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
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 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
Cc: Will Deacon
---
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
Re: [PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
> ---
> 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
[PATCH 1/3] arm64: ptrace: Add is_syscall_success to handle compat
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
---
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
