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

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

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

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  <alex.coplan@arm.com>

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.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1782 bytes --]

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index ecb15d23437..aad0c4fb6f3 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -4106,11 +4106,13 @@ parse_sys_reg (char **str, struct hash_control *sys_regs,
 
   p = buf;
   for (q = *str; ISALNUM (*q) || *q == '_'; q++)
-    if (p < buf + 31)
+    if (p < buf + (sizeof (buf) - 1))
       *p++ = TOLOWER (*q);
   *p = '\0';
-  /* Assert that BUF be large enough.  */
-  gas_assert (p - buf == q - *str);
+
+  /* Assume that our buffer is big enough for any valid system register.  */
+  if (p - buf != q - *str)
+    return PARSE_FAIL;
 
   o = hash_find (sys_regs, buf);
   if (!o)
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d
new file mode 100644
index 00000000000..a6279bbe7c2
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d
@@ -0,0 +1,3 @@
+#name: don't assert on long system register
+#source: invalid-sysreg-assert.s
+#error_output: invalid-sysreg-assert.l
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l
new file mode 100644
index 00000000000..b6049108b07
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+.*: Error: unknown or missing system register name at operand 1 -- `msr 00000000000000000000000000000000'
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s
new file mode 100644
index 00000000000..8b3706fd9cd
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s
@@ -0,0 +1,2 @@
+// This input caused an assertion failure in parse_sys_reg.
+msr 00000000000000000000000000000000

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

[Nick Clifton]

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

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

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

Hi Nick,

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

> -----Original Message-----
> From: Nick Clifton <nickc@redhat.com>
> Sent: 22 July 2020 15:38
> To: Alex Coplan <Alex.Coplan@arm.com>; binutils@sourceware.org
> Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>
> 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  <alex.coplan@arm.com>

	* 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  <alex.coplan@arm.com>

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


[-- Attachment #2: patch_v2.txt --]
[-- Type: text/plain, Size: 5356 bytes --]

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index ecb15d2343..460061df4a 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -4100,17 +4100,21 @@ parse_sys_reg (char **str, struct hash_control *sys_regs,
 	       uint32_t* flags)
 {
   char *p, *q;
-  char buf[32];
+  char buf[AARCH64_MAX_SYSREG_NAME_LEN];
   const aarch64_sys_reg *o;
   int value;
 
   p = buf;
   for (q = *str; ISALNUM (*q) || *q == '_'; q++)
-    if (p < buf + 31)
+    if (p < buf + (sizeof (buf) - 1))
       *p++ = TOLOWER (*q);
   *p = '\0';
-  /* Assert that BUF be large enough.  */
-  gas_assert (p - buf == q - *str);
+
+  /* If the name is longer than AARCH64_MAX_SYSREG_NAME_LEN then it cannot be a
+     valid system register.  This is enforced by construction of the hash
+     table.  */
+  if (p - buf != q - *str)
+    return PARSE_FAIL;
 
   o = hash_find (sys_regs, buf);
   if (!o)
@@ -4159,15 +4163,21 @@ static const aarch64_sys_ins_reg *
 parse_sys_ins_reg (char **str, struct hash_control *sys_ins_regs)
 {
   char *p, *q;
-  char buf[32];
+  char buf[AARCH64_MAX_SYSREG_NAME_LEN];
   const aarch64_sys_ins_reg *o;
 
   p = buf;
   for (q = *str; ISALNUM (*q) || *q == '_'; q++)
-    if (p < buf + 31)
+    if (p < buf + (sizeof (buf) - 1))
       *p++ = TOLOWER (*q);
   *p = '\0';
 
+  /* If the name is longer than AARCH64_MAX_SYSREG_NAME_LEN then it cannot be a
+     valid system register.  This is enforced by construction of the hash
+     table.  */
+  if (p - buf != q - *str)
+    return NULL;
+
   o = hash_find (sys_ins_regs, buf);
   if (!o)
     return NULL;
@@ -8612,6 +8622,13 @@ checked_hash_insert (struct hash_control *table, const char *key, void *value)
     printf ("Internal Error:  Can't hash %s\n", key);
 }
 
+static void
+sysreg_hash_insert (struct hash_control *table, const char *key, void *value)
+{
+  gas_assert (strlen (key) < AARCH64_MAX_SYSREG_NAME_LEN);
+  checked_hash_insert (table, key, value);
+}
+
 static void
 fill_instruction_hash_table (void)
 {
@@ -8686,36 +8703,36 @@ md_begin (void)
   fill_instruction_hash_table ();
 
   for (i = 0; aarch64_sys_regs[i].name != NULL; ++i)
-    checked_hash_insert (aarch64_sys_regs_hsh, aarch64_sys_regs[i].name,
+    sysreg_hash_insert (aarch64_sys_regs_hsh, aarch64_sys_regs[i].name,
 			 (void *) (aarch64_sys_regs + i));
 
   for (i = 0; aarch64_pstatefields[i].name != NULL; ++i)
-    checked_hash_insert (aarch64_pstatefield_hsh,
+    sysreg_hash_insert (aarch64_pstatefield_hsh,
 			 aarch64_pstatefields[i].name,
 			 (void *) (aarch64_pstatefields + i));
 
   for (i = 0; aarch64_sys_regs_ic[i].name != NULL; i++)
-    checked_hash_insert (aarch64_sys_regs_ic_hsh,
+    sysreg_hash_insert (aarch64_sys_regs_ic_hsh,
 			 aarch64_sys_regs_ic[i].name,
 			 (void *) (aarch64_sys_regs_ic + i));
 
   for (i = 0; aarch64_sys_regs_dc[i].name != NULL; i++)
-    checked_hash_insert (aarch64_sys_regs_dc_hsh,
+    sysreg_hash_insert (aarch64_sys_regs_dc_hsh,
 			 aarch64_sys_regs_dc[i].name,
 			 (void *) (aarch64_sys_regs_dc + i));
 
   for (i = 0; aarch64_sys_regs_at[i].name != NULL; i++)
-    checked_hash_insert (aarch64_sys_regs_at_hsh,
+    sysreg_hash_insert (aarch64_sys_regs_at_hsh,
 			 aarch64_sys_regs_at[i].name,
 			 (void *) (aarch64_sys_regs_at + i));
 
   for (i = 0; aarch64_sys_regs_tlbi[i].name != NULL; i++)
-    checked_hash_insert (aarch64_sys_regs_tlbi_hsh,
+    sysreg_hash_insert (aarch64_sys_regs_tlbi_hsh,
 			 aarch64_sys_regs_tlbi[i].name,
 			 (void *) (aarch64_sys_regs_tlbi + i));
 
   for (i = 0; aarch64_sys_regs_sr[i].name != NULL; i++)
-    checked_hash_insert (aarch64_sys_regs_sr_hsh,
+    sysreg_hash_insert (aarch64_sys_regs_sr_hsh,
 			 aarch64_sys_regs_sr[i].name,
 			 (void *) (aarch64_sys_regs_sr + i));
 
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d
new file mode 100644
index 0000000000..a6279bbe7c
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.d
@@ -0,0 +1,3 @@
+#name: don't assert on long system register
+#source: invalid-sysreg-assert.s
+#error_output: invalid-sysreg-assert.l
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l
new file mode 100644
index 0000000000..b6049108b0
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+.*: Error: unknown or missing system register name at operand 1 -- `msr 00000000000000000000000000000000'
diff --git a/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s
new file mode 100644
index 0000000000..8b3706fd9c
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/invalid-sysreg-assert.s
@@ -0,0 +1,2 @@
+// This input caused an assertion failure in parse_sys_reg.
+msr 00000000000000000000000000000000
diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
index 1e6ea191c3..82cba13a94 100644
--- a/include/opcode/aarch64.h
+++ b/include/opcode/aarch64.h
@@ -943,6 +943,8 @@ extern const struct aarch64_name_value_pair aarch64_barrier_options [16];
 extern const struct aarch64_name_value_pair aarch64_prfops [32];
 extern const struct aarch64_name_value_pair aarch64_hint_options [];
 
+#define AARCH64_MAX_SYSREG_NAME_LEN 32
+
 typedef struct
 {
   const char *  name;

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

[Nick Clifton]

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