[PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Kees Cook]

When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware
floating-point registers (NT_ARM_VFP) are available. If the inferior
uses hard-float, do not request soft-float registers (NT_PRFPREG) and
run the risk of failing with EINVAL. This is most noticeably exposed
when running "generate-core-file":

(gdb) generate-core-file myprog.core
Unable to fetch the floating point registers.: Invalid argument.

ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument)

gdb/ChangeLog:

2016-12-02  Kees Cook  <keescook@google.com>

	* gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float.

---
 gdb/arm-linux-nat.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index d11bdc6..2126cd7 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops,
   if (-1 == regno)
     {
       fetch_regs (regcache);
-      fetch_fpregs (regcache);
       if (tdep->have_wmmx_registers)
 	fetch_wmmx_regs (regcache);
       if (tdep->vfp_register_count > 0)
 	fetch_vfp_regs (regcache);
+      else
+	fetch_fpregs (regcache);
     }
-  else 
+  else
     {
       if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
 	fetch_regs (regcache);
-      else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
+      else if (tdep->vfp_register_count == 0
+	       && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
 	fetch_fpregs (regcache);
       else if (tdep->have_wmmx_registers
 	       && regno >= ARM_WR0_REGNUM && regno <= ARM_WCGR7_REGNUM)
@@ -420,17 +422,19 @@ arm_linux_store_inferior_registers (struct target_ops *ops,
   if (-1 == regno)
     {
       store_regs (regcache);
-      store_fpregs (regcache);
       if (tdep->have_wmmx_registers)
 	store_wmmx_regs (regcache);
       if (tdep->vfp_register_count > 0)
 	store_vfp_regs (regcache);
+      else
+	store_fpregs (regcache);
     }
   else
     {
       if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
 	store_regs (regcache);
-      else if ((regno >= ARM_F0_REGNUM) && (regno <= ARM_FPS_REGNUM))
+      else if (tdep->vfp_register_count == 0
+	       && (regno >= ARM_F0_REGNUM) && (regno <= ARM_FPS_REGNUM))
 	store_fpregs (regcache);
       else if (tdep->have_wmmx_registers
 	       && regno >= ARM_WR0_REGNUM && regno <= ARM_WCGR7_REGNUM)
-- 
2.7.4


-- 
Kees Cook
Nexus Security

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Yao Qi]

On 16-12-02 13:46:13, Kees Cook wrote:
> When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware
> floating-point registers (NT_ARM_VFP) are available. If the inferior
> uses hard-float, do not request soft-float registers (NT_PRFPREG) and
> run the risk of failing with EINVAL. This is most noticeably exposed

"soft-float" is not accurate.  FPA is coprocessor.  Both VFP and FPA is
implemented in the combination of software and hardware.  I'd like to
rewrite the commit log like this,

"When running a 32-bit ARM inferior with a 32-bit ARM GDB on 64-bit
AArch64 host, only VFP registers (NT_ARM_VFP) are available.  The FPA
registers (NT_PRFPREG) is not available."

> when running "generate-core-file":
> 
> (gdb) generate-core-file myprog.core
> Unable to fetch the floating point registers.: Invalid argument.
> 
> ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument)
> 
> gdb/ChangeLog:
> 
> 2016-12-02  Kees Cook  <keescook@google.com>

You don't have FSF copyright assignment.

> 
> 	* gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float.
> 
> ---
>  gdb/arm-linux-nat.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index d11bdc6..2126cd7 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops,
>    if (-1 == regno)
>      {
>        fetch_regs (regcache);
> -      fetch_fpregs (regcache);

We should only call fetch_fpregs if tdep->have_fpa_registers is true.

>        if (tdep->have_wmmx_registers)
>  	fetch_wmmx_regs (regcache);
>        if (tdep->vfp_register_count > 0)
>  	fetch_vfp_regs (regcache);
> +      else
> +	fetch_fpregs (regcache);
>      }
> -  else 
> +  else
>      {
>        if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
>  	fetch_regs (regcache);
> -      else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
> +      else if (tdep->vfp_register_count == 0
> +	       && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
>  	fetch_fpregs (regcache);

Do we really need this change?  If FPA registers are not available,
REGNO can't fall in this range (ARM_F0_REGNUM, ARM_FPS_REGNUM).

These two comments above also apply to store registers.

-- 
Yao 

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Kees Cook]

On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On 16-12-02 13:46:13, Kees Cook wrote:
>> When running a 32-bit ARM inferior on a 64-bit ARM host, only the hardware
>> floating-point registers (NT_ARM_VFP) are available. If the inferior
>> uses hard-float, do not request soft-float registers (NT_PRFPREG) and
>> run the risk of failing with EINVAL. This is most noticeably exposed
>
> "soft-float" is not accurate.  FPA is coprocessor.  Both VFP and FPA is
> implemented in the combination of software and hardware.  I'd like to
> rewrite the commit log like this,
>
> "When running a 32-bit ARM inferior with a 32-bit ARM GDB on 64-bit
> AArch64 host, only VFP registers (NT_ARM_VFP) are available.  The FPA
> registers (NT_PRFPREG) is not available."

That would be fine by me. I was kind of scratching my head over the
naming of the types of floating-point registers. :) Whatever the case,
arm64 doesn't support FPA, so an inferior using FPA couldn't run there
to start with. :)

>> when running "generate-core-file":
>>
>> (gdb) generate-core-file myprog.core
>> Unable to fetch the floating point registers.: Invalid argument.
>>
>> ptrace(PTRACE_GETREGSET, 27642, NT_FPREGSET, 0xffcc67f0) = -1 EINVAL (Invalid argument)
>>
>> gdb/ChangeLog:
>>
>> 2016-12-02  Kees Cook  <keescook@google.com>
>
> You don't have FSF copyright assignment.

Oh, hm, I thought there might be a Google-contributions-to-gdb one I
was already covered under. What's the best approach for me to take to
fix this?

>>       * gdb/arm-linux-nat.c: Skip soft-float registers when using hard-float.
>>
>> ---
>>  gdb/arm-linux-nat.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
>> index d11bdc6..2126cd7 100644
>> --- a/gdb/arm-linux-nat.c
>> +++ b/gdb/arm-linux-nat.c
>> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops,
>>    if (-1 == regno)
>>      {
>>        fetch_regs (regcache);
>> -      fetch_fpregs (regcache);
>
> We should only call fetch_fpregs if tdep->have_fpa_registers is true.

I couldn't determine how this was handled. What actually sets
org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and
seems to imply it's always included with arm? I wasn't able to follow,
but it seemed like _having_ VFP was a indicator that FPA wasn't used.

>
>>        if (tdep->have_wmmx_registers)
>>       fetch_wmmx_regs (regcache);
>>        if (tdep->vfp_register_count > 0)
>>       fetch_vfp_regs (regcache);
>> +      else
>> +     fetch_fpregs (regcache);
>>      }
>> -  else
>> +  else
>>      {
>>        if (regno < ARM_F0_REGNUM || regno == ARM_PS_REGNUM)
>>       fetch_regs (regcache);
>> -      else if (regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
>> +      else if (tdep->vfp_register_count == 0
>> +            && regno >= ARM_F0_REGNUM && regno <= ARM_FPS_REGNUM)
>>       fetch_fpregs (regcache);
>
> Do we really need this change?  If FPA registers are not available,
> REGNO can't fall in this range (ARM_F0_REGNUM, ARM_FPS_REGNUM).
>
> These two comments above also apply to store registers.

It seemed like a reasonable change to make, but I see your point.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Doug Evans]

On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On 16-12-02 13:46:13, Kees Cook wrote:
>> gdb/ChangeLog:
>>
>> 2016-12-02  Kees Cook  <keescook@google.com>
>
> You don't have FSF copyright assignment.

Hi.
Google has a blanket copyright assignment, so this is ok in that regard.

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Yao Qi]

On Fri, Dec 2, 2016 at 11:49 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Dec 2, 2016 at 2:49 PM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> On 16-12-02 13:46:13, Kees Cook wrote:
>>> gdb/ChangeLog:
>>>
>>> 2016-12-02  Kees Cook  <keescook@google.com>
>>
>> You don't have FSF copyright assignment.
>
> Hi.
> Google has a blanket copyright assignment, so this is ok in that regard.

Hi Doug,
I am bad at parsing /gd/gnuorg/copyright.list :(, you are right.  I find it now.

-- 
Yao (齐尧)

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Yao Qi]

On Fri, Dec 2, 2016 at 11:08 PM, Kees Cook <keescook@chromium.org> wrote:
>>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
>>> index d11bdc6..2126cd7 100644
>>> --- a/gdb/arm-linux-nat.c
>>> +++ b/gdb/arm-linux-nat.c
>>> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops,
>>>    if (-1 == regno)
>>>      {
>>>        fetch_regs (regcache);
>>> -      fetch_fpregs (regcache);
>>
>> We should only call fetch_fpregs if tdep->have_fpa_registers is true.
>
> I couldn't determine how this was handled. What actually sets
> org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and
> seems to imply it's always included with arm? I wasn't able to follow,
> but it seemed like _having_ VFP was a indicator that FPA wasn't used.
>

What is I meant is that instead of calling fetch_fpregs unconditionally,
we call fetch_fpregs if tdep->have_fpa_registers is true, like this,

  if (tdep->have_fpa_registers)
     fetch_fpregs (regcache);

IOW, we only fetch FPA registers if we know FPA registers are available,
as described in target description.

-- 
Yao (齐尧)

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Kees Cook]

On Sun, Dec 4, 2016 at 3:11 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 11:08 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
>>>> index d11bdc6..2126cd7 100644
>>>> --- a/gdb/arm-linux-nat.c
>>>> +++ b/gdb/arm-linux-nat.c
>>>> @@ -384,17 +384,19 @@ arm_linux_fetch_inferior_registers (struct target_ops *ops,
>>>>    if (-1 == regno)
>>>>      {
>>>>        fetch_regs (regcache);
>>>> -      fetch_fpregs (regcache);
>>>
>>> We should only call fetch_fpregs if tdep->have_fpa_registers is true.
>>
>> I couldn't determine how this was handled. What actually sets
>> org.gnu.gdb.arm.fpa in tdesc? I found gdb/features/arm/arm-fpa.xml and
>> seems to imply it's always included with arm? I wasn't able to follow,
>> but it seemed like _having_ VFP was a indicator that FPA wasn't used.
>>
>
> What is I meant is that instead of calling fetch_fpregs unconditionally,
> we call fetch_fpregs if tdep->have_fpa_registers is true, like this,
>
>   if (tdep->have_fpa_registers)
>      fetch_fpregs (regcache);
>
> IOW, we only fetch FPA registers if we know FPA registers are available,
> as described in target description.

Right, I was asking how the target description is built. I couldn't
determine where FPA was detected.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Yao Qi]

On 16-12-04 11:30:36, Kees Cook wrote:
> Right, I was asking how the target description is built. I couldn't
> determine where FPA was detected.
>

It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature
org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is
found.

-- 
Yao 

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Kees Cook]

On Mon, Dec 5, 2016 at 2:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> On 16-12-04 11:30:36, Kees Cook wrote:
>> Right, I was asking how the target description is built. I couldn't
>> determine where FPA was detected.
>>
>
> It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature
> org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is
> found.

Yup, that part I found. What sets org.gnu.gdb.arm.fpa?

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] Fix PTRACE_GETREGSET failure for compat inferiors on arm64

[Yao Qi]

On 16-12-05 08:06:06, Kees Cook wrote:
> On Mon, Dec 5, 2016 at 2:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> > On 16-12-04 11:30:36, Kees Cook wrote:
> >> Right, I was asking how the target description is built. I couldn't
> >> determine where FPA was detected.
> >>
> >
> > It is detected in arm-tdep.c:arm_gdbarch_init, which looks for feature
> > org.gnu.gdb.arm.fpa, and set have_fpa_registers to 1 if this feature is
> > found.
> 
> Yup, that part I found. What sets org.gnu.gdb.arm.fpa?
> 

The remote stub may know the architecture support FPA, and reply the
target description with feature org.gnu.gdb.arm.fpa to GDB.

-- 
Yao (齐尧)