| Message ID | 20161014151336.31418-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote: > This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0. > Specifically a movs pc,lr in the kernels ret_fast_syscall returning to > some thumb mode user space code but store_reg unconditionally aligned > the return PC instead of treating the return as an "interworking" > branch. > > I suspect we need to audit all calls to store_reg that might involve the > PC to ensure "interworking" branches are correctly handled. Also I'm not > quite sure how the code worked before 9b6a3e as the store_reg path > wouldn't have triggered the store_cpu_field(var, thumb) to set the > processor mode back to thumb. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> I think this is the wrong fix to the problem -- see the patch I sent a few days back. thanks -- PMM
On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote: > I suspect we need to audit all calls to store_reg that might involve the > PC to ensure "interworking" branches are correctly handled. Also I'm not > quite sure how the code worked before 9b6a3e as the store_reg path > wouldn't have triggered the store_cpu_field(var, thumb) to set the > processor mode back to thumb. The answer to this question, incidentally, is that the thumb bit is in the SPSR we're restoring, not in the low bit of the PC value, and it gets written by gen_helper_cpsr_write_eret(). thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 14 October 2016 at 16:13, Alex Bennée <alex.bennee@linaro.org> wrote: >> This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0. >> Specifically a movs pc,lr in the kernels ret_fast_syscall returning to >> some thumb mode user space code but store_reg unconditionally aligned >> the return PC instead of treating the return as an "interworking" >> branch. >> >> I suspect we need to audit all calls to store_reg that might involve the >> PC to ensure "interworking" branches are correctly handled. Also I'm not >> quite sure how the code worked before 9b6a3e as the store_reg path >> wouldn't have triggered the store_cpu_field(var, thumb) to set the >> processor mode back to thumb. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > I think this is the wrong fix to the problem -- see the > patch I sent a few days back. Well at least my analysis of the problem was correct even if the solution was too hacky. Your patch is obviously the better solution ;-) For ref: [PATCH] Fix masking of PC lower bits when doing exception returns > > thanks > -- PMM -- Alex Bennée
diff --git a/target-arm/translate.c b/target-arm/translate.c index 5e21b52..373d68a 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -4318,7 +4318,7 @@ static void gen_mrs_banked(DisasContext *s, int r, int sysm, int rn) static void gen_exception_return(DisasContext *s, TCGv_i32 pc) { TCGv_i32 tmp; - store_reg(s, 15, pc); + store_reg_bx(s, 15, pc); tmp = load_cpu_field(spsr); gen_helper_cpsr_write_eret(cpu_env, tmp); tcg_temp_free_i32(tmp);
This was broken by the fix for 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0. Specifically a movs pc,lr in the kernels ret_fast_syscall returning to some thumb mode user space code but store_reg unconditionally aligned the return PC instead of treating the return as an "interworking" branch. I suspect we need to audit all calls to store_reg that might involve the PC to ensure "interworking" branches are correctly handled. Also I'm not quite sure how the code worked before 9b6a3e as the store_reg path wouldn't have triggered the store_cpu_field(var, thumb) to set the processor mode back to thumb. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- target-arm/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.9.3