[PATCH] aarch64: Don't assert on long system registers
Alex Coplan
alex.coplan@arm.com
Tue Aug 4 09:15:59 GMT 2020
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.
-------------- next part --------------
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;
More information about the Binutils
mailing list