arm64: extable: fix null deref in load_unaligned_zeropad.

ex_handler_load_unaligned_zeropad extracts the source and data register numbers from the wrong field of the exception table.

Fixes 753b3236

ex_handler_load_unaligned_zeropad extracts the source and data register numbers from the wrong field of the exception table.

Fixes 753b3236

Signed-off-by Evgenii Stepanov <[email protected]>

---
 arch/arm64/mm/extable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/mm/extable.c +2 -2

index c0181e60cc98..489455309695 100644
@@ -40,8 +40,8 @@ static bool
 ex_handler_load_unaligned_zeropad(const struct exception_table_entry *ex,
 				  struct pt_regs *regs)
 {
-	int reg_data = FIELD_GET(EX_DATA_REG_DATA, ex->type);
-	int reg_addr = FIELD_GET(EX_DATA_REG_ADDR, ex->type);
+	int reg_data = FIELD_GET(EX_DATA_REG_DATA, ex->data);
+	int reg_addr = FIELD_GET(EX_DATA_REG_ADDR, ex->data);
 	unsigned long data, addr, offset;
 
 	addr = pt_regs_read_reg(regs, reg_addr);

Signature · 1 line

2.35.0.rc0.227.g00780c9af4-goog

Quoted 2 lines · Evgenii Stepanov

ex_handler_load_unaligned_zeropad extracts the source and data register numbers from the wrong field of the exception table.

Ouch. Did you find this by inspection, or did this show up in testing?

Sorry about this.

I think we should be a little more explicit as to exactly what goes wrong. How about:

| In ex_handler_load_unaligned_zeropad() we erroneously extract the data and
| addr register indices from ex->type rather than ex->data. As ex->type will
| contain EX_TYPE_LOAD_UNALIGNED_ZEROPAD (i.e. 4):
| 
| * We'll always treat X0 as the address register, since EX_DATA_REG_ADDR is
|   extracted from bits [9:5]. Thus, we may attempt to dereference an arbitrary
|   address as X0 may hold an arbitary value.
| 
| * We'll always treat X4 as the data register, since EX_DATA_REG_DATA is
|   extracted from bits [4:0]. Thus we will corrupt X4 and cause arbitrary
|   behaviour within load_unaligned_zeropad() and its caller.
| 
| Fix this by extracting both values from ex->data as originally intended.

Quoted 1 line

Fixes 753b3236

That should be expanded, e.g.

Fixes 753b32368705c396 ("arm64: extable: add load_unaligned_zeropad() handler")

With those changes:

Reviewed-by Mark Rutland <[email protected]>

Quoted 31 lines · Mark Rutland

Quoted 2 lines · Evgenii Stepanov

ex_handler_load_unaligned_zeropad extracts the source and data register numbers from the wrong field of the exception table.

Ouch. Did you find this by inspection, or did this show up in testing?

Sorry about this.

I think we should be a little more explicit as to exactly what goes wrong. How about:

| In ex_handler_load_unaligned_zeropad() we erroneously extract the data and
| addr register indices from ex->type rather than ex->data. As ex->type will
| contain EX_TYPE_LOAD_UNALIGNED_ZEROPAD (i.e. 4):
|
| * We'll always treat X0 as the address register, since EX_DATA_REG_ADDR is
|   extracted from bits [9:5]. Thus, we may attempt to dereference an arbitrary
|   address as X0 may hold an arbitary value.
|
| * We'll always treat X4 as the data register, since EX_DATA_REG_DATA is
|   extracted from bits [4:0]. Thus we will corrupt X4 and cause arbitrary
|   behaviour within load_unaligned_zeropad() and its caller.
|
| Fix this by extracting both values from ex->data as originally intended.

Quoted 1 line

Fixes 753b3236

That should be expanded, e.g.

Fixes 753b32368705c396 ("arm64: extable: add load_unaligned_zeropad() handler")

Quoted 3 lines

With those changes:

Reviewed-by Mark Rutland <[email protected]>

Looking again, sicne this isn't jsut a null-deref, can we also rework the title, something like:

| arm64: extable: fix load_unaligned_zeropad() reg indices

Thanks,
Mark.

Quoted 40 lines · Mark Rutland

Quoted 27 lines · Mark Rutland

Quoted 2 lines · Evgenii Stepanov

ex_handler_load_unaligned_zeropad extracts the source and data register numbers from the wrong field of the exception table.

Ouch. Did you find this by inspection, or did this show up in testing?

Sorry about this.

I think we should be a little more explicit as to exactly what goes wrong. How about:

| In ex_handler_load_unaligned_zeropad() we erroneously extract the data and
| addr register indices from ex->type rather than ex->data. As ex->type will
| contain EX_TYPE_LOAD_UNALIGNED_ZEROPAD (i.e. 4):
|
| * We'll always treat X0 as the address register, since EX_DATA_REG_ADDR is
|   extracted from bits [9:5]. Thus, we may attempt to dereference an arbitrary
|   address as X0 may hold an arbitary value.
|
| * We'll always treat X4 as the data register, since EX_DATA_REG_DATA is
|   extracted from bits [4:0]. Thus we will corrupt X4 and cause arbitrary
|   behaviour within load_unaligned_zeropad() and its caller.
|
| Fix this by extracting both values from ex->data as originally intended.

Quoted 1 line

Fixes 753b3236

Quoted 7 lines

That should be expanded, e.g.

Fixes 753b32368705c396 ("arm64: extable: add load_unaligned_zeropad() handler")

With those changes:

Reviewed-by Mark Rutland <[email protected]>

Looking again, sicne this isn't jsut a null-deref, can we also rework the title, something like:

Quoted 1 line

| arm64: extable: fix load_unaligned_zeropad() reg indices

That's a much better commit message, thank you! I'll upload v2 shortly.

This was found by updating to a newer QEMU that correctly delivers MTE faults from unaligned memory accesses, and triggers this bug reliably during Android boot. I'll add a stack trace to v2.