Re: [PATCH for 6.2 28/49] bsd-user: Move stack initializtion into a per-os file.

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:
> +static inline int setup_initial_stack(struct bsd_binprm *bprm,
> +        abi_ulong *ret_addr, abi_ulong *stringp)
> +{
> +    int i;
> +    abi_ulong stack_hi_addr;
> +    size_t execpath_len, stringspace;
> +    abi_ulong destp, argvp, envp, p;
> +    struct target_ps_strings ps_strs;
> +    char canary[sizeof(abi_long) * 8];
> +
> +    stack_hi_addr = p = target_stkbas + target_stksiz;
> +
> +    /* Save some space for ps_strings. */
> +    p -= sizeof(struct target_ps_strings);
> +
> +#ifdef TARGET_SZSIGCODE
> +    /* Add machine depedent sigcode. */
> +    p -= TARGET_SZSIGCODE;
> +    if (setup_sigtramp(p, (unsigned)offsetof(struct target_sigframe, sf_uc),
> +            TARGET_FREEBSD_NR_sigreturn)) {
> +        errno = EFAULT;
> +        return -1;
> +    }
> +#endif

Hmm.  The x86 version you just added always returns -EOPNOTSUPP.  Therefore I conclude 
that TARGET_SZSIGCODE is unset.

Perhaps a better interface would be

  p = setup_sigtramp(p, ...);
  if (p == 0) {
    // fail
  }

then you don't need to expose TARGET_SZSIGCODE or have conditional compilation 
here.

Perhaps for the to-do list...

> +    /* Add canary for SSP. */
> +    arc4random_buf(canary, sizeof(canary));

You should use qemu_guest_getrandom_nofail here.

> +    /*
> +     * Deviate from FreeBSD stack layout: force stack to new page here
> +     * so that signal trampoline is not sharing the page with user stack
> +     * frames. This is actively harmful in qemu as it marks pages with
> +     * code it translated as read-only, which is somewhat problematic
> +     * for user trying to use the stack as intended.
> +     */

A decent short-term solution.

I'll draw your attention to my vdso patch set for linux-user:
https://patchew.org/QEMU/20210706234932.356913-1-richard.henderson@linaro.org/

Modulo randomness,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~