* [PATCH 0/3] arm64: Coredump fixes
@ 2017-06-21 14:55 Dave Martin
2017-06-21 15:00 ` [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps Dave Martin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dave Martin @ 2017-06-21 14:55 UTC (permalink / raw)
To: linux-arm-kernel
We have a few bugs affecting coredumps on arm64, namely:
* VFP regs missing for compat
* dumped FPSIMD/VFP regs and user RW TLS reg may be stale
It appears that these bugs were all introduced in the original arm64
ptrace implementation in v3.6: 478fcb2cdb23 ("arm64: Debugging
support").
Judging by the amount of noise people have(n't) been making about these,
they're not likely to be critical for anyone, but the patches apply back
to v4.9 without conflicts so it may be worth Cc-ing stable for those.
I've not done that yet to avoid the patches being pulled prematurely.
I have simple backports back to v3.6 if anyone cares, but I won't post
them unless someone shouts -- they may just cause unnecessary churn.
Basic testing has been done, on v4.12-rc* only, with a simple userspace
test and manual parsing of the resulting coredumps. Other revisions
have only been build-tested.
I haven't checked for similar bugs in arch/arm. Casual inspection
suggests that the required flushing is already in place there.
Dave Martin (3):
arm64: compat: Fix VFP register dumping in coredumps
arm64: ptrace: Flush FPSIMD regs back to thread_struct before reading
arm64: ptrace: Flush user-RW TLS reg to thread_struct before reading
arch/arm64/include/asm/processor.h | 3 +++
arch/arm64/kernel/process.c | 8 ++++++--
arch/arm64/kernel/ptrace.c | 22 ++++++++++++++++++----
3 files changed, 27 insertions(+), 6 deletions(-)
--
2.1.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps
2017-06-21 14:55 [PATCH 0/3] arm64: Coredump fixes Dave Martin
@ 2017-06-21 15:00 ` Dave Martin
2017-06-21 15:23 ` Mark Rutland
2017-06-21 15:00 ` [PATCH 2/3] arm64: ptrace: Flush FPSIMD regs back to thread_struct before reading Dave Martin
2017-06-21 15:00 ` [PATCH 3/3] arm64: ptrace: Flush user-RW TLS reg " Dave Martin
2 siblings, 1 reply; 6+ messages in thread
From: Dave Martin @ 2017-06-21 15:00 UTC (permalink / raw)
To: linux-arm-kernel
Currently, VFP registers are omitted from coredumps for compat
processes, due to a bug in the REGSET_COMPAT_VFP regset
implementation.
compat_vfp_get() needs to transfer non-contiguous data from
thread_struct.fpsimd_state, and uses put_user() to handle the
offending trailing word (FPSCR). This fails when copying to a
kernel address (i.e., kbuf && !ubuf), which is what happens when
dumping core. As a result, the ELF coredump core code silently
omits the NT_ARM_VFP note from the dump.
It would be possible to work around this with additional special
case code for the put_user(), but since user_regset_copyout() is
explcltly designed to handle this scenario it is cleaner to port
the put_user() to a user_regset_copyout() call, which this patch
does.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/kernel/ptrace.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index c142459..0e5aaec 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -894,7 +894,7 @@ static int compat_vfp_get(struct task_struct *target,
{
struct user_fpsimd_state *uregs;
compat_ulong_t fpscr;
- int ret;
+ int ret, vregs_end_pos;
uregs = &target->thread.fpsimd_state.user_fpsimd;
@@ -902,13 +902,16 @@ static int compat_vfp_get(struct task_struct *target,
* The VFP registers are packed into the fpsimd_state, so they all sit
* nicely together for us. We just need to create the fpscr separately.
*/
- ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs, 0,
- VFP_STATE_SIZE - sizeof(compat_ulong_t));
+ vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
+ 0, vregs_end_pos);
if (count && !ret) {
fpscr = (uregs->fpsr & VFP_FPSCR_STAT_MASK) |
(uregs->fpcr & VFP_FPSCR_CTRL_MASK);
- ret = put_user(fpscr, (compat_ulong_t *)ubuf);
+
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fpscr,
+ vregs_end_pos, VFP_STATE_SIZE);
}
return ret;
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] arm64: ptrace: Flush FPSIMD regs back to thread_struct before reading
2017-06-21 14:55 [PATCH 0/3] arm64: Coredump fixes Dave Martin
2017-06-21 15:00 ` [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps Dave Martin
@ 2017-06-21 15:00 ` Dave Martin
2017-06-21 15:00 ` [PATCH 3/3] arm64: ptrace: Flush user-RW TLS reg " Dave Martin
2 siblings, 0 replies; 6+ messages in thread
From: Dave Martin @ 2017-06-21 15:00 UTC (permalink / raw)
To: linux-arm-kernel
When reading the FPSIMD state of current (which occurs when dumping
core), it is possible that userspace has modified the FPSIMD
registers since the time the task was last scheduled out. Such
changes are not guaranteed to be reflected immedately in
thread_struct.
As a result, a coredump can contain stale values for these
registers. Reading the registers of a stopped task via ptrace is
unaffected.
This patch explicitly flushes the CPU state back to thread_struct
before dumping when operating on current, thus ensuring that
coredump contents are up to date.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/kernel/ptrace.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 0e5aaec..eeef01a 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -623,6 +623,10 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
{
struct user_fpsimd_state *uregs;
uregs = &target->thread.fpsimd_state.user_fpsimd;
+
+ if (target == current)
+ fpsimd_preserve_current_state();
+
return user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs, 0, -1);
}
@@ -898,6 +902,9 @@ static int compat_vfp_get(struct task_struct *target,
uregs = &target->thread.fpsimd_state.user_fpsimd;
+ if (target == current)
+ fpsimd_preserve_current_state();
+
/*
* The VFP registers are packed into the fpsimd_state, so they all sit
* nicely together for us. We just need to create the fpscr separately.
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] arm64: ptrace: Flush user-RW TLS reg to thread_struct before reading
2017-06-21 14:55 [PATCH 0/3] arm64: Coredump fixes Dave Martin
2017-06-21 15:00 ` [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps Dave Martin
2017-06-21 15:00 ` [PATCH 2/3] arm64: ptrace: Flush FPSIMD regs back to thread_struct before reading Dave Martin
@ 2017-06-21 15:00 ` Dave Martin
2 siblings, 0 replies; 6+ messages in thread
From: Dave Martin @ 2017-06-21 15:00 UTC (permalink / raw)
To: linux-arm-kernel
When reading current's user-writable TLS register (which occurs
when dumping core for native tasks), it is possible that userspace
has modified it since the time the task was last scheduled out.
The new TLS register value is not guaranteed to have been written
immediately back to thread_struct in this case.
As a result, a coredump can capture stale data for this register.
Reading the register for a stopped task via ptrace is unaffected.
For native tasks, this patch explicitly flushes the TPIDR_EL0
register back to thread_struct before dumping when operating on
current, thus ensuring that coredump contents are up to date. For
compat tasks, the TLS register is not user-writable and so cannot
be out of sync, so no flush is required in compat_tls_get().
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/include/asm/processor.h | 3 +++
arch/arm64/kernel/process.c | 8 ++++++--
arch/arm64/kernel/ptrace.c | 4 ++++
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 9428b93..64c9e78 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -104,6 +104,9 @@ struct thread_struct {
#define task_user_tls(t) (&(t)->thread.tp_value)
#endif
+/* Sync TPIDR_EL0 back to thread_struct for current */
+void tls_preserve_current_state(void);
+
#define INIT_THREAD { }
static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index ae2a835..0ac0159 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -297,12 +297,16 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
return 0;
}
+void tls_preserve_current_state(void)
+{
+ *task_user_tls(current) = read_sysreg(tpidr_el0);
+}
+
static void tls_thread_switch(struct task_struct *next)
{
unsigned long tpidr, tpidrro;
- tpidr = read_sysreg(tpidr_el0);
- *task_user_tls(current) = tpidr;
+ tls_preserve_current_state();
tpidr = *task_user_tls(next);
tpidrro = is_compat_thread(task_thread_info(next)) ?
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index eeef01a..35846f1 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -652,6 +652,10 @@ static int tls_get(struct task_struct *target, const struct user_regset *regset,
void *kbuf, void __user *ubuf)
{
unsigned long *tls = &target->thread.tp_value;
+
+ if (target == current)
+ tls_preserve_current_state();
+
return user_regset_copyout(&pos, &count, &kbuf, &ubuf, tls, 0, -1);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps
2017-06-21 15:00 ` [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps Dave Martin
@ 2017-06-21 15:23 ` Mark Rutland
2017-06-21 16:05 ` Dave Martin
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2017-06-21 15:23 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 04:00:42PM +0100, Dave Martin wrote:
> Currently, VFP registers are omitted from coredumps for compat
> processes, due to a bug in the REGSET_COMPAT_VFP regset
> implementation.
>
> compat_vfp_get() needs to transfer non-contiguous data from
> thread_struct.fpsimd_state, and uses put_user() to handle the
> offending trailing word (FPSCR). This fails when copying to a
> kernel address (i.e., kbuf && !ubuf), which is what happens when
> dumping core. As a result, the ELF coredump core code silently
> omits the NT_ARM_VFP note from the dump.
>
> It would be possible to work around this with additional special
> case code for the put_user(), but since user_regset_copyout() is
> explcltly designed to handle this scenario it is cleaner to port
Nit: explicitly
> the put_user() to a user_regset_copyout() call, which this patch
> does.
Given, 32-bit arm also uses user_regset_copyout(), it seems like the
all-round right thing to do.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> arch/arm64/kernel/ptrace.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index c142459..0e5aaec 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -894,7 +894,7 @@ static int compat_vfp_get(struct task_struct *target,
> {
> struct user_fpsimd_state *uregs;
> compat_ulong_t fpscr;
> - int ret;
> + int ret, vregs_end_pos;
>
> uregs = &target->thread.fpsimd_state.user_fpsimd;
>
> @@ -902,13 +902,16 @@ static int compat_vfp_get(struct task_struct *target,
> * The VFP registers are packed into the fpsimd_state, so they all sit
> * nicely together for us. We just need to create the fpscr separately.
> */
> - ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs, 0,
> - VFP_STATE_SIZE - sizeof(compat_ulong_t));
> + vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
> + 0, vregs_end_pos);
>
> if (count && !ret) {
> fpscr = (uregs->fpsr & VFP_FPSCR_STAT_MASK) |
> (uregs->fpcr & VFP_FPSCR_CTRL_MASK);
> - ret = put_user(fpscr, (compat_ulong_t *)ubuf);
> +
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fpscr,
> + vregs_end_pos, VFP_STATE_SIZE);
> }
It's a shame compat_user_vfp is defined in signal32.c, otherwise we
could've used offsetof(struct compat-user_vfp, fpscr) here (and also for
the fpregs), mirroring the structure of 32-bit's vfp_get().
Otherwise, this looks sane to me.
Thanks,
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps
2017-06-21 15:23 ` Mark Rutland
@ 2017-06-21 16:05 ` Dave Martin
0 siblings, 0 replies; 6+ messages in thread
From: Dave Martin @ 2017-06-21 16:05 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 04:23:20PM +0100, Mark Rutland wrote:
> On Wed, Jun 21, 2017 at 04:00:42PM +0100, Dave Martin wrote:
> > Currently, VFP registers are omitted from coredumps for compat
> > processes, due to a bug in the REGSET_COMPAT_VFP regset
> > implementation.
> >
> > compat_vfp_get() needs to transfer non-contiguous data from
> > thread_struct.fpsimd_state, and uses put_user() to handle the
> > offending trailing word (FPSCR). This fails when copying to a
> > kernel address (i.e., kbuf && !ubuf), which is what happens when
> > dumping core. As a result, the ELF coredump core code silently
> > omits the NT_ARM_VFP note from the dump.
> >
> > It would be possible to work around this with additional special
> > case code for the put_user(), but since user_regset_copyout() is
> > explcltly designed to handle this scenario it is cleaner to port
>
> Nit: explicitly
Fixed for respin (if there is one).
> > the put_user() to a user_regset_copyout() call, which this patch
> > does.
>
> Given, 32-bit arm also uses user_regset_copyout(), it seems like the
> all-round right thing to do.
Agreed. There may be cases where user_regset_copyout() doesn't cut it,
but it seems OK here.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> > arch/arm64/kernel/ptrace.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index c142459..0e5aaec 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -894,7 +894,7 @@ static int compat_vfp_get(struct task_struct *target,
> > {
> > struct user_fpsimd_state *uregs;
> > compat_ulong_t fpscr;
> > - int ret;
> > + int ret, vregs_end_pos;
> >
> > uregs = &target->thread.fpsimd_state.user_fpsimd;
> >
> > @@ -902,13 +902,16 @@ static int compat_vfp_get(struct task_struct *target,
> > * The VFP registers are packed into the fpsimd_state, so they all sit
> > * nicely together for us. We just need to create the fpscr separately.
> > */
> > - ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs, 0,
> > - VFP_STATE_SIZE - sizeof(compat_ulong_t));
> > + vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
> > + 0, vregs_end_pos);
> >
> > if (count && !ret) {
> > fpscr = (uregs->fpsr & VFP_FPSCR_STAT_MASK) |
> > (uregs->fpcr & VFP_FPSCR_CTRL_MASK);
> > - ret = put_user(fpscr, (compat_ulong_t *)ubuf);
> > +
> > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fpscr,
> > + vregs_end_pos, VFP_STATE_SIZE);
> > }
>
> It's a shame compat_user_vfp is defined in signal32.c, otherwise we
> could've used offsetof(struct compat-user_vfp, fpscr) here (and also for
> the fpregs), mirroring the structure of 32-bit's vfp_get().
Could be nicer -- I was trying to make the minimum change here.
> Otherwise, this looks sane to me.
Thanks for looking it over.
Cheers
---Dave
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-21 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-21 14:55 [PATCH 0/3] arm64: Coredump fixes Dave Martin
2017-06-21 15:00 ` [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps Dave Martin
2017-06-21 15:23 ` Mark Rutland
2017-06-21 16:05 ` Dave Martin
2017-06-21 15:00 ` [PATCH 2/3] arm64: ptrace: Flush FPSIMD regs back to thread_struct before reading Dave Martin
2017-06-21 15:00 ` [PATCH 3/3] arm64: ptrace: Flush user-RW TLS reg " Dave Martin
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).