[PATCH] aarch64: Don't assert on long system registers

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] aarch64: Don't assert on long system registers

Alex Coplan
Hello,

This patch fixes an assertion failure on long system register operands
in the AArch64 backend. See the new testcase for an input which
reproduces the issue.

Testing:
 * New test which fails before and passes after the patch.
 * Testsuite run on x64 -> aarch64-none-elf build with no regressions.

OK for master?

Thanks,
Alex

---

2020-07-21  Alex Coplan  <[hidden email]>

gas/ChangeLog:

        * config/tc-aarch64.c (parse_sys_reg): Reject system registers longer
        than 31 bytes instead instead of asserting.
        * testsuite/gas/aarch64/invalid-sysreg-assert.d: New test.
        * testsuite/gas/aarch64/invalid-sysreg-assert.l: Error output.
        * testsuite/gas/aarch64/invalid-sysreg-assert.s: Input.

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] aarch64: Don't assert on long system registers

Sourceware - binutils list mailing list
Hi Alex,

> OK for master?

Sorry - I do not like this assumption:

  /* Assume that our buffer is big enough for any valid system register.  */

You never know - one day there might be an extra long system register name.

I think that it would be better to provide a definition for the maximum
length include/opcode/aarch64.h and then use it the code you are patching.
Something like:

  aarch64.h:

   #define MAX_SYSREG_NAME_LEN 32
   typedef struct
   {
      const char     name[MAX_SYSREG_NAME_LEN];
      aarch64_insn   value;
      uint32_t     flags;
   [....]


  tc-aarch64.c:

    static int
    parse_sys_reg (char **str, struct hash_control *sys_regs,
               int imple_defined_p, int pstatefield_p,
               uint32_t* flags)
    {
      char *p, *q;
      char buf[MAX_SYSREG_NAME_LEN];

      [...]

      /* If the name is longer than our buffer then it cannot be valid.  */
      if (p - buf != q - *str)
        return PARSE_FAIL;


  If an extra long system register name is ever defined then the
  initialisation code in opcodes/aarch64-opc.c should fail (at
  compile time) and the #define can be increased to accommodate the
  new maximum length.

Cheers
  Nick

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] aarch64: Don't assert on long system registers

Alex Coplan
Hi Nick,

Sorry for the delay; just got back from some time off.

> -----Original Message-----
> From: Nick Clifton <[hidden email]>
> Sent: 22 July 2020 15:38
> To: Alex Coplan <[hidden email]>; [hidden email]
> Cc: Richard Earnshaw <[hidden email]>; Marcus Shawcroft
> <[hidden email]>
> Subject: Re: [PATCH] aarch64: Don't assert on long system registers
>
> > OK for master?
>
> Sorry - I do not like this assumption:
>
>   /* Assume that our buffer is big enough for any valid system register.
> */
OK, that seems reasonable.

>
> You never know - one day there might be an extra long system register
> name.
>
> I think that it would be better to provide a definition for the maximum
> length include/opcode/aarch64.h and then use it the code you are
> patching.
> Something like:
>
>   aarch64.h:
>
>    #define MAX_SYSREG_NAME_LEN 32
>    typedef struct
>    {
>       const char     name[MAX_SYSREG_NAME_LEN];
>       aarch64_insn   value;
>       uint32_t     flags;
>    [....]
>
>
>   tc-aarch64.c:
>
>     static int
>     parse_sys_reg (char **str, struct hash_control *sys_regs,
>       int imple_defined_p, int pstatefield_p,
>       uint32_t* flags)
>     {
>       char *p, *q;
>       char buf[MAX_SYSREG_NAME_LEN];
>
>       [...]
>
>       /* If the name is longer than our buffer then it cannot be valid.
> */
>       if (p - buf != q - *str)
>         return PARSE_FAIL;
>
>
>   If an extra long system register name is ever defined then the
>   initialisation code in opcodes/aarch64-opc.c should fail (at
>   compile time) and the #define can be increased to accommodate the
>   new maximum length.
Please see the reworked patch attached. I had a go at using a fixed-size array
for the name field but this ended up being quite an invasive change since we
tend to assume that this field is a pointer. This also ends up wasting space in
the binary.

With this patch I'm proposing an alternative approach which is to assert that
the system register names are no longer than AARCH64_MAX_SYSREG_NAME_LEN when
they are inserted into the hash table.

While this won't catch someone adding an unusually-long system register at
compile time, the resulting GAS binary will crash unconditionally on startup,
which will ensure that this gets caught quickly in any case.

Testing:
 * Regression tested on aarch64-none-elf.

Is the updated patch OK for master?

Thanks,
Alex

---

gas/ChangeLog:

2020-08-04  Alex Coplan  <[hidden email]>

        * config/tc-aarch64.c (parse_sys_reg): Don't assert when parsing
        a long system register.
        (parse_sys_ins_reg): Likewise.
        (sysreg_hash_insert): New.
        (md_begin): Use sysreg_hash_insert() to ensure all system
        registers are no longer than the maximum length at startup.
        * testsuite/gas/aarch64/invalid-sysreg-assert.d: New test.
        * testsuite/gas/aarch64/invalid-sysreg-assert.l: Error output.
        * testsuite/gas/aarch64/invalid-sysreg-assert.s: Input.

include/ChangeLog:

2020-08-04  Alex Coplan  <[hidden email]>

        * opcode/aarch64.h (AARCH64_MAX_SYSREG_NAME_LEN): New.


patch_v2.txt (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] aarch64: Don't assert on long system registers

Sourceware - binutils list mailing list
Hi Alex,

> Please see the reworked patch attached. I had a go at using a fixed-size array
> for the name field but this ended up being quite an invasive change since we
> tend to assume that this field is a pointer. This also ends up wasting space in
> the binary.
>
> With this patch I'm proposing an alternative approach which is to assert that
> the system register names are no longer than AARCH64_MAX_SYSREG_NAME_LEN when
> they are inserted into the hash table.
>
> While this won't catch someone adding an unusually-long system register at
> compile time, the resulting GAS binary will crash unconditionally on startup,
> which will ensure that this gets caught quickly in any case.
>
> Testing:
>  * Regression tested on aarch64-none-elf.
>
> Is the updated patch OK for master?

Nice.  Thanks for taking the time to rework the patch.  Approved - please apply.

Cheers
  Nick