[ipxe-devel] [PATCH 1/1] [efi] avoid unaligned read in efi_devpath_end()

[Heinrich Schuchardt]

The old coding resulted in a "data abort" when compiled with gcc 6.3 for
armhf and run on an Allwinner A20 SOC.

The unaligned access occured when path->Length was on an uneven address.

Signed-off-by: Heinrich Schuchardt <***@gmx.de>
---
src/interface/efi/efi_utils.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/interface/efi/efi_utils.c b/src/interface/efi/efi_utils.c
index 4dc75414..dd59613b 100644
--- a/src/interface/efi/efi_utils.c
+++ b/src/interface/efi/efi_utils.c
@@ -39,12 +39,15 @@ FILE_LICENCE ( GPL2_OR_LATER );
* @ret path_end End of device path
*/
EFI_DEVICE_PATH_PROTOCOL * efi_devpath_end ( EFI_DEVICE_PATH_PROTOCOL *path ) {
+ EFI_DEVICE_PATH_PROTOCOL path_c;

- while ( path->Type != END_DEVICE_PATH_TYPE ) {
+ memcpy(&path_c, path, sizeof(EFI_DEVICE_PATH_PROTOCOL));
+ while ( path_c.Type != END_DEVICE_PATH_TYPE ) {
path = ( ( ( void * ) path ) +
/* There's this amazing new-fangled thing known as
* a UINT16, but who wants to use one of those? */
- ( ( path->Length[1] << 8 ) | path->Length[0] ) );
+ ( ( path_c.Length[1] << 8 ) | path_c.Length[0] ) );
+ memcpy(&path_c, path, sizeof(EFI_DEVICE_PATH_PROTOCOL));
}

return path;

[Michael Brown]

Post by Heinrich Schuchardt
The old coding resulted in a "data abort" when compiled with gcc 6.3 for
armhf and run on an Allwinner A20 SOC.
The unaligned access occured when path->Length was on an uneven address.
- ( ( path->Length[1] << 8 ) | path->Length[0] ) );

should ever be able to produce an unaligned access abort, since it just
dereferences individual bytes. What do you see if you disassemble the
object code?

Michael

[Heinrich Schuchardt]

Post by Michael Brown

Post by Heinrich Schuchardt
The old coding resulted in a "data abort" when compiled with gcc 6.3 for
armhf and run on an Allwinner A20 SOC.
The unaligned access occured when path->Length was on an uneven address.
-             ( ( path->Length[1] << 8 ) | path->Length[0] ) );

should ever be able to produce an unaligned access abort, since it just
dereferences individual bytes.  What do you see if you disassemble the
object code?
Michael

Hello Michael,

I put a DGBC before and after the access. And this is were the interrupt
occurs.

I do not know how to disassemble the object code. Which files do you need?

Best regards

Heinrich

[Michael Brown]

Post by Heinrich Schuchardt

Post by Michael Brown

Post by Heinrich Schuchardt
-             ( ( path->Length[1] << 8 ) | path->Length[0] ) );

should ever be able to produce an unaligned access abort, since it just
dereferences individual bytes.  What do you see if you disassemble the
object code?

I put a DGBC before and after the access. And this is were the interrupt
occurs.
I do not know how to disassemble the object code. Which files do you need?

You should be able to use:

objdump -dS bin-arm32-efi/efi_utils.o

Michael

[Heinrich Schuchardt]

Post by Heinrich Schuchardt

Post by Michael Brown

Post by Heinrich Schuchardt
-             ( ( path->Length[1] << 8 ) | path->Length[0] ) );

should ever be able to produce an unaligned access abort, since it just
dereferences individual bytes.  What do you see if you disassemble the
object code?

I put a DGBC before and after the access. And this is were the interrupt
occurs.
I do not know how to disassemble the object code. Which files do you need?

  objdump -dS bin-arm32-efi/efi_utils.o
Michael

00000000 <efi_devpath_end>:
0: 7803 ldrb r3, [r0, #0] <<< Reading on byte
2: 2b7f cmp r3, #127 ; 0x7f
4: d100 bne.n 8 <efi_devpath_end+0x8>
6: 4770 bx lr
8: 8843 ldrh r3, [r0, #2] <<< Reading two bytes
infocenter.arm.com/help/topic/com.arm.doc.faqs/ka15414.html
LDRH/STRH - address must be 2-byte aligned.

a: 4418 add r0, r3
c: e7f8 b.n 0 <efi_devpath_end>

Regards

Heinrich

[Michael Brown]

Post by Heinrich Schuchardt
0: 7803 ldrb r3, [r0, #0] <<< Reading on byte
2: 2b7f cmp r3, #127 ; 0x7f
4: d100 bne.n 8 <efi_devpath_end+0x8>
6: 4770 bx lr
8: 8843 ldrh r3, [r0, #2] <<< Reading two bytes
infocenter.arm.com/help/topic/com.arm.doc.faqs/ka15414.html
LDRH/STRH - address must be 2-byte aligned.
a: 4418 add r0, r3
c: e7f8 b.n 0 <efi_devpath_end>

Thanks. The compiler is indeed creating a single ldrh instruction.
This indicates that the compiler believes that unaligned accesses are
permitted, and so is optimising away the two byte loads to a single word
load.

You can try building with -mno-unaligned-access; this should cause the
compiler to emit byte-by-byte accesses for anything that is potentially
unaligned. This is likely to substantially increase the code size, and
decrease execution speed.

As in my other e-mail: your best option is probably to enable the MMU
and fix up unaligned accesses as they occur. That way you will at least
avoid the penalty for accesses that are correctly aligned.

Michael

[Heinrich Schuchardt]

    0:   7803            ldrb    r3, [r0, #0]  <<< Reading on byte
    2:   2b7f            cmp     r3, #127        ; 0x7f
    4:   d100            bne.n   8 <efi_devpath_end+0x8>
    6:   4770            bx      lr
    8:   8843            ldrh    r3, [r0, #2]  <<< Reading two bytes
infocenter.arm.com/help/topic/com.arm.doc.faqs/ka15414.html
LDRH/STRH - address must be 2-byte aligned.
    a:   4418            add     r0, r3
    c:   e7f8            b.n     0 <efi_devpath_end>

Thanks.  The compiler is indeed creating a single ldrh instruction. This
indicates that the compiler believes that unaligned accesses are
permitted, and so is optimising away the two byte loads to a single word
load.
You can try building with -mno-unaligned-access; this should cause the
compiler to emit byte-by-byte accesses for anything that is potentially
unaligned.  This is likely to substantially increase the code size, and
decrease execution speed.

00000000 <efi_devpath_end>:
0: 7803 ldrb r3, [r0, #0]
2: 2b7f cmp r3, #127 ; 0x7f
4: d100 bne.n 8 <efi_devpath_end+0x8>
6: 4770 bx lr
8: 7883 ldrb r3, [r0, #2]
a: 78c2 ldrb r2, [r0, #3]
c: ea43 2302 orr.w r3, r3, r2, lsl #8
10: 4418 add r0, r3
12: e7f5 b.n 0 <efi_devpath_end>

That looks better.

The GCC documentations says: "By default unaligned access is disabled
for all pre-ARMv6, all ARMv6-M and for ARMv8-M Baseline architectures,
and enabled for all other architectures."

I just sent you the corresponding patch.

As in my other e-mail: your best option is probably to enable the MMU
and fix up unaligned accesses as they occur.  That way you will at least
avoid the penalty for accesses that are correctly aligned.

I don't think that all ARM 32bit MMUs can fix the problem.

We receive an interrupt due to unaligned access. Possibly the interrupt
handler could analyze the failed machine code, execute the load, and
continue. But that is tons of code.

Best regards

Heinrich

[Heinrich Schuchardt]

Post by Heinrich Schuchardt
The old coding resulted in a "data abort" when compiled with gcc 6.3 for
armhf and run on an Allwinner A20 SOC.
The unaligned access occured when path->Length was on an uneven address.
---
src/interface/efi/efi_utils.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/interface/efi/efi_utils.c b/src/interface/efi/efi_utils.c
index 4dc75414..dd59613b 100644
--- a/src/interface/efi/efi_utils.c
+++ b/src/interface/efi/efi_utils.c
@@ -39,12 +39,15 @@ FILE_LICENCE ( GPL2_OR_LATER );
*/
EFI_DEVICE_PATH_PROTOCOL * efi_devpath_end ( EFI_DEVICE_PATH_PROTOCOL *path ) {
+ EFI_DEVICE_PATH_PROTOCOL path_c;
- while ( path->Type != END_DEVICE_PATH_TYPE ) {
+ memcpy(&path_c, path, sizeof(EFI_DEVICE_PATH_PROTOCOL));
+ while ( path_c.Type != END_DEVICE_PATH_TYPE ) {
path = ( ( ( void * ) path ) +
/* There's this amazing new-fangled thing known as
* a UINT16, but who wants to use one of those? */
- ( ( path->Length[1] << 8 ) | path->Length[0] ) );
+ ( ( path_c.Length[1] << 8 ) | path_c.Length[0] ) );
+ memcpy(&path_c, path, sizeof(EFI_DEVICE_PATH_PROTOCOL));
}
return path;

Hello Michael,

with the patch above I reach the iPXE prompt on a BananaPi. But when
executing the dhcp command I see another "data abort", see below.

I am trying to track down, in which routine this happens. The error
occurs in monojob_wait(). Where do I find the job-code that is executed
in monojob_wait?

Best regards

Heinrich

iPXE> dhcp


hci/commands/ifmgmt_cmd.c(219) ifconf_payload:
SNP net0 could not set station address before initialising: Error
0x7f594083 (http://ipxe.org/7f594083)
***@01c50000 Waiting for PHY auto negotiation to complete...... done
Speed: 1000, full duplex


SNP net0 could not set station address after initialising: Error
0x7f594083 (http://ipxe.org/7f594083)
SNP net0 could not set receive filters 0x00 (have 0x00): Error
0x7f594083 (http://ipxe.org/7f594083)

SNP net0 could not set receive filters 0x07 (have 0x00): Error
0x7f594083 (http://ipxe.org/7f594083)

SNP net0 could not set receive filters 0x05 (have 0x00): Error
0x7f594083 (http://ipxe.org/7f594083)

SNP net0 could not set receive filters 0x01 (have 0x00): Error
0x7f594083 (http://ipxe.org/7f594083)

SNPDEV 0x79eaaa64 link is up
net/udp/dhcp.c(1348) start_dhcp:
INTF 0x79eab014+18 opening (SOCK_DGRAM,AF_INET) socket


net/udp/dhcp.c(1354) start_dhcp:


DHCP 0x79eab014 entering discovery state
net/udp/dhcp.c(1356) start_dhcp:
Configuring (net0 02:49:04:c2:32:99)usr/ifmgmt.c(300) ifconf:
usr/ifmgmt.c(189) ifpoller_wait:
core/monojob.c(97) monojob_wait:
...data abort
pc : [<79e979a8>] lr : [<79e83713>]
reloc pc : [<44f329a8>] lr : [<44f1e713>]
sp : 7af3a438 ip : 7af3a448 fp : 7af3a458
r10: 00000005 r9 : 7af3a4c4 r8 : 00000000
r7 : 00000000 r6 : 79ea9ee8 r5 : 79e978c9 r4 : 7af3a4c4
r3 : 79eaa9f0 r2 : 00000005 r1 : 00000000 r0 : 7af3a4c4
Flags: nZCv IRQs off FIQs off Mode SVC_32
Resetting CPU ...

resetting ...

[Michael Brown]

Post by Heinrich Schuchardt
with the patch above I reach the iPXE prompt on a BananaPi. But when
executing the dhcp command I see another "data abort", see below.
I am trying to track down, in which routine this happens. The error
occurs in monojob_wait(). Where do I find the job-code that is executed
in monojob_wait?

I'm not particularly surprised. The UEFI spec apparently[*] requires
the MMU to be enabled and for unaligned accesses to be permitted. The
iPXE code therefore assumes that it can make unaligned accesses to
network packet contents, and that this will just result in a slowdown
rather than a crash.

Given the number of places at which an unaligned access could occur, I
think you will probably need to enable the MMU.

[*] according to various people at ARM that I spoke to while creating
the ARM port of iPXE

Michael