| Message ID | 20180423075215.4572-5-christophe.lyon@st.com |
|---|---|
| State | New |
| Headers | show |
| Series | FDPIC ABI for ARM | expand |
On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote: > The FDPIC restorer needs to deal with a function descriptor, hence we > have to extend 'retcode' such that it can hold the instructions needed > to perform this. > > The restorer sequence uses the same thumbness as the exception > handler (mainly to support Thumb-only architectures). > > Co-Authored-By: Mickaël Guêné <mickael.guene@st.com> > Signed-off-by: Christophe Lyon <christophe.lyon@st.com> > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 8d9e6e8..d01b459 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -2045,13 +2045,13 @@ struct sigframe_v1 > { > struct target_sigcontext sc; > abi_ulong extramask[TARGET_NSIG_WORDS-1]; > - abi_ulong retcode; > + abi_ulong retcode[4]; > }; > > struct sigframe_v2 > { > struct target_ucontext_v2 uc; > - abi_ulong retcode; > + abi_ulong retcode[4]; > }; > > struct rt_sigframe_v1 > @@ -2060,14 +2060,14 @@ struct rt_sigframe_v1 > abi_ulong puc; > struct target_siginfo info; > struct target_ucontext_v1 uc; > - abi_ulong retcode; > + abi_ulong retcode[4]; > }; > > struct rt_sigframe_v2 > { > struct target_siginfo info; > struct target_ucontext_v2 uc; > - abi_ulong retcode; > + abi_ulong retcode[4]; > }; I was slightly surprised to find that the kernel adds the extra retcode slots to the signal frame unconditionally, but it does, so we can too, which is nice. > #define TARGET_CONFIG_CPU_32 1 > @@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = { > SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN > }; > > +/* > + * Stub needed to make sure the FD register (r9) contains the right > + * value. > + */ > +static const unsigned long sigreturn_fdpic_codes[3] = { > + 0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */ > + 0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */ > + 0xe59cf000 /* ldr pc, [r12] to jump into restorer */ > +}; > + > +static const unsigned long sigreturn_fdpic_thumb_codes[3] = { > + 0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */ > + 0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */ > + 0xf000f8dc /* ldr pc, [r12] to jump into restorer */ > +}; > > static inline int valid_user_regs(CPUARMState *regs) > { > @@ -2149,7 +2164,21 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, > { > abi_ulong handler = ka->_sa_handler; > abi_ulong retcode; > - int thumb = handler & 1; > + abi_ulong funcdesc_ptr = 0; > + > + int thumb; > + int is_fdpic = ((TaskState *)thread_cpu->opaque)->is_fdpic; > + > + if (is_fdpic) { > + /* In FDPIC mode, ka->_sa_handler points to a function > + * descriptor (FD). The first word contains the address of the > + * handler. The second word contains the value of the PIC > + * register (r9). */ > + funcdesc_ptr = ka->_sa_handler; You already have ka->_sa_handler in the 'handler' local, so you can just use that. > + get_user_ual(handler, funcdesc_ptr); You need to check whether get_user_ual() returned non-zero (indicating that the descriptor isn't readable) and handle that. > + } > + thumb = handler & 1; > + > uint32_t cpsr = cpsr_read(env); > > cpsr &= ~CPSR_IT; > @@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, > } > > if (ka->sa_flags & TARGET_SA_RESTORER) { > - retcode = ka->sa_restorer; > - } else { > - unsigned int idx = thumb; > + if (is_fdpic) { > + /* For FDPIC we ensure that the restorer is called with a > + * correct r9 value. For that we need to write code on > + * the stack that sets r9 and jumps back to restorer > + * value. > + */ > + if (thumb) { > + __put_user(sigreturn_fdpic_thumb_codes[0], rc); > + __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1); > + __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2); > + __put_user((abi_ulong)ka->sa_restorer, rc + 3); > + } else { > + __put_user(sigreturn_fdpic_codes[0], rc); > + __put_user(sigreturn_fdpic_codes[1], rc + 1); > + __put_user(sigreturn_fdpic_codes[2], rc + 2); > + __put_user((abi_ulong)ka->sa_restorer, rc + 3); > + } > > - if (ka->sa_flags & TARGET_SA_SIGINFO) { > - idx += 2; > + retcode = rc_addr + thumb; > + } else { > + retcode = ka->sa_restorer; > } > + } else { > + if (is_fdpic) { > + qemu_log_mask(LOG_UNIMP, > + "arm: FDPIC signal return not implemented"); > + abort(); The kernel setup_return() doesn't seem to do anything special for this case -- why do we need to abort() ? > + } else { > + unsigned int idx = thumb; > + > + if (ka->sa_flags & TARGET_SA_SIGINFO) { > + idx += 2; > + } > > - __put_user(retcodes[idx], rc); > + __put_user(retcodes[idx], rc); > > - retcode = rc_addr + thumb; > + retcode = rc_addr + thumb; > + } > } > > env->regs[0] = usig; > + if (is_fdpic) { > + get_user_ual(env->regs[9], funcdesc_ptr + 4); This also needs to check for unreadable memory. You might find it easier to read both halves of the descriptor at the same point in the code, the same way the kernel's setup_return() does. > + } thanks -- PMM
On 23/04/2018 15:05, Peter Maydell wrote: > On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote: >> The FDPIC restorer needs to deal with a function descriptor, hence we >> have to extend 'retcode' such that it can hold the instructions needed >> to perform this. >> >> The restorer sequence uses the same thumbness as the exception >> handler (mainly to support Thumb-only architectures). >> >> Co-Authored-By: Mickaël Guêné <mickael.guene@st.com> >> Signed-off-by: Christophe Lyon <christophe.lyon@st.com> >> >> diff --git a/linux-user/signal.c b/linux-user/signal.c >> index 8d9e6e8..d01b459 100644 >> --- a/linux-user/signal.c >> +++ b/linux-user/signal.c >> @@ -2045,13 +2045,13 @@ struct sigframe_v1 >> { >> struct target_sigcontext sc; >> abi_ulong extramask[TARGET_NSIG_WORDS-1]; >> - abi_ulong retcode; >> + abi_ulong retcode[4]; >> }; >> >> struct sigframe_v2 >> { >> struct target_ucontext_v2 uc; >> - abi_ulong retcode; >> + abi_ulong retcode[4]; >> }; >> >> struct rt_sigframe_v1 >> @@ -2060,14 +2060,14 @@ struct rt_sigframe_v1 >> abi_ulong puc; >> struct target_siginfo info; >> struct target_ucontext_v1 uc; >> - abi_ulong retcode; >> + abi_ulong retcode[4]; >> }; >> >> struct rt_sigframe_v2 >> { >> struct target_siginfo info; >> struct target_ucontext_v2 uc; >> - abi_ulong retcode; >> + abi_ulong retcode[4]; >> }; > > I was slightly surprised to find that the kernel adds the extra > retcode slots to the signal frame unconditionally, but it does, > so we can too, which is nice. > >> #define TARGET_CONFIG_CPU_32 1 >> @@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = { >> SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN >> }; >> >> +/* >> + * Stub needed to make sure the FD register (r9) contains the right >> + * value. >> + */ >> +static const unsigned long sigreturn_fdpic_codes[3] = { >> + 0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */ >> + 0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */ >> + 0xe59cf000 /* ldr pc, [r12] to jump into restorer */ >> +}; >> + >> +static const unsigned long sigreturn_fdpic_thumb_codes[3] = { >> + 0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */ >> + 0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */ >> + 0xf000f8dc /* ldr pc, [r12] to jump into restorer */ >> +}; >> >> static inline int valid_user_regs(CPUARMState *regs) >> { >> @@ -2149,7 +2164,21 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, >> { >> abi_ulong handler = ka->_sa_handler; >> abi_ulong retcode; >> - int thumb = handler & 1; >> + abi_ulong funcdesc_ptr = 0; >> + >> + int thumb; >> + int is_fdpic = ((TaskState *)thread_cpu->opaque)->is_fdpic; >> + >> + if (is_fdpic) { >> + /* In FDPIC mode, ka->_sa_handler points to a function >> + * descriptor (FD). The first word contains the address of the >> + * handler. The second word contains the value of the PIC >> + * register (r9). */ >> + funcdesc_ptr = ka->_sa_handler; > > You already have ka->_sa_handler in the 'handler' local, so you > can just use that. > OK, I thought it was clearer to use a different name. >> + get_user_ual(handler, funcdesc_ptr); > > You need to check whether get_user_ual() returned non-zero > (indicating that the descriptor isn't readable) and handle that. > Ha... I feared that :) I noticed that quite a lot of get_user_XXX() calls do not check the returned value, and since this patch changes void setup_return(), I'm not sure what to do in case of read error? Call abort() or is there a global flag for this purpose, that I should set and which would be checked by the caller? >> + } >> + thumb = handler & 1; >> + >> uint32_t cpsr = cpsr_read(env); >> >> cpsr &= ~CPSR_IT; >> @@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, >> } >> >> if (ka->sa_flags & TARGET_SA_RESTORER) { >> - retcode = ka->sa_restorer; >> - } else { >> - unsigned int idx = thumb; >> + if (is_fdpic) { >> + /* For FDPIC we ensure that the restorer is called with a >> + * correct r9 value. For that we need to write code on >> + * the stack that sets r9 and jumps back to restorer >> + * value. >> + */ >> + if (thumb) { >> + __put_user(sigreturn_fdpic_thumb_codes[0], rc); >> + __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1); >> + __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2); >> + __put_user((abi_ulong)ka->sa_restorer, rc + 3); >> + } else { >> + __put_user(sigreturn_fdpic_codes[0], rc); >> + __put_user(sigreturn_fdpic_codes[1], rc + 1); >> + __put_user(sigreturn_fdpic_codes[2], rc + 2); >> + __put_user((abi_ulong)ka->sa_restorer, rc + 3); >> + } >> >> - if (ka->sa_flags & TARGET_SA_SIGINFO) { >> - idx += 2; >> + retcode = rc_addr + thumb; >> + } else { >> + retcode = ka->sa_restorer; >> } >> + } else { >> + if (is_fdpic) { >> + qemu_log_mask(LOG_UNIMP, >> + "arm: FDPIC signal return not implemented"); >> + abort(); > > The kernel setup_return() doesn't seem to do anything special > for this case -- why do we need to abort() ? > I guess that's a leftover for debugging. I'll remove it. >> + } else { >> + unsigned int idx = thumb; >> + >> + if (ka->sa_flags & TARGET_SA_SIGINFO) { >> + idx += 2; >> + } >> >> - __put_user(retcodes[idx], rc); >> + __put_user(retcodes[idx], rc); >> >> - retcode = rc_addr + thumb; >> + retcode = rc_addr + thumb; >> + } >> } >> >> env->regs[0] = usig; >> + if (is_fdpic) { >> + get_user_ual(env->regs[9], funcdesc_ptr + 4); > > This also needs to check for unreadable memory. You might find it > easier to read both halves of the descriptor at the same point > in the code, the same way the kernel's setup_return() does. > Indeed. Thanks, Christophe >> + } > > thanks > -- PMM > . >
On 23 April 2018 at 15:22, Christophe Lyon <christophe.lyon@st.com> wrote: > On 23/04/2018 15:05, Peter Maydell wrote: >> >> On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote: >>> @@ -2149,7 +2164,21 @@ setup_return(CPUARMState *env, struct >>> target_sigaction *ka, >>> { >>> abi_ulong handler = ka->_sa_handler; >>> abi_ulong retcode; >>> - int thumb = handler & 1; >>> + abi_ulong funcdesc_ptr = 0; >>> + >>> + int thumb; >>> + int is_fdpic = ((TaskState *)thread_cpu->opaque)->is_fdpic; >>> + >>> + if (is_fdpic) { >>> + /* In FDPIC mode, ka->_sa_handler points to a function >>> + * descriptor (FD). The first word contains the address of the >>> + * handler. The second word contains the value of the PIC >>> + * register (r9). */ >>> + funcdesc_ptr = ka->_sa_handler; >> >> >> You already have ka->_sa_handler in the 'handler' local, so you >> can just use that. >> > OK, I thought it was clearer to use a different name. The other way to structure that would be to put handler = ka->_sa_handler; in an else clause of this if (). >>> + get_user_ual(handler, funcdesc_ptr); >> >> >> You need to check whether get_user_ual() returned non-zero >> (indicating that the descriptor isn't readable) and handle that. >> > Ha... I feared that :) > I noticed that quite a lot of get_user_XXX() calls do not > check the returned value, and since this patch changes > void setup_return(), I'm not sure what to do in case of > read error? Call abort() or is there a global flag > for this purpose, that I should set and which would > be checked by the caller? You need to: * make setup_return() return an error indication (other code in this file seems to follow the kernel's example and have this be a return value that's true on an error and false otherwise) * have the callers check the error indication, and if there was an error do: - unlock the user struct - call force_sigsegv() All the callers already have code for doing force_sigsegv, though they don't yet have a codepath that handles doing the unlock first. You should be able to just put in the call to unlock_user_struct(frame, ...) at the existing 'sigsegv:' labels, because that is guaranteed to be safe on a NULL host pointer, which is what you'll get in the other code paths that go to that label. Then you can have the error check be if (setup_return(...)) { goto sigsegv; } thanks -- PMM
diff --git a/linux-user/signal.c b/linux-user/signal.c index 8d9e6e8..d01b459 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -2045,13 +2045,13 @@ struct sigframe_v1 { struct target_sigcontext sc; abi_ulong extramask[TARGET_NSIG_WORDS-1]; - abi_ulong retcode; + abi_ulong retcode[4]; }; struct sigframe_v2 { struct target_ucontext_v2 uc; - abi_ulong retcode; + abi_ulong retcode[4]; }; struct rt_sigframe_v1 @@ -2060,14 +2060,14 @@ struct rt_sigframe_v1 abi_ulong puc; struct target_siginfo info; struct target_ucontext_v1 uc; - abi_ulong retcode; + abi_ulong retcode[4]; }; struct rt_sigframe_v2 { struct target_siginfo info; struct target_ucontext_v2 uc; - abi_ulong retcode; + abi_ulong retcode[4]; }; #define TARGET_CONFIG_CPU_32 1 @@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = { SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN }; +/* + * Stub needed to make sure the FD register (r9) contains the right + * value. + */ +static const unsigned long sigreturn_fdpic_codes[3] = { + 0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */ + 0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */ + 0xe59cf000 /* ldr pc, [r12] to jump into restorer */ +}; + +static const unsigned long sigreturn_fdpic_thumb_codes[3] = { + 0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */ + 0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */ + 0xf000f8dc /* ldr pc, [r12] to jump into restorer */ +}; static inline int valid_user_regs(CPUARMState *regs) { @@ -2149,7 +2164,21 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, { abi_ulong handler = ka->_sa_handler; abi_ulong retcode; - int thumb = handler & 1; + abi_ulong funcdesc_ptr = 0; + + int thumb; + int is_fdpic = ((TaskState *)thread_cpu->opaque)->is_fdpic; + + if (is_fdpic) { + /* In FDPIC mode, ka->_sa_handler points to a function + * descriptor (FD). The first word contains the address of the + * handler. The second word contains the value of the PIC + * register (r9). */ + funcdesc_ptr = ka->_sa_handler; + get_user_ual(handler, funcdesc_ptr); + } + thumb = handler & 1; + uint32_t cpsr = cpsr_read(env); cpsr &= ~CPSR_IT; @@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka, } if (ka->sa_flags & TARGET_SA_RESTORER) { - retcode = ka->sa_restorer; - } else { - unsigned int idx = thumb; + if (is_fdpic) { + /* For FDPIC we ensure that the restorer is called with a + * correct r9 value. For that we need to write code on + * the stack that sets r9 and jumps back to restorer + * value. + */ + if (thumb) { + __put_user(sigreturn_fdpic_thumb_codes[0], rc); + __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1); + __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2); + __put_user((abi_ulong)ka->sa_restorer, rc + 3); + } else { + __put_user(sigreturn_fdpic_codes[0], rc); + __put_user(sigreturn_fdpic_codes[1], rc + 1); + __put_user(sigreturn_fdpic_codes[2], rc + 2); + __put_user((abi_ulong)ka->sa_restorer, rc + 3); + } - if (ka->sa_flags & TARGET_SA_SIGINFO) { - idx += 2; + retcode = rc_addr + thumb; + } else { + retcode = ka->sa_restorer; } + } else { + if (is_fdpic) { + qemu_log_mask(LOG_UNIMP, + "arm: FDPIC signal return not implemented"); + abort(); + } else { + unsigned int idx = thumb; + + if (ka->sa_flags & TARGET_SA_SIGINFO) { + idx += 2; + } - __put_user(retcodes[idx], rc); + __put_user(retcodes[idx], rc); - retcode = rc_addr + thumb; + retcode = rc_addr + thumb; + } } env->regs[0] = usig; + if (is_fdpic) { + get_user_ual(env->regs[9], funcdesc_ptr + 4); + } env->regs[13] = frame_addr; env->regs[14] = retcode; env->regs[15] = handler & (thumb ? ~1 : ~3); @@ -2270,7 +2329,7 @@ static void setup_frame_v1(int usig, struct target_sigaction *ka, __put_user(set->sig[i], &frame->extramask[i - 1]); } - setup_return(regs, ka, &frame->retcode, frame_addr, usig, + setup_return(regs, ka, frame->retcode, frame_addr, usig, frame_addr + offsetof(struct sigframe_v1, retcode)); unlock_user_struct(frame, frame_addr, 1); @@ -2292,7 +2351,7 @@ static void setup_frame_v2(int usig, struct target_sigaction *ka, setup_sigframe_v2(&frame->uc, set, regs); - setup_return(regs, ka, &frame->retcode, frame_addr, usig, + setup_return(regs, ka, frame->retcode, frame_addr, usig, frame_addr + offsetof(struct sigframe_v2, retcode)); unlock_user_struct(frame, frame_addr, 1); @@ -2347,7 +2406,7 @@ static void setup_rt_frame_v1(int usig, struct target_sigaction *ka, __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]); } - setup_return(env, ka, &frame->retcode, frame_addr, usig, + setup_return(env, ka, frame->retcode, frame_addr, usig, frame_addr + offsetof(struct rt_sigframe_v1, retcode)); env->regs[1] = info_addr; @@ -2378,7 +2437,7 @@ static void setup_rt_frame_v2(int usig, struct target_sigaction *ka, setup_sigframe_v2(&frame->uc, set, env); - setup_return(env, ka, &frame->retcode, frame_addr, usig, + setup_return(env, ka, frame->retcode, frame_addr, usig, frame_addr + offsetof(struct rt_sigframe_v2, retcode)); env->regs[1] = info_addr;
The FDPIC restorer needs to deal with a function descriptor, hence we have to extend 'retcode' such that it can hold the instructions needed to perform this. The restorer sequence uses the same thumbness as the exception handler (mainly to support Thumb-only architectures). Co-Authored-By: Mickaël Guêné <mickael.guene@st.com> Signed-off-by: Christophe Lyon <christophe.lyon@st.com> -- 2.6.3