[Qemu-devel] [ARM/FDPIC v2 0/4] FDPIC ABI for ARM

[Qemu-devel] [ARM/FDPIC v2 0/4] FDPIC ABI for ARM

[Christophe Lyon]

Hello,

This is v2 of:
https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00783.html

Compared to v1, I have addressed Peter's comments:
- patch #1 removes CONFIG_USE_FDPIC
- patch #2 corresponds to the previous patch #1, and is now simpler
  without configure option
- patch #3 corresponds to the previous patch #2, and uses TaskState
  instead of CPUARMState
- patch #4 corresponds to the previous patch #3, and fixes guest
  pointer dereferencing

Is this now OK?

Thanks,

Christophe

Christophe Lyon (4):
  Remove CONFIG_USE_FDPIC.
  linux-user: ARM-FDPIC: Identify ARM FDPIC binaries
  linux-user: ARM-FDPIC: Add support of FDPIC for ARM.
  linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

 include/elf.h        |  1 +
 linux-user/elfload.c | 53 +++++++++++++++++++++++-------
 linux-user/main.c    |  3 ++
 linux-user/qemu.h    | 10 ++++--
 linux-user/signal.c  | 91 +++++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 129 insertions(+), 29 deletions(-)

-- 
2.6.3

[Qemu-devel] [ARM/FDPIC v2 1/4] Remove CONFIG_USE_FDPIC.

[Christophe Lyon]

We want to avoid code disabled by default, because it ends up less
tested. This patch removes all instances of #ifdef CONFIG_USE_FDPIC,
most of which can be safely kept. For the ones that should be
conditionally executed, we define elf_is_fdpic(). Without this patch,
defining CONFIG_USE_FDPIC would prevent QEMU from building precisely
because elf_is_fdpic is not defined.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index c77ed1b..bbe93b0 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1681,7 +1681,12 @@ static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
     }
 }
 
-#ifdef CONFIG_USE_FDPIC
+/* Default implementation, always false.  */
+static int elf_is_fdpic(struct elfhdr *exec)
+{
+    return 0;
+}
+
 static abi_ulong loader_build_fdpic_loadmap(struct image_info *info, abi_ulong sp)
 {
     uint16_t n;
@@ -1706,7 +1711,6 @@ static abi_ulong loader_build_fdpic_loadmap(struct image_info *info, abi_ulong s
 
     return sp;
 }
-#endif
 
 static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
                                    struct elfhdr *exec,
@@ -1725,7 +1729,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
 
     sp = p;
 
-#ifdef CONFIG_USE_FDPIC
     /* Needs to be before we load the env/argc/... */
     if (elf_is_fdpic(exec)) {
         /* Need 4 byte alignment for these structs */
@@ -1737,7 +1740,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
             sp = loader_build_fdpic_loadmap(interp_info, sp);
         }
     }
-#endif
 
     u_platform = 0;
     k_platform = ELF_PLATFORM;
@@ -2153,10 +2155,8 @@ static void load_elf_image(const char *image_name, int image_fd,
     }
     bswap_phdr(phdr, ehdr->e_phnum);
 
-#ifdef CONFIG_USE_FDPIC
     info->nsegs = 0;
     info->pt_dynamic_addr = 0;
-#endif
 
     mmap_lock();
 
@@ -2173,9 +2173,7 @@ static void load_elf_image(const char *image_name, int image_fd,
             if (a > hiaddr) {
                 hiaddr = a;
             }
-#ifdef CONFIG_USE_FDPIC
             ++info->nsegs;
-#endif
         }
     }
 
@@ -2200,8 +2198,7 @@ static void load_elf_image(const char *image_name, int image_fd,
     }
     load_bias = load_addr - loaddr;
 
-#ifdef CONFIG_USE_FDPIC
-    {
+    if (elf_is_fdpic(ehdr)) {
         struct elf32_fdpic_loadseg *loadsegs = info->loadsegs =
             g_malloc(sizeof(*loadsegs) * info->nsegs);
 
@@ -2219,7 +2216,6 @@ static void load_elf_image(const char *image_name, int image_fd,
             }
         }
     }
-#endif
 
     info->load_bias = load_bias;
     info->load_addr = load_addr;
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 192a0d2..da3b517 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -51,13 +51,13 @@ struct image_info {
         abi_ulong       file_string;
         uint32_t        elf_flags;
 	int		personality;
-#ifdef CONFIG_USE_FDPIC
+
+        /* The fields below are used in FDPIC mode.  */
         abi_ulong       loadmap_addr;
         uint16_t        nsegs;
         void           *loadsegs;
         abi_ulong       pt_dynamic_addr;
         struct image_info *other_info;
-#endif
 };
 
 #ifdef TARGET_I386
-- 
2.6.3

Re: [Qemu-devel] [ARM/FDPIC v2 1/4] Remove CONFIG_USE_FDPIC.

[Peter Maydell]

On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
> We want to avoid code disabled by default, because it ends up less
> tested. This patch removes all instances of #ifdef CONFIG_USE_FDPIC,
> most of which can be safely kept. For the ones that should be
> conditionally executed, we define elf_is_fdpic(). Without this patch,
> defining CONFIG_USE_FDPIC would prevent QEMU from building precisely
> because elf_is_fdpic is not defined.
>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [ARM/FDPIC v2 2/4] linux-user: ARM-FDPIC: Identify ARM FDPIC binaries

[Christophe Lyon]

Define an ARM-specific version of elf_is_fdpic:
FDPIC ELF objects are identified with e_ident[EI_OSABI] ==
ELFOSABI_ARM_FDPIC.

Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
Signed-off-by: Christophe Lyon <christophe.lyon@st.com>

diff --git a/include/elf.h b/include/elf.h
index c0dc9bb..934dbbd 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1483,6 +1483,7 @@ typedef struct elf64_shdr {
 #define ELFOSABI_TRU64          10      /* Compaq TRU64 UNIX.  */
 #define ELFOSABI_MODESTO        11      /* Novell Modesto.  */
 #define ELFOSABI_OPENBSD        12      /* OpenBSD.  */
+#define ELFOSABI_ARM_FDPIC      65      /* ARM FDPIC */
 #define ELFOSABI_ARM            97      /* ARM */
 #define ELFOSABI_STANDALONE     255     /* Standalone (embedded) application */
 
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index bbe93b0..76d7718 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1681,11 +1681,18 @@ static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
     }
 }
 
+#ifdef TARGET_ARM
+static int elf_is_fdpic(struct elfhdr *exec)
+{
+    return exec->e_ident[EI_OSABI] == ELFOSABI_ARM_FDPIC;
+}
+#else
 /* Default implementation, always false.  */
 static int elf_is_fdpic(struct elfhdr *exec)
 {
     return 0;
 }
+#endif
 
 static abi_ulong loader_build_fdpic_loadmap(struct image_info *info, abi_ulong sp)
 {
-- 
2.6.3

Re: [Qemu-devel] [ARM/FDPIC v2 2/4] linux-user: ARM-FDPIC: Identify ARM FDPIC binaries

[Peter Maydell]

On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
> Define an ARM-specific version of elf_is_fdpic:
> FDPIC ELF objects are identified with e_ident[EI_OSABI] ==
> ELFOSABI_ARM_FDPIC.
>
> Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>
> diff --git a/include/elf.h b/include/elf.h
> index c0dc9bb..934dbbd 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -1483,6 +1483,7 @@ typedef struct elf64_shdr {
>  #define ELFOSABI_TRU64          10      /* Compaq TRU64 UNIX.  */
>  #define ELFOSABI_MODESTO        11      /* Novell Modesto.  */
>  #define ELFOSABI_OPENBSD        12      /* OpenBSD.  */
> +#define ELFOSABI_ARM_FDPIC      65      /* ARM FDPIC */
>  #define ELFOSABI_ARM            97      /* ARM */
>  #define ELFOSABI_STANDALONE     255     /* Standalone (embedded) application */
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index bbe93b0..76d7718 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1681,11 +1681,18 @@ static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
>      }
>  }
>
> +#ifdef TARGET_ARM
> +static int elf_is_fdpic(struct elfhdr *exec)
> +{
> +    return exec->e_ident[EI_OSABI] == ELFOSABI_ARM_FDPIC;
> +}
> +#else
>  /* Default implementation, always false.  */
>  static int elf_is_fdpic(struct elfhdr *exec)
>  {
>      return 0;
>  }
> +#endif

I have a strong dislike for per-target ifdef ladders. Can we instead
put the target's implementation of elf_is_fdpic() into
linux-user/$ARCH/target_elf.h
and also have that header do
#define TARGET_HAS_ELF_FDPIC

and then in the generic code we can protect the default elf_is_fdpic()
with #ifndef TARGET_HAS_ELF_FDPIC.

thanks
-- PMM

Re: [Qemu-devel] [ARM/FDPIC v2 2/4] linux-user: ARM-FDPIC: Identify ARM FDPIC binaries

[Christophe Lyon]

On 23/04/2018 14:17, Peter Maydell wrote:
> On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
>> Define an ARM-specific version of elf_is_fdpic:
>> FDPIC ELF objects are identified with e_ident[EI_OSABI] ==
>> ELFOSABI_ARM_FDPIC.
>>
>> Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
>> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>>
>> diff --git a/include/elf.h b/include/elf.h
>> index c0dc9bb..934dbbd 100644
>> --- a/include/elf.h
>> +++ b/include/elf.h
>> @@ -1483,6 +1483,7 @@ typedef struct elf64_shdr {
>>   #define ELFOSABI_TRU64          10      /* Compaq TRU64 UNIX.  */
>>   #define ELFOSABI_MODESTO        11      /* Novell Modesto.  */
>>   #define ELFOSABI_OPENBSD        12      /* OpenBSD.  */
>> +#define ELFOSABI_ARM_FDPIC      65      /* ARM FDPIC */
>>   #define ELFOSABI_ARM            97      /* ARM */
>>   #define ELFOSABI_STANDALONE     255     /* Standalone (embedded) application */
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index bbe93b0..76d7718 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1681,11 +1681,18 @@ static void zero_bss(abi_ulong elf_bss, abi_ulong last_bss, int prot)
>>       }
>>   }
>>
>> +#ifdef TARGET_ARM
>> +static int elf_is_fdpic(struct elfhdr *exec)
>> +{
>> +    return exec->e_ident[EI_OSABI] == ELFOSABI_ARM_FDPIC;
>> +}
>> +#else
>>   /* Default implementation, always false.  */
>>   static int elf_is_fdpic(struct elfhdr *exec)
>>   {
>>       return 0;
>>   }
>> +#endif
> 
> I have a strong dislike for per-target ifdef ladders. Can we instead
> put the target's implementation of elf_is_fdpic() into
> linux-user/$ARCH/target_elf.h
> and also have that header do
> #define TARGET_HAS_ELF_FDPIC
> 
> and then in the generic code we can protect the default elf_is_fdpic()
> with #ifndef TARGET_HAS_ELF_FDPIC.
> 

How invasive could that be?
Your proposal is appealing, but target_elf.h is only included by linux-user/main.c, which does not define elfhdr etc...
All that knowledge is in linux-user/elfload.c, which controls what include/elf.h defines.

Should I re-engineer that?

Thanks,

Christophe


> thanks
> -- PMM
> .
> 

Re: [Qemu-devel] [ARM/FDPIC v2 2/4] linux-user: ARM-FDPIC: Identify ARM FDPIC binaries

[Peter Maydell]

On 23 April 2018 at 13:53, Christophe Lyon <christophe.lyon@st.com> wrote:
> On 23/04/2018 14:17, Peter Maydell wrote:
>> I have a strong dislike for per-target ifdef ladders. Can we instead
>> put the target's implementation of elf_is_fdpic() into
>> linux-user/$ARCH/target_elf.h
>> and also have that header do
>> #define TARGET_HAS_ELF_FDPIC
>>
>> and then in the generic code we can protect the default elf_is_fdpic()
>> with #ifndef TARGET_HAS_ELF_FDPIC.
>>
>
> How invasive could that be?
> Your proposal is appealing, but target_elf.h is only included by
> linux-user/main.c, which does not define elfhdr etc...
> All that knowledge is in linux-user/elfload.c, which controls what
> include/elf.h defines.

Hmm, I see what you mean. We can't even put the function inside the
existing per-target ifdef ladder, because that comes before the
include of elf.h halfway down elfload.c.

That's a bit of a mess, but it's a mess we shouldn't try to
clean up as part of this patchset, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [ARM/FDPIC v2 3/4] linux-user: ARM-FDPIC: Add support of FDPIC for ARM.

[Christophe Lyon]

Add FDPIC info into image_info structure since interpreter info is on
stack and needs to be saved to be accessed later on.

Co-Authored-By:  Mickaël Guêné <mickael.guene@st.com>
Signed-off-by: Christophe Lyon <christophe.lyon@st.com>

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 76d7718..1ee1e38 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -78,6 +78,11 @@ enum {
  */
 #define personality(pers)       (pers & PER_MASK)
 
+int info_is_fdpic(struct image_info *info)
+{
+    return (info->personality == PER_LINUX_FDPIC);
+}
+
 /* this flag is uneffective under linux too, should be deleted */
 #ifndef MAP_DENYWRITE
 #define MAP_DENYWRITE 0
@@ -287,6 +292,24 @@ static inline void init_thread(struct target_pt_regs *regs,
     /* For uClinux PIC binaries.  */
     /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
     regs->uregs[10] = infop->start_data;
+
+    /* Support ARM FDPIC.  */
+    if (info_is_fdpic(infop)) {
+            /* As described in the ABI document, r7 points to the loadmap info
+             * prepared by the kernel. If an interpreter is needed, r8 points
+             * to the interpreter loadmap and r9 points to the interpreter
+             * PT_DYNAMIC info. If no interpreter is needed, r8 is zer0, and
+             * r9 points to the main program PT_DYNAMIC info.  */
+            regs->uregs[7] = infop->loadmap_addr;
+            if (infop->interpreter_loadmap_addr) {
+                /* Executable is dynamically loaded.  */
+                regs->uregs[8] = infop->interpreter_loadmap_addr;
+                regs->uregs[9] = infop->interpreter_pt_dynamic_addr;
+            } else {
+                regs->uregs[8] = 0;
+                regs->uregs[9] = infop->pt_dynamic_addr;
+            }
+        }
 }
 
 #define ELF_NREG    18
@@ -1745,6 +1768,11 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
         if (interp_info) {
             interp_info->other_info = info;
             sp = loader_build_fdpic_loadmap(interp_info, sp);
+            info->interpreter_loadmap_addr = interp_info->loadmap_addr;
+            info->interpreter_pt_dynamic_addr = interp_info->pt_dynamic_addr;
+        } else {
+            info->interpreter_loadmap_addr = 0;
+            info->interpreter_pt_dynamic_addr = 0;
         }
     }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index 2acac36..3579f0e 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4893,6 +4893,9 @@ int main(int argc, char **argv, char **envp)
             env->cp15.sctlr_el[1] |= SCTLR_B;
         }
 #endif
+
+        /* Are we running an FDPIC binary?  */
+        ((TaskState *)thread_cpu->opaque)->is_fdpic = info_is_fdpic(info);
     }
 #elif defined(TARGET_SPARC)
     {
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index da3b517..a2ed148 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -57,6 +57,8 @@ struct image_info {
         uint16_t        nsegs;
         void           *loadsegs;
         abi_ulong       pt_dynamic_addr;
+        abi_ulong       interpreter_loadmap_addr;
+        abi_ulong       interpreter_pt_dynamic_addr;
         struct image_info *other_info;
 };
 
@@ -145,6 +147,9 @@ typedef struct TaskState {
      */
     int signal_pending;
 
+    /* We need to know if we have an FDPIC binary to adapt signal
+     * syscalls.  */
+    int is_fdpic;
 } __attribute__((aligned(16))) TaskState;
 
 extern char *exec_path;
@@ -182,6 +187,7 @@ abi_ulong loader_build_argptr(int envc, int argc, abi_ulong sp,
 int loader_exec(int fdexec, const char *filename, char **argv, char **envp,
              struct target_pt_regs * regs, struct image_info *infop,
              struct linux_binprm *);
+int info_is_fdpic(struct image_info *info);
 
 uint32_t get_elf_eflags(int fd);
 int load_elf_binary(struct linux_binprm *bprm, struct image_info *info);
-- 
2.6.3

Re: [Qemu-devel] [ARM/FDPIC v2 3/4] linux-user: ARM-FDPIC: Add support of FDPIC for ARM.

[Peter Maydell]

On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
> Add FDPIC info into image_info structure since interpreter info is on
> stack and needs to be saved to be accessed later on.
>
> Co-Authored-By:  Mickaël Guêné <mickael.guene@st.com>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 76d7718..1ee1e38 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -78,6 +78,11 @@ enum {
>   */
>  #define personality(pers)       (pers & PER_MASK)
>
> +int info_is_fdpic(struct image_info *info)
> +{
> +    return (info->personality == PER_LINUX_FDPIC);
> +}
> +
>  /* this flag is uneffective under linux too, should be deleted */
>  #ifndef MAP_DENYWRITE
>  #define MAP_DENYWRITE 0
> @@ -287,6 +292,24 @@ static inline void init_thread(struct target_pt_regs *regs,
>      /* For uClinux PIC binaries.  */
>      /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
>      regs->uregs[10] = infop->start_data;
> +
> +    /* Support ARM FDPIC.  */
> +    if (info_is_fdpic(infop)) {
> +            /* As described in the ABI document, r7 points to the loadmap info
> +             * prepared by the kernel. If an interpreter is needed, r8 points
> +             * to the interpreter loadmap and r9 points to the interpreter
> +             * PT_DYNAMIC info. If no interpreter is needed, r8 is zer0, and

"zero"

> +             * r9 points to the main program PT_DYNAMIC info.  */

Something odd seems to have happened to the indentation here.

Nit: */ should be on a line of its own.

> +            regs->uregs[7] = infop->loadmap_addr;
> +            if (infop->interpreter_loadmap_addr) {
> +                /* Executable is dynamically loaded.  */
> +                regs->uregs[8] = infop->interpreter_loadmap_addr;
> +                regs->uregs[9] = infop->interpreter_pt_dynamic_addr;
> +            } else {
> +                regs->uregs[8] = 0;
> +                regs->uregs[9] = infop->pt_dynamic_addr;
> +            }
> +        }
>  }
>
>  #define ELF_NREG    18
> @@ -1745,6 +1768,11 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
>          if (interp_info) {
>              interp_info->other_info = info;
>              sp = loader_build_fdpic_loadmap(interp_info, sp);
> +            info->interpreter_loadmap_addr = interp_info->loadmap_addr;
> +            info->interpreter_pt_dynamic_addr = interp_info->pt_dynamic_addr;
> +        } else {
> +            info->interpreter_loadmap_addr = 0;
> +            info->interpreter_pt_dynamic_addr = 0;
>          }
>      }
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 2acac36..3579f0e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -4893,6 +4893,9 @@ int main(int argc, char **argv, char **envp)
>              env->cp15.sctlr_el[1] |= SCTLR_B;
>          }
>  #endif
> +
> +        /* Are we running an FDPIC binary?  */
> +        ((TaskState *)thread_cpu->opaque)->is_fdpic = info_is_fdpic(info);
>      }
>  #elif defined(TARGET_SPARC)
>      {
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index da3b517..a2ed148 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -57,6 +57,8 @@ struct image_info {
>          uint16_t        nsegs;
>          void           *loadsegs;
>          abi_ulong       pt_dynamic_addr;
> +        abi_ulong       interpreter_loadmap_addr;
> +        abi_ulong       interpreter_pt_dynamic_addr;
>          struct image_info *other_info;
>  };
>
> @@ -145,6 +147,9 @@ typedef struct TaskState {
>       */
>      int signal_pending;
>
> +    /* We need to know if we have an FDPIC binary to adapt signal
> +     * syscalls.  */
> +    int is_fdpic;

Looking at the TaskState struct it already has a pointer
to the image_info struct. So we could just have something like
    bool task_is_fdpic(TaskState *ts)
    {
        return ts->info->personality == PER_LINUX_FDPIC;
    }

and save the need to set up and test a separate flag, I think.

Otherwise looks good.

thanks
-- PMM

Re: [Qemu-devel] [ARM/FDPIC v2 3/4] linux-user: ARM-FDPIC: Add support of FDPIC for ARM.

[Christophe Lyon]

On 23/04/2018 14:49, Peter Maydell wrote:
> On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
>> Add FDPIC info into image_info structure since interpreter info is on
>> stack and needs to be saved to be accessed later on.
>>
>> Co-Authored-By:  Mickaël Guêné <mickael.guene@st.com>
>> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 76d7718..1ee1e38 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -78,6 +78,11 @@ enum {
>>    */
>>   #define personality(pers)       (pers & PER_MASK)
>>
>> +int info_is_fdpic(struct image_info *info)
>> +{
>> +    return (info->personality == PER_LINUX_FDPIC);
>> +}
>> +
>>   /* this flag is uneffective under linux too, should be deleted */
>>   #ifndef MAP_DENYWRITE
>>   #define MAP_DENYWRITE 0
>> @@ -287,6 +292,24 @@ static inline void init_thread(struct target_pt_regs *regs,
>>       /* For uClinux PIC binaries.  */
>>       /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
>>       regs->uregs[10] = infop->start_data;
>> +
>> +    /* Support ARM FDPIC.  */
>> +    if (info_is_fdpic(infop)) {
>> +            /* As described in the ABI document, r7 points to the loadmap info
>> +             * prepared by the kernel. If an interpreter is needed, r8 points
>> +             * to the interpreter loadmap and r9 points to the interpreter
>> +             * PT_DYNAMIC info. If no interpreter is needed, r8 is zer0, and
> 
> "zero"
> 
>> +             * r9 points to the main program PT_DYNAMIC info.  */
> 
> Something odd seems to have happened to the indentation here.
> 
> Nit: */ should be on a line of its own.
> 
>> +            regs->uregs[7] = infop->loadmap_addr;
>> +            if (infop->interpreter_loadmap_addr) {
>> +                /* Executable is dynamically loaded.  */
>> +                regs->uregs[8] = infop->interpreter_loadmap_addr;
>> +                regs->uregs[9] = infop->interpreter_pt_dynamic_addr;
>> +            } else {
>> +                regs->uregs[8] = 0;
>> +                regs->uregs[9] = infop->pt_dynamic_addr;
>> +            }
>> +        }
>>   }
>>
>>   #define ELF_NREG    18
>> @@ -1745,6 +1768,11 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
>>           if (interp_info) {
>>               interp_info->other_info = info;
>>               sp = loader_build_fdpic_loadmap(interp_info, sp);
>> +            info->interpreter_loadmap_addr = interp_info->loadmap_addr;
>> +            info->interpreter_pt_dynamic_addr = interp_info->pt_dynamic_addr;
>> +        } else {
>> +            info->interpreter_loadmap_addr = 0;
>> +            info->interpreter_pt_dynamic_addr = 0;
>>           }
>>       }
>>
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index 2acac36..3579f0e 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -4893,6 +4893,9 @@ int main(int argc, char **argv, char **envp)
>>               env->cp15.sctlr_el[1] |= SCTLR_B;
>>           }
>>   #endif
>> +
>> +        /* Are we running an FDPIC binary?  */
>> +        ((TaskState *)thread_cpu->opaque)->is_fdpic = info_is_fdpic(info);
>>       }
>>   #elif defined(TARGET_SPARC)
>>       {
>> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
>> index da3b517..a2ed148 100644
>> --- a/linux-user/qemu.h
>> +++ b/linux-user/qemu.h
>> @@ -57,6 +57,8 @@ struct image_info {
>>           uint16_t        nsegs;
>>           void           *loadsegs;
>>           abi_ulong       pt_dynamic_addr;
>> +        abi_ulong       interpreter_loadmap_addr;
>> +        abi_ulong       interpreter_pt_dynamic_addr;
>>           struct image_info *other_info;
>>   };
>>
>> @@ -145,6 +147,9 @@ typedef struct TaskState {
>>        */
>>       int signal_pending;
>>
>> +    /* We need to know if we have an FDPIC binary to adapt signal
>> +     * syscalls.  */
>> +    int is_fdpic;
> 
> Looking at the TaskState struct it already has a pointer
> to the image_info struct. So we could just have something like
>      bool task_is_fdpic(TaskState *ts)
>      {
>          return ts->info->personality == PER_LINUX_FDPIC;
>      }
> 
> and save the need to set up and test a separate flag, I think.
> 

This patch defines and uses info_is_fdpic, the next one in the
series uses information for TaskState, so I suppose it's OK
not to add this new flag as you suggest, and in the next patch
use info_is_fdpic(ts->info) instead of adding task_is_fdpic() ?

Thanks,

Christophe

> Otherwise looks good.
> 
> thanks
> -- PMM
> .
> 

Re: [Qemu-devel] [ARM/FDPIC v2 3/4] linux-user: ARM-FDPIC: Add support of FDPIC for ARM.

[Peter Maydell]

On 23 April 2018 at 15:13, Christophe Lyon <christophe.lyon@st.com> wrote:
> On 23/04/2018 14:49, Peter Maydell wrote:
> This patch defines and uses info_is_fdpic, the next one in the
> series uses information for TaskState, so I suppose it's OK
> not to add this new flag as you suggest, and in the next patch
> use info_is_fdpic(ts->info) instead of adding task_is_fdpic() ?

Yes, that works too.

thanks
-- PMM

[Qemu-devel] [ARM/FDPIC v2 4/4] linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

[Christophe Lyon]

The FDPIC restorer needs to deal with a function descriptor, hence we
have to extend 'retcode' such that it can hold the instructions needed
to perform this.

The restorer sequence uses the same thumbness as the exception
handler (mainly to support Thumb-only architectures).

Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
Signed-off-by: Christophe Lyon <christophe.lyon@st.com>

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 8d9e6e8..d01b459 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -2045,13 +2045,13 @@ struct sigframe_v1
 {
     struct target_sigcontext sc;
     abi_ulong extramask[TARGET_NSIG_WORDS-1];
-    abi_ulong retcode;
+    abi_ulong retcode[4];
 };
 
 struct sigframe_v2
 {
     struct target_ucontext_v2 uc;
-    abi_ulong retcode;
+    abi_ulong retcode[4];
 };
 
 struct rt_sigframe_v1
@@ -2060,14 +2060,14 @@ struct rt_sigframe_v1
     abi_ulong puc;
     struct target_siginfo info;
     struct target_ucontext_v1 uc;
-    abi_ulong retcode;
+    abi_ulong retcode[4];
 };
 
 struct rt_sigframe_v2
 {
     struct target_siginfo info;
     struct target_ucontext_v2 uc;
-    abi_ulong retcode;
+    abi_ulong retcode[4];
 };
 
 #define TARGET_CONFIG_CPU_32 1
@@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = {
 	SWI_SYS_RT_SIGRETURN,	SWI_THUMB_RT_SIGRETURN
 };
 
+/*
+ * Stub needed to make sure the FD register (r9) contains the right
+ * value.
+ */
+static const unsigned long sigreturn_fdpic_codes[3] = {
+    0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */
+    0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */
+    0xe59cf000  /* ldr pc, [r12] to jump into restorer */
+};
+
+static const unsigned long sigreturn_fdpic_thumb_codes[3] = {
+    0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */
+    0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */
+    0xf000f8dc  /* ldr pc, [r12] to jump into restorer */
+};
 
 static inline int valid_user_regs(CPUARMState *regs)
 {
@@ -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;
+        get_user_ual(handler, funcdesc_ptr);
+    }
+    thumb = handler & 1;
+
     uint32_t cpsr = cpsr_read(env);
 
     cpsr &= ~CPSR_IT;
@@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
     }
 
     if (ka->sa_flags & TARGET_SA_RESTORER) {
-        retcode = ka->sa_restorer;
-    } else {
-        unsigned int idx = thumb;
+        if (is_fdpic) {
+            /* For FDPIC we ensure that the restorer is called with a
+             * correct r9 value.  For that we need to write code on
+             * the stack that sets r9 and jumps back to restorer
+             * value.
+             */
+            if (thumb) {
+                __put_user(sigreturn_fdpic_thumb_codes[0], rc);
+                __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1);
+                __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2);
+                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
+            } else {
+                __put_user(sigreturn_fdpic_codes[0], rc);
+                __put_user(sigreturn_fdpic_codes[1], rc + 1);
+                __put_user(sigreturn_fdpic_codes[2], rc + 2);
+                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
+            }
 
-        if (ka->sa_flags & TARGET_SA_SIGINFO) {
-            idx += 2;
+            retcode = rc_addr + thumb;
+        } else {
+            retcode = ka->sa_restorer;
         }
+    } else {
+        if (is_fdpic) {
+            qemu_log_mask(LOG_UNIMP,
+                          "arm: FDPIC signal return not implemented");
+            abort();
+        } else {
+            unsigned int idx = thumb;
+
+            if (ka->sa_flags & TARGET_SA_SIGINFO) {
+                idx += 2;
+            }
 
-        __put_user(retcodes[idx], rc);
+            __put_user(retcodes[idx], rc);
 
-        retcode = rc_addr + thumb;
+            retcode = rc_addr + thumb;
+        }
     }
 
     env->regs[0] = usig;
+    if (is_fdpic) {
+        get_user_ual(env->regs[9], funcdesc_ptr + 4);
+    }
     env->regs[13] = frame_addr;
     env->regs[14] = retcode;
     env->regs[15] = handler & (thumb ? ~1 : ~3);
@@ -2270,7 +2329,7 @@ static void setup_frame_v1(int usig, struct target_sigaction *ka,
         __put_user(set->sig[i], &frame->extramask[i - 1]);
     }
 
-    setup_return(regs, ka, &frame->retcode, frame_addr, usig,
+    setup_return(regs, ka, frame->retcode, frame_addr, usig,
                  frame_addr + offsetof(struct sigframe_v1, retcode));
 
     unlock_user_struct(frame, frame_addr, 1);
@@ -2292,7 +2351,7 @@ static void setup_frame_v2(int usig, struct target_sigaction *ka,
 
     setup_sigframe_v2(&frame->uc, set, regs);
 
-    setup_return(regs, ka, &frame->retcode, frame_addr, usig,
+    setup_return(regs, ka, frame->retcode, frame_addr, usig,
                  frame_addr + offsetof(struct sigframe_v2, retcode));
 
     unlock_user_struct(frame, frame_addr, 1);
@@ -2347,7 +2406,7 @@ static void setup_rt_frame_v1(int usig, struct target_sigaction *ka,
         __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
     }
 
-    setup_return(env, ka, &frame->retcode, frame_addr, usig,
+    setup_return(env, ka, frame->retcode, frame_addr, usig,
                  frame_addr + offsetof(struct rt_sigframe_v1, retcode));
 
     env->regs[1] = info_addr;
@@ -2378,7 +2437,7 @@ static void setup_rt_frame_v2(int usig, struct target_sigaction *ka,
 
     setup_sigframe_v2(&frame->uc, set, env);
 
-    setup_return(env, ka, &frame->retcode, frame_addr, usig,
+    setup_return(env, ka, frame->retcode, frame_addr, usig,
                  frame_addr + offsetof(struct rt_sigframe_v2, retcode));
 
     env->regs[1] = info_addr;
-- 
2.6.3

Re: [Qemu-devel] [ARM/FDPIC v2 4/4] linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

[Peter Maydell]

On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
> The FDPIC restorer needs to deal with a function descriptor, hence we
> have to extend 'retcode' such that it can hold the instructions needed
> to perform this.
>
> The restorer sequence uses the same thumbness as the exception
> handler (mainly to support Thumb-only architectures).
>
> Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8d9e6e8..d01b459 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -2045,13 +2045,13 @@ struct sigframe_v1
>  {
>      struct target_sigcontext sc;
>      abi_ulong extramask[TARGET_NSIG_WORDS-1];
> -    abi_ulong retcode;
> +    abi_ulong retcode[4];
>  };
>
>  struct sigframe_v2
>  {
>      struct target_ucontext_v2 uc;
> -    abi_ulong retcode;
> +    abi_ulong retcode[4];
>  };
>
>  struct rt_sigframe_v1
> @@ -2060,14 +2060,14 @@ struct rt_sigframe_v1
>      abi_ulong puc;
>      struct target_siginfo info;
>      struct target_ucontext_v1 uc;
> -    abi_ulong retcode;
> +    abi_ulong retcode[4];
>  };
>
>  struct rt_sigframe_v2
>  {
>      struct target_siginfo info;
>      struct target_ucontext_v2 uc;
> -    abi_ulong retcode;
> +    abi_ulong retcode[4];
>  };

I was slightly surprised to find that the kernel adds the extra
retcode slots to the signal frame unconditionally, but it does,
so we can too, which is nice.

>  #define TARGET_CONFIG_CPU_32 1
> @@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = {
>         SWI_SYS_RT_SIGRETURN,   SWI_THUMB_RT_SIGRETURN
>  };
>
> +/*
> + * Stub needed to make sure the FD register (r9) contains the right
> + * value.
> + */
> +static const unsigned long sigreturn_fdpic_codes[3] = {
> +    0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */
> +    0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */
> +    0xe59cf000  /* ldr pc, [r12] to jump into restorer */
> +};
> +
> +static const unsigned long sigreturn_fdpic_thumb_codes[3] = {
> +    0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */
> +    0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */
> +    0xf000f8dc  /* ldr pc, [r12] to jump into restorer */
> +};
>
>  static inline int valid_user_regs(CPUARMState *regs)
>  {
> @@ -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.

> +        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.

> +    }
> +    thumb = handler & 1;
> +
>      uint32_t cpsr = cpsr_read(env);
>
>      cpsr &= ~CPSR_IT;
> @@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>      }
>
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
> -        retcode = ka->sa_restorer;
> -    } else {
> -        unsigned int idx = thumb;
> +        if (is_fdpic) {
> +            /* For FDPIC we ensure that the restorer is called with a
> +             * correct r9 value.  For that we need to write code on
> +             * the stack that sets r9 and jumps back to restorer
> +             * value.
> +             */
> +            if (thumb) {
> +                __put_user(sigreturn_fdpic_thumb_codes[0], rc);
> +                __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1);
> +                __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2);
> +                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
> +            } else {
> +                __put_user(sigreturn_fdpic_codes[0], rc);
> +                __put_user(sigreturn_fdpic_codes[1], rc + 1);
> +                __put_user(sigreturn_fdpic_codes[2], rc + 2);
> +                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
> +            }
>
> -        if (ka->sa_flags & TARGET_SA_SIGINFO) {
> -            idx += 2;
> +            retcode = rc_addr + thumb;
> +        } else {
> +            retcode = ka->sa_restorer;
>          }
> +    } else {
> +        if (is_fdpic) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "arm: FDPIC signal return not implemented");
> +            abort();

The kernel setup_return() doesn't seem to do anything special
for this case -- why do we need to abort() ?

> +        } else {
> +            unsigned int idx = thumb;
> +
> +            if (ka->sa_flags & TARGET_SA_SIGINFO) {
> +                idx += 2;
> +            }
>
> -        __put_user(retcodes[idx], rc);
> +            __put_user(retcodes[idx], rc);
>
> -        retcode = rc_addr + thumb;
> +            retcode = rc_addr + thumb;
> +        }
>      }
>
>      env->regs[0] = usig;
> +    if (is_fdpic) {
> +        get_user_ual(env->regs[9], funcdesc_ptr + 4);

This also needs to check for unreadable memory. You might find it
easier to read both halves of the descriptor at the same point
in the code, the same way the kernel's setup_return() does.

> +    }

thanks
-- PMM

Re: [Qemu-devel] [ARM/FDPIC v2 4/4] linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

[Christophe Lyon]

On 23/04/2018 15:05, Peter Maydell wrote:
> On 23 April 2018 at 08:51, Christophe Lyon <christophe.lyon@st.com> wrote:
>> The FDPIC restorer needs to deal with a function descriptor, hence we
>> have to extend 'retcode' such that it can hold the instructions needed
>> to perform this.
>>
>> The restorer sequence uses the same thumbness as the exception
>> handler (mainly to support Thumb-only architectures).
>>
>> Co-Authored-By: Mickaël Guêné <mickael.guene@st.com>
>> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index 8d9e6e8..d01b459 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -2045,13 +2045,13 @@ struct sigframe_v1
>>   {
>>       struct target_sigcontext sc;
>>       abi_ulong extramask[TARGET_NSIG_WORDS-1];
>> -    abi_ulong retcode;
>> +    abi_ulong retcode[4];
>>   };
>>
>>   struct sigframe_v2
>>   {
>>       struct target_ucontext_v2 uc;
>> -    abi_ulong retcode;
>> +    abi_ulong retcode[4];
>>   };
>>
>>   struct rt_sigframe_v1
>> @@ -2060,14 +2060,14 @@ struct rt_sigframe_v1
>>       abi_ulong puc;
>>       struct target_siginfo info;
>>       struct target_ucontext_v1 uc;
>> -    abi_ulong retcode;
>> +    abi_ulong retcode[4];
>>   };
>>
>>   struct rt_sigframe_v2
>>   {
>>       struct target_siginfo info;
>>       struct target_ucontext_v2 uc;
>> -    abi_ulong retcode;
>> +    abi_ulong retcode[4];
>>   };
> 
> I was slightly surprised to find that the kernel adds the extra
> retcode slots to the signal frame unconditionally, but it does,
> so we can too, which is nice.
> 
>>   #define TARGET_CONFIG_CPU_32 1
>> @@ -2090,6 +2090,21 @@ static const abi_ulong retcodes[4] = {
>>          SWI_SYS_RT_SIGRETURN,   SWI_THUMB_RT_SIGRETURN
>>   };
>>
>> +/*
>> + * Stub needed to make sure the FD register (r9) contains the right
>> + * value.
>> + */
>> +static const unsigned long sigreturn_fdpic_codes[3] = {
>> +    0xe59fc004, /* ldr r12, [pc, #4] to read function descriptor */
>> +    0xe59c9004, /* ldr r9, [r12, #4] to setup GOT */
>> +    0xe59cf000  /* ldr pc, [r12] to jump into restorer */
>> +};
>> +
>> +static const unsigned long sigreturn_fdpic_thumb_codes[3] = {
>> +    0xc008f8df, /* ldr r12, [pc, #8] to read function descriptor */
>> +    0x9004f8dc, /* ldr r9, [r12, #4] to setup GOT */
>> +    0xf000f8dc  /* ldr pc, [r12] to jump into restorer */
>> +};
>>
>>   static inline int valid_user_regs(CPUARMState *regs)
>>   {
>> @@ -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.

>> +        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?

>> +    }
>> +    thumb = handler & 1;
>> +
>>       uint32_t cpsr = cpsr_read(env);
>>
>>       cpsr &= ~CPSR_IT;
>> @@ -2160,20 +2189,50 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>>       }
>>
>>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>> -        retcode = ka->sa_restorer;
>> -    } else {
>> -        unsigned int idx = thumb;
>> +        if (is_fdpic) {
>> +            /* For FDPIC we ensure that the restorer is called with a
>> +             * correct r9 value.  For that we need to write code on
>> +             * the stack that sets r9 and jumps back to restorer
>> +             * value.
>> +             */
>> +            if (thumb) {
>> +                __put_user(sigreturn_fdpic_thumb_codes[0], rc);
>> +                __put_user(sigreturn_fdpic_thumb_codes[1], rc + 1);
>> +                __put_user(sigreturn_fdpic_thumb_codes[2], rc + 2);
>> +                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
>> +            } else {
>> +                __put_user(sigreturn_fdpic_codes[0], rc);
>> +                __put_user(sigreturn_fdpic_codes[1], rc + 1);
>> +                __put_user(sigreturn_fdpic_codes[2], rc + 2);
>> +                __put_user((abi_ulong)ka->sa_restorer, rc + 3);
>> +            }
>>
>> -        if (ka->sa_flags & TARGET_SA_SIGINFO) {
>> -            idx += 2;
>> +            retcode = rc_addr + thumb;
>> +        } else {
>> +            retcode = ka->sa_restorer;
>>           }
>> +    } else {
>> +        if (is_fdpic) {
>> +            qemu_log_mask(LOG_UNIMP,
>> +                          "arm: FDPIC signal return not implemented");
>> +            abort();
> 
> The kernel setup_return() doesn't seem to do anything special
> for this case -- why do we need to abort() ?
> 
I guess that's a leftover for debugging. I'll remove it.

>> +        } else {
>> +            unsigned int idx = thumb;
>> +
>> +            if (ka->sa_flags & TARGET_SA_SIGINFO) {
>> +                idx += 2;
>> +            }
>>
>> -        __put_user(retcodes[idx], rc);
>> +            __put_user(retcodes[idx], rc);
>>
>> -        retcode = rc_addr + thumb;
>> +            retcode = rc_addr + thumb;
>> +        }
>>       }
>>
>>       env->regs[0] = usig;
>> +    if (is_fdpic) {
>> +        get_user_ual(env->regs[9], funcdesc_ptr + 4);
> 
> This also needs to check for unreadable memory. You might find it
> easier to read both halves of the descriptor at the same point
> in the code, the same way the kernel's setup_return() does.
> 

Indeed.

Thanks,
Christophe

>> +    }
> 
> thanks
> -- PMM
> .
> 

Re: [Qemu-devel] [ARM/FDPIC v2 4/4] linux-user: ARM-FDPIC: Add support for signals for FDPIC targets

[Peter Maydell]

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