Re: [RFC v2 3/6] target/i386: Add native library calls

[Richard Henderson]

On 6/7/23 09:47, Yeqi Fu wrote:
> +    arg0 = *(target_ulong *)g2h(cs, env->regs[R_ESP] + 4); \
> +    arg1 = *(target_ulong *)g2h(cs, env->regs[R_ESP] + 8); \
> +    arg2 = *(target_ulong *)g2h(cs, env->regs[R_ESP] + 12);

This is not correct, and will fail on big-endian hosts.

You need to use

    uintptr_t ra = GETPC();
    cpu_ldl_data_ra(env, guest_pointer, ra);

which will (amongst other things) take care of the byte swapping.

> +void helper_native_memcpy(CPUX86State *env)
> +{
> +    CPUState *cs = env_cpu(env);
> +    NATIVE_FN_W_3W();
> +    void *ret;
> +    void *dest = g2h(cs, arg0);
> +    void *src = g2h(cs, arg1);
> +    size_t n = (size_t)arg2;
> +    ret = memcpy(dest, src, n);
> +    env->regs[R_EAX] = (target_ulong)h2g(ret);
> +}

You need to do something for the case in which either src or dst is not 
accessible.

Routines like cpu_ldl_data_ra handle this for you, but you don't want to use 
that for memcpy.

There are several ways of doing this.  None of the existing helpers are ideal.

(A) void *dest = probe_write(env, arg0, arg2, MMU_USER_IDX, ra);
    void *src = probe_read(env, arg1, arg2, MMU_USER_IDX, ra);

which will raise SIGSEGV in case any byte of either region is not correctly mapped, and 
also perform the guest-to-host address remapping.  However, probe_* are written to expect 
probing of no more than one page.  Which means you'd need a loop, processing remaining 
page fractions.

(B) There is page_check_range(), which can check a large region, but doesn't handle 
address translation.  And you still wind up with a race condition if another thread 
changes page mappings at the same time.

(C) Perform the address translation etc yourself, and then protect the actual host memory 
operation in the same way as exec/cpu_ldst.h functions:

    set_helper_retaddr(ra);
    memcpy(dest, src, n);
    clear_helper_retaddr();

In this case you must also validate that 'n' is representable.  This is only an issue for 
32-bit host and 64-bit guest.  A check like (arg2 > SIZE_MAX) is likely to generate a 
silly warning about always false comparison on 64-bit hosts.  Therefore I suggest

    if (n != arg2) {
        /*
         * Overflow of size_t means that sequential pointer access would wrap.
         * We know that NULL is unmapped, so at least that one byte would fault.
         * There is nothing in the specification of memcpy that requires bytes
         * to be accessed in order, so we are allowed to fault early.
         */
        cpu_loop_exit_sigsegv(env_cpu(env), 0, MMU_DATA_LOAD, true, ra);
    }

Finally, you know the return value from the specification of memcpy: arg0.
There is no need to remap the return value back from host to guest space.


r~