Re: [PATCH 1/1] target/ppc: fix ELFv2 signal handler endianness

[David Gibson]

On Wed, Mar 18, 2020 at 10:00:20AM -0500, Vincent Fazio wrote:
> David, Laurent,
> 
> On 3/15/20 9:21 PM, David Gibson wrote:
> > On Sun, Mar 15, 2020 at 07:29:04PM -0500, Vincent Fazio wrote:
> > > Laurent,
> > > 
> > > On Sun, Mar 15, 2020 at 1:10 PM Laurent Vivier <address@hidden> wrote:
> > > > Le 15/03/2020 à 16:52, Vincent Fazio a écrit :
> > > > > From: Vincent Fazio <address@hidden>
> > > > > 
> > > > > In ELFv2, function pointers are entry points and are in host 
> > > > > endianness.
> > > > "host endianness" is misleading here. "target endianness" is better.
> > Yeah, the trouble here is that I think the ELF spec will use "host"
> > and "target" in a quite different sense than qemu.
> > 
> I'll be simplifying the wording in the message to just mention the
> problematic cross-endian scenario
> > > I do want to clarify here. In a mixed endian scenario (my test case
> > > was an x86 host and e5500 PPC BE target), the function pointers are in
> > > host endianness (little endian) so that the virtual address can be
> > > dereferenced by the host for the target instructions to be
> > > translated.
> > This can't be right.  The ELF is operating entirely within the guest,
> > and has no concept of a host (in the qemu sense).  Therefore it's
> > impossible for it to specify anything as "host endian" (again in the
> > qemu sense).
> > 
> > It *is* possible that it's little endian explicitly (in which case
> > we'd need a conditional swap that's different from the one we have
> > now).
> > 
> > But even that seems pretty odd.  AFAICT that target_sigaction
> > structure is copied verbatim from guest memory when the guest makes
> > the sigaction() syscall.  Are we expecting a BE process to put LE
> > parameters into a syscall structure?  That seems unlikely.
> > 
> > I really think you need to put some instrumentation in the sigaction()
> > call that comes before this, to see exactly what the guest process is
> > supplying there.
> > 
> > And then we maybe need to look at your guest side libc and/or a native
> > e5500 BE kernel to see what it expects in that structure.
> As we discussed in the other thread, I missed the endian swap done as part
> of get_user in do_sigaction.

Right, as did I when I looked the first time.

> So while my initial determination for the root
> cause of the problem was wrong, the fix is still the same (drop the `tswapl`
> call). The commit message will be updated.

Right, agreed.

> > > > > Previously, the signal handler would be swapped if the target CPU was 
> > > > > a
> > > > > different endianness than the host. This would cause a SIGSEGV when
> > > > > attempting to translate the opcode pointed to by the swapped address.
> > > > This is correct.
> > > > 
> > > > >   Thread 1 "qemu-ppc64" received signal SIGSEGV, Segmentation fault.
> > > > >   0x00000000600a9257 in ldl_he_p (ptr=0x4c2c061000000000) at 
> > > > > qemu/include/qemu/bswap.h:351
> > > > >   351        __builtin_memcpy(&r, ptr, sizeof(r));
> > > > > 
> > > > >   #0  0x00000000600a9257 in ldl_he_p (ptr=0x4c2c061000000000) at 
> > > > > qemu/include/qemu/bswap.h:351
> > > > >   #1  0x00000000600a92fe in ldl_be_p (ptr=0x4c2c061000000000) at 
> > > > > qemu/include/qemu/bswap.h:449
> > > > >   #2  0x00000000600c0790 in translator_ldl_swap at 
> > > > > qemu/include/exec/translator.h:201
> > > > >   #3  0x000000006011c1ab in ppc_tr_translate_insn at 
> > > > > qemu/target/ppc/translate.c:7856
> > > > >   #4  0x000000006005ae70 in translator_loop at 
> > > > > qemu/accel/tcg/translator.c:102
> > > > > 
> > > > > Now, no swap is performed and execution continues properly.
> > > > > 
> > > > > Signed-off-by: Vincent Fazio <address@hidden>
> > > > > ---
> > > > >   linux-user/ppc/signal.c | 10 +++++++---
> > > > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> > > > > index 5b82af6cb6..c7f6455170 100644
> > > > > --- a/linux-user/ppc/signal.c
> > > > > +++ b/linux-user/ppc/signal.c
> > > > > @@ -567,9 +567,13 @@ void setup_rt_frame(int sig, struct 
> > > > > target_sigaction *ka,
> > > > >           env->nip = tswapl(handler->entry);
> > > > >           env->gpr[2] = tswapl(handler->toc);
> > > > >       } else {
> > > > > -        /* ELFv2 PPC64 function pointers are entry points, but R12
> > > > > -         * must also be set */
> > > > > -        env->nip = tswapl((target_ulong) ka->_sa_handler);
> > > > > +        /*
> > > > > +         * ELFv2 PPC64 function pointers are entry points and are in 
> > > > > host
> > > > > +         * endianness so should not to be swapped.
> > > > "target endianness"
> > > > 
> > > > > +         *
> > > > > +         * Note: R12 must also be set.
> > > > > +         */
> > > > > +        env->nip = (target_ulong) ka->_sa_handler;
> > > > The cast is not needed: nip and _sa_handler are abi_ulong.
> > > I'll drop this in v2
> > > 
> > > > >           env->gpr[12] = env->nip;
> > > > >       }
> > > > >   #else
> > > > > 
> > > > If you repost with the fix I've reported above you can add my:
> > > > 
> > > > Reviewed-by: Laurent Vivier <address@hidden>
> > > > 
> > > I'll hold off on reposting until the endianness wording is figured out.
> I'll be submitting v2 shortly, but it will have a different commit message
> to better reflect the issue.
> > > > Thanks,
> > > > Laurent
> > > Thanks,
> > > -Vincent
> > > 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature