[24/30] bsd-user/signal.c: setup_frame

setup_frame sets up a signalled stack frame. Associated routines to
extract the pointer to the stack frame and to support alternate stacks.

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Kyle Evans <kevans@freebsd.org>
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/signal.c | 166 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 144 insertions(+), 22 deletions(-)

On Sun, 9 Jan 2022 at 16:36, Warner Losh <imp@bsdimp.com> wrote:
>
> setup_frame sets up a signalled stack frame. Associated routines to
> extract the pointer to the stack frame and to support alternate stacks.
>
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Kyle Evans <kevans@freebsd.org>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  bsd-user/signal.c | 166 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 144 insertions(+), 22 deletions(-)
>
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index 8dadc9a39a7..8e1427553da 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -30,11 +30,27 @@
>   * fork.
>   */
>
> +static target_stack_t target_sigaltstack_used = {
> +    .ss_sp = 0,
> +    .ss_size = 0,
> +    .ss_flags = TARGET_SS_DISABLE,
> +};

sigaltstacks are per-thread, so this needs to be in the TaskState,
not global. (We fixed this in linux-user in commit 5bfce0b74fbd5d5
in 2019: the change is relatively small.)

> +
>  static struct target_sigaction sigact_table[TARGET_NSIG];
>  static void host_signal_handler(int host_sig, siginfo_t *info, void *puc);
>  static void target_to_host_sigset_internal(sigset_t *d,
>          const target_sigset_t *s);
>
> +static inline int on_sig_stack(unsigned long sp)
> +{
> +    return sp - target_sigaltstack_used.ss_sp < target_sigaltstack_used.ss_size;
> +}
> +
> +static inline int sas_ss_flags(unsigned long sp)
> +{
> +    return target_sigaltstack_used.ss_size == 0 ? SS_DISABLE : on_sig_stack(sp)
> +        ? SS_ONSTACK : 0;
> +}
>
>  int host_to_target_signal(int sig)
>  {
> @@ -336,28 +352,6 @@ void queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
>      return;
>  }
>
> -static int fatal_signal(int sig)
> -{
> -
> -    switch (sig) {
> -    case TARGET_SIGCHLD:
> -    case TARGET_SIGURG:
> -    case TARGET_SIGWINCH:
> -    case TARGET_SIGINFO:
> -        /* Ignored by default. */
> -        return 0;
> -    case TARGET_SIGCONT:
> -    case TARGET_SIGSTOP:
> -    case TARGET_SIGTSTP:
> -    case TARGET_SIGTTIN:
> -    case TARGET_SIGTTOU:
> -        /* Job control signals.  */
> -        return 0;
> -    default:
> -        return 1;
> -    }
> -}

There wasn't any need to move this function, I think ?

> -
>  /*
>   * Force a synchronously taken QEMU_SI_FAULT signal. For QEMU the
>   * 'force' part is handled in process_pending_signals().
> @@ -484,6 +478,134 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
>      cpu_exit(thread_cpu);
>  }
>
> +static int fatal_signal(int sig)
> +{
> +
> +    switch (sig) {
> +    case TARGET_SIGCHLD:
> +    case TARGET_SIGURG:
> +    case TARGET_SIGWINCH:
> +    case TARGET_SIGINFO:
> +        /* Ignored by default. */
> +        return 0;
> +    case TARGET_SIGCONT:
> +    case TARGET_SIGSTOP:
> +    case TARGET_SIGTSTP:
> +    case TARGET_SIGTTIN:
> +    case TARGET_SIGTTOU:
> +        /* Job control signals.  */
> +        return 0;
> +    default:
> +        return 1;
> +    }
> +}
> +
> +static inline abi_ulong get_sigframe(struct target_sigaction *ka,
> +        CPUArchState *regs, size_t frame_size)
> +{
> +    abi_ulong sp;
> +
> +    /* Use default user stack */
> +    sp = get_sp_from_cpustate(regs);
> +
> +    if ((ka->sa_flags & TARGET_SA_ONSTACK) && (sas_ss_flags(sp) == 0)) {
> +        sp = target_sigaltstack_used.ss_sp +
> +            target_sigaltstack_used.ss_size;
> +    }
> +
> +#if defined(TARGET_MIPS) || defined(TARGET_ARM)
> +    return (sp - frame_size) & ~7;
> +#elif defined(TARGET_AARCH64)
> +    return (sp - frame_size) & ~15;
> +#else
> +    return sp - frame_size;
> +#endif

We don't need to do it in this patchseries, but you should strongly
consider pulling the architecture-specifics out in a way that
avoids this kind of ifdef ladder.

> +}
> +
> +/* compare to mips/mips/pm_machdep.c and sparc64/sparc64/machdep.c sendsig() */
> +static void setup_frame(int sig, int code, struct target_sigaction *ka,
> +    target_sigset_t *set, target_siginfo_t *tinfo, CPUArchState *regs)
> +{
> +    struct target_sigframe *frame;
> +    abi_ulong frame_addr;
> +    int i;
> +
> +    frame_addr = get_sigframe(ka, regs, sizeof(*frame));
> +    trace_user_setup_frame(regs, frame_addr);
> +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> +        goto give_sigsegv;

FreeBSD for Arm (haven't checked other BSDs or other archs)
gives a SIGILL for the "can't write signal frame to stack"
case, I think:
https://github.com/freebsd/freebsd-src/blob/main/sys/arm/arm/exec_machdep.c#L316
I don't understand why they picked SIGILL, SIGSEGV seems much more
logical to me, but we should follow the kernel behaviour.

> +    }
> +
> +    memset(frame, 0, sizeof(*frame));
> +#if defined(TARGET_MIPS)
> +    int mflags = on_sig_stack(frame_addr) ? TARGET_MC_ADD_MAGIC :
> +        TARGET_MC_SET_ONSTACK | TARGET_MC_ADD_MAGIC;
> +#else
> +    int mflags = 0;
> +#endif
> +    if (get_mcontext(regs, &frame->sf_uc.uc_mcontext, mflags)) {
> +        goto give_sigsegv;

The FreeBSD kernel get_mcontext() can't fail -- why can ours ?
(This matters because SIGSEGV may not be the right response to
whatever the failure case is.)

> +    }
> +
> +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
> +        if (__put_user(set->__bits[i], &frame->sf_uc.uc_sigmask.__bits[i])) {
> +            goto give_sigsegv;

__get_user() and __put_user() in QEMU can't fail, so you don't
need to check for errors here, unlike the non-double-underscore
versions. At some point you might want to take the current linux-user
versions of these user-access functions/macros: it looks like bsd-user
has an older version which doesn't handle the case where the guest
has looser alignment restrictions than the host. The new ones
actually expand to do { ... } while(0) statements which won't
be valid inside an if() condition.

(Historical note: the reason QEMU's __put_user/__get_user ever had
return values at all is that the linux-user code was copy-and-pasted
from the Linux kernel. In the Linux kernel handling of writing
data to userspace is/was error-checked on every write, whereas
QEMU does the "is this writable" test once with the lock_user
function and then can assume all the writes to that area succeed.
But we still started with a lot of copy-pasted code that was doing
unnecessary checks on __put_user and __get_user return values.
FreeBSD seems to handle write-checking in yet a third way, by
assembling the struct in kernel-space and checking for writability
once at the end when it copies the whole block out to userspace.)

> +        }
> +    }
> +
> +    if (tinfo) {
> +        frame->sf_si.si_signo = tinfo->si_signo;
> +        frame->sf_si.si_errno = tinfo->si_errno;
> +        frame->sf_si.si_code = tinfo->si_code;
> +        frame->sf_si.si_pid = tinfo->si_pid;
> +        frame->sf_si.si_uid = tinfo->si_uid;
> +        frame->sf_si.si_status = tinfo->si_status;
> +        frame->sf_si.si_addr = tinfo->si_addr;
> +
> +        if (TARGET_SIGILL == sig || TARGET_SIGFPE == sig ||
> +                TARGET_SIGSEGV == sig || TARGET_SIGBUS == sig ||
> +                TARGET_SIGTRAP == sig) {
> +            frame->sf_si._reason._fault._trapno = tinfo->_reason._fault._trapno;
> +        }
> +
> +        /*
> +         * If si_code is one of SI_QUEUE, SI_TIMER, SI_ASYNCIO, or
> +         * SI_MESGQ, then si_value contains the application-specified
> +         * signal value. Otherwise, the contents of si_value are
> +         * undefined.
> +         */
> +        if (SI_QUEUE == code || SI_TIMER == code || SI_ASYNCIO == code ||
> +                SI_MESGQ == code) {
> +            frame->sf_si.si_value.sival_int = tinfo->si_value.sival_int;
> +        }
> +
> +        if (SI_TIMER == code) {
> +            frame->sf_si._reason._timer._timerid =
> +                tinfo->_reason._timer._timerid;
> +            frame->sf_si._reason._timer._overrun =
> +                tinfo->_reason._timer._overrun;
> +        }
> +
> +#ifdef SIGPOLL
> +        if (SIGPOLL == sig) {
> +            frame->sf_si._reason._band = tinfo->_reason._band;
> +        }
> +#endif

This seems to be yet a third set of the logic for handling
target_siginfo_t's union, to go along with tswap_siginfo() and
host_to_target_siginfo_noswap(), except that the logic here is
different. linux-user calls tswap_siginfo() in its signal-frame
setup code.

> +
> +    }
> +
> +    if (set_sigtramp_args(regs, sig, frame, frame_addr, ka)) {
> +        goto give_sigsegv;
> +    }

set_sigtramp_args() can't fail. (Not sure why it has a non-void
return type.)

> +
> +    unlock_user_struct(frame, frame_addr, 1);
> +    return;
> +
> +give_sigsegv:
> +    unlock_user_struct(frame, frame_addr, 1);
> +    force_sig(TARGET_SIGSEGV);
> +}
> +
>  void signal_init(void)
>  {
>      TaskState *ts = (TaskState *)thread_cpu->opaque;
> --
> 2.33.1

thanks
-- PMM

On Fri, Jan 14, 2022 at 4:40 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sun, 9 Jan 2022 at 16:36, Warner Losh <imp@bsdimp.com> wrote:
> >
> > setup_frame sets up a signalled stack frame. Associated routines to
> > extract the pointer to the stack frame and to support alternate stacks.
> >
> > Signed-off-by: Stacey Son <sson@FreeBSD.org>
> > Signed-off-by: Kyle Evans <kevans@freebsd.org>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >  bsd-user/signal.c | 166 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 144 insertions(+), 22 deletions(-)
> >
> > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > index 8dadc9a39a7..8e1427553da 100644
> > --- a/bsd-user/signal.c
> > +++ b/bsd-user/signal.c
> > @@ -30,11 +30,27 @@
> >   * fork.
> >   */
> >
> > +static target_stack_t target_sigaltstack_used = {
> > +    .ss_sp = 0,
> > +    .ss_size = 0,
> > +    .ss_flags = TARGET_SS_DISABLE,
> > +};
>
> sigaltstacks are per-thread, so this needs to be in the TaskState,
> not global. (We fixed this in linux-user in commit 5bfce0b74fbd5d5
> in 2019: the change is relatively small.)
>

Done. I saw go mentioned, which I know doesn't work today, so it's
a good step in that direction...


> > +
> >  static struct target_sigaction sigact_table[TARGET_NSIG];
> >  static void host_signal_handler(int host_sig, siginfo_t *info, void
> *puc);
> >  static void target_to_host_sigset_internal(sigset_t *d,
> >          const target_sigset_t *s);
> >
> > +static inline int on_sig_stack(unsigned long sp)
> > +{
> > +    return sp - target_sigaltstack_used.ss_sp <
> target_sigaltstack_used.ss_size;
> > +}
> > +
> > +static inline int sas_ss_flags(unsigned long sp)
> > +{
> > +    return target_sigaltstack_used.ss_size == 0 ? SS_DISABLE :
> on_sig_stack(sp)
> > +        ? SS_ONSTACK : 0;
> > +}
> >
> >  int host_to_target_signal(int sig)
> >  {
> > @@ -336,28 +352,6 @@ void queue_signal(CPUArchState *env, int sig,
> target_siginfo_t *info)
> >      return;
> >  }
> >
> > -static int fatal_signal(int sig)
> > -{
> > -
> > -    switch (sig) {
> > -    case TARGET_SIGCHLD:
> > -    case TARGET_SIGURG:
> > -    case TARGET_SIGWINCH:
> > -    case TARGET_SIGINFO:
> > -        /* Ignored by default. */
> > -        return 0;
> > -    case TARGET_SIGCONT:
> > -    case TARGET_SIGSTOP:
> > -    case TARGET_SIGTSTP:
> > -    case TARGET_SIGTTIN:
> > -    case TARGET_SIGTTOU:
> > -        /* Job control signals.  */
> > -        return 0;
> > -    default:
> > -        return 1;
> > -    }
> > -}
>
> There wasn't any need to move this function, I think ?
>

No, there was some other conflict during rebase getting the patch train
ready that I thought I'd cleaned up, but this was fallout from that which
I overlooked. I've undone it...


> > -
> >  /*
> >   * Force a synchronously taken QEMU_SI_FAULT signal. For QEMU the
> >   * 'force' part is handled in process_pending_signals().
> > @@ -484,6 +478,134 @@ static void host_signal_handler(int host_sig,
> siginfo_t *info, void *puc)
> >      cpu_exit(thread_cpu);
> >  }
> >
> > +static int fatal_signal(int sig)
> > +{
> > +
> > +    switch (sig) {
> > +    case TARGET_SIGCHLD:
> > +    case TARGET_SIGURG:
> > +    case TARGET_SIGWINCH:
> > +    case TARGET_SIGINFO:
> > +        /* Ignored by default. */
> > +        return 0;
> > +    case TARGET_SIGCONT:
> > +    case TARGET_SIGSTOP:
> > +    case TARGET_SIGTSTP:
> > +    case TARGET_SIGTTIN:
> > +    case TARGET_SIGTTOU:
> > +        /* Job control signals.  */
> > +        return 0;
> > +    default:
> > +        return 1;
> > +    }
> > +}
> > +
> > +static inline abi_ulong get_sigframe(struct target_sigaction *ka,
> > +        CPUArchState *regs, size_t frame_size)
> > +{
> > +    abi_ulong sp;
> > +
> > +    /* Use default user stack */
> > +    sp = get_sp_from_cpustate(regs);
> > +
> > +    if ((ka->sa_flags & TARGET_SA_ONSTACK) && (sas_ss_flags(sp) == 0)) {
> > +        sp = target_sigaltstack_used.ss_sp +
> > +            target_sigaltstack_used.ss_size;
> > +    }
> > +
> > +#if defined(TARGET_MIPS) || defined(TARGET_ARM)
> > +    return (sp - frame_size) & ~7;
> > +#elif defined(TARGET_AARCH64)
> > +    return (sp - frame_size) & ~15;
> > +#else
> > +    return sp - frame_size;
> > +#endif
>
> We don't need to do it in this patchseries, but you should strongly
> consider pulling the architecture-specifics out in a way that
> avoids this kind of ifdef ladder.
>

Totally agreed. I debated fixing this before I started this patch
run, but I decided to pick my battles... I'll fix this in a follow up.


> > +}
> > +
> > +/* compare to mips/mips/pm_machdep.c and sparc64/sparc64/machdep.c
> sendsig() */
>

Two dead architectures... I've updated the comments...  and the filename
which
they live in...


> > +static void setup_frame(int sig, int code, struct target_sigaction *ka,
> > +    target_sigset_t *set, target_siginfo_t *tinfo, CPUArchState *regs)
> > +{
> > +    struct target_sigframe *frame;
> > +    abi_ulong frame_addr;
> > +    int i;
> > +
> > +    frame_addr = get_sigframe(ka, regs, sizeof(*frame));
> > +    trace_user_setup_frame(regs, frame_addr);
> > +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> > +        goto give_sigsegv;
>
> FreeBSD for Arm (haven't checked other BSDs or other archs)
> gives a SIGILL for the "can't write signal frame to stack"
> case, I think:
>
> https://github.com/freebsd/freebsd-src/blob/main/sys/arm/arm/exec_machdep.c#L316
> I don't understand why they picked SIGILL, SIGSEGV seems much more
> logical to me, but we should follow the kernel behaviour.
>

This is a good thing to find. I'm going to have to study all the
architectures, but
the first 5 I looked at all returned SIGILL, so this code has to change to
reflect
that...


> > +    }
> > +
> > +    memset(frame, 0, sizeof(*frame));
> > +#if defined(TARGET_MIPS)
> > +    int mflags = on_sig_stack(frame_addr) ? TARGET_MC_ADD_MAGIC :
> > +        TARGET_MC_SET_ONSTACK | TARGET_MC_ADD_MAGIC;
> > +#else
> > +    int mflags = 0;
> > +#endif
> > +    if (get_mcontext(regs, &frame->sf_uc.uc_mcontext, mflags)) {
> > +        goto give_sigsegv;
>
> The FreeBSD kernel get_mcontext() can't fail -- why can ours ?
> (This matters because SIGSEGV may not be the right response to
> whatever the failure case is.)
>

    if (mcp->mc_vfp_size != 0 && mcp->mc_vfp_size !=
sizeof(target_mcontext_vfp_t)) {

is what I try to validate, but looking at the kernel code it does NOT
return vfp, so that
check is bogus. I need to remove it (I'd added as part of a prior review
without fully
checking before I made the change, to be honest).


> > +    }
> > +
> > +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
> > +        if (__put_user(set->__bits[i],
> &frame->sf_uc.uc_sigmask.__bits[i])) {
> > +            goto give_sigsegv;
>
> __get_user() and __put_user() in QEMU can't fail, so you don't
> need to check for errors here, unlike the non-double-underscore
> versions. At some point you might want to take the current linux-user
> versions of these user-access functions/macros: it looks like bsd-user
> has an older version which doesn't handle the case where the guest
> has looser alignment restrictions than the host. The new ones
> actually expand to do { ... } while(0) statements which won't
> be valid inside an if() condition.
>

OK. I'll make a sweep in addition to fixing this.


> (Historical note: the reason QEMU's __put_user/__get_user ever had
> return values at all is that the linux-user code was copy-and-pasted
> from the Linux kernel. In the Linux kernel handling of writing
> data to userspace is/was error-checked on every write, whereas
> QEMU does the "is this writable" test once with the lock_user
> function and then can assume all the writes to that area succeed.
> But we still started with a lot of copy-pasted code that was doing
> unnecessary checks on __put_user and __get_user return values.
> FreeBSD seems to handle write-checking in yet a third way, by
> assembling the struct in kernel-space and checking for writability
> once at the end when it copies the whole block out to userspace.)
>

We're nothing if not creative... But this is old school Unix which started
doing this in V7 which started doing it for stat and ftime system calls...


> > +        }
> > +    }
> > +
> > +    if (tinfo) {
> > +        frame->sf_si.si_signo = tinfo->si_signo;
> > +        frame->sf_si.si_errno = tinfo->si_errno;
> > +        frame->sf_si.si_code = tinfo->si_code;
> > +        frame->sf_si.si_pid = tinfo->si_pid;
> > +        frame->sf_si.si_uid = tinfo->si_uid;
> > +        frame->sf_si.si_status = tinfo->si_status;
> > +        frame->sf_si.si_addr = tinfo->si_addr;
> > +
> > +        if (TARGET_SIGILL == sig || TARGET_SIGFPE == sig ||
> > +                TARGET_SIGSEGV == sig || TARGET_SIGBUS == sig ||
> > +                TARGET_SIGTRAP == sig) {
> > +            frame->sf_si._reason._fault._trapno =
> tinfo->_reason._fault._trapno;
> > +        }
> > +
> > +        /*
> > +         * If si_code is one of SI_QUEUE, SI_TIMER, SI_ASYNCIO, or
> > +         * SI_MESGQ, then si_value contains the application-specified
> > +         * signal value. Otherwise, the contents of si_value are
> > +         * undefined.
> > +         */
> > +        if (SI_QUEUE == code || SI_TIMER == code || SI_ASYNCIO == code
> ||
> > +                SI_MESGQ == code) {
> > +            frame->sf_si.si_value.sival_int = tinfo->si_value.sival_int;
> > +        }
> > +
> > +        if (SI_TIMER == code) {
> > +            frame->sf_si._reason._timer._timerid =
> > +                tinfo->_reason._timer._timerid;
> > +            frame->sf_si._reason._timer._overrun =
> > +                tinfo->_reason._timer._overrun;
> > +        }
> > +
> > +#ifdef SIGPOLL
> > +        if (SIGPOLL == sig) {
> > +            frame->sf_si._reason._band = tinfo->_reason._band;
> > +        }
> > +#endif
>
> This seems to be yet a third set of the logic for handling
> target_siginfo_t's union, to go along with tswap_siginfo() and
> host_to_target_siginfo_noswap(), except that the logic here is
> different. linux-user calls tswap_siginfo() in its signal-frame
> setup code.
>

Yea, I need to get that sorted out as well.. It's my biggest remaining
item to resolve (except maybe for the comments for do_* you made,
I've not yet completely digested them when I saw they were more
than reviewed by).


> > +
> > +    }
> > +
> > +    if (set_sigtramp_args(regs, sig, frame, frame_addr, ka)) {
> > +        goto give_sigsegv;
> > +    }
>
> set_sigtramp_args() can't fail. (Not sure why it has a non-void
> return type.)
>

OK. I'll fix this as well.


> > +
> > +    unlock_user_struct(frame, frame_addr, 1);
> > +    return;
> > +
> > +give_sigsegv:
> > +    unlock_user_struct(frame, frame_addr, 1);
> > +    force_sig(TARGET_SIGSEGV);
> > +}
> > +
> >  void signal_init(void)
> >  {
> >      TaskState *ts = (TaskState *)thread_cpu->opaque;
> > --
> > 2.33.1
>
> thanks
>

No, thank you. This is quite helpful and got me to look at a number of new
places and understand some historic background.

Warmer -- PMM

On Sun, Jan 16, 2022 at 11:58 PM Warner Losh <imp@bsdimp.com> wrote:

> > +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
>> > +        goto give_sigsegv;
>>
>> FreeBSD for Arm (haven't checked other BSDs or other archs)
>> gives a SIGILL for the "can't write signal frame to stack"
>> case, I think:
>>
>> https://github.com/freebsd/freebsd-src/blob/main/sys/arm/arm/exec_machdep.c#L316
>> I don't understand why they picked SIGILL, SIGSEGV seems much more
>> logical to me, but we should follow the kernel behaviour.
>>
>
> This is a good thing to find. I'm going to have to study all the
> architectures, but
> the first 5 I looked at all returned SIGILL, so this code has to change to
> reflect
> that...
>

Sorry to follow up my own message, but  this dates to 4.1BSD (4BSD sent a
SIGKILL),
but it's not present in V7, 32V or 3BSD.

So it's very old-school BSD behavior, dating from 1981 :)

Warner