On 23 April 2018 at 15:22, Christophe Lyon <christophe.l...@st.com> wrote:
> On 23/04/2018 15:05, Peter Maydell wrote:
>>
>> On 23 April 2018 at 08:51, Christophe Lyon <christophe.l...@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
