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