Skip to content

runtime/libtock_layout.ld: ASSERT well-aligned RAM address#477

Merged
bors[bot] merged 1 commit intotock:masterfrom
lschuermann:dev/linker-align-assertion
Jul 10, 2023
Merged

runtime/libtock_layout.ld: ASSERT well-aligned RAM address#477
bors[bot] merged 1 commit intotock:masterfrom
lschuermann:dev/linker-align-assertion

Conversation

@lschuermann
Copy link
Copy Markdown
Member

@lschuermann lschuermann commented Jun 29, 2023

Rust's default linker (llvm-ld) as used with the Rust toolchain versions of at least 2022-06-10, 2023-01-26, and 2023-06-27 can produce broken ELF binaries when the RAM region's start address is not well-aligned to a 4kB boundary. Unfortunately, this behavior is rather tricky to debug: instead of refusing to produce a binary or producing a corrupt output, it generates an ELF file which includes a segment that points to the ELF file's header itself. elf2tab will include this segment in the final binary (as it is set to be loaded), producing broken TBFs. This (overrideable) check is designed to warn users that the linker may be misbehaved under these conditions.

To reproduce this on this current git revision, with a Rust toolchain version 2023-06-27, change the opentitan.ld layout file to add an offset of 0x800 bytes to the RAM segment's start address:

@@ -5,7 +5,7 @@ MEMORY {
    * the kernel binary, check for the actual address of APP_MEMORY!
    */
   FLASH (X) : ORIGIN = 0x20030000, LENGTH = 32M
-  RAM   (W) : ORIGIN = 0x10004000, LENGTH = 512K
+  RAM   (W) : ORIGIN = 0x10004000 + 0x800, LENGTH = 512K - 0x800
 }

 TBF_HEADER_SIZE = 0x60;

Further, remove the .tbf_header reservation:

@@ -67,7 +67,7 @@ SECTIONS {
     /* Add a section where elf2tab will place the TBF headers, so that the rest
      * of the FLASH sections are in the right locations. */ .tbf_header (NOLOAD) : {
-        . = . + TBF_HEADER_SIZE;
+        /* . = . + TBF_HEADER_SIZE; */
     } > FLASH

     /* Runtime header. Contains values the linker knows that the runtime needs

Now, compile the console example for the opentitan target. When looking at the generated ELF file, it contains a segment at (an ELF-file) offset 0x000000, set to be LOADed, with a non-zero size. This segment thus points to the ELF file header itself.

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x10004034 0x10004034 0x000c0 0x000c0 R   0x4
  LOAD           0x000000 0x10004000 0x10004000 0x000f4 0x000f4 R   0x1000
  LOAD           0x001000 0x20030000 0x20030000 0x00d64 0x00d64 R E 0x1000
  LOAD           0x001d64 0x20030d64 0x20030d64 0x001ec 0x001ec R   0x1000
  LOAD           0x002800 0x10004800 0x20030f50 0x00100 0x00100 RW  0x1000
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

 Section to Segment mapping:
  Segment Sections...
   00
   01
   02     .start .text
   03     .rodata
   04     .stack
   05

When switching to the GNU LD instead, we get a correct ELF file. For that, make the following change to .cargo/config:

@@ -9,6 +9,8 @@ rtv7em = "rthumbv7em"
 # Common settings for all embedded targets
 [target.'cfg(any(target_arch = "arm", target_arch = "riscv32"))']
 rustflags = [
+    "-C", "linker=riscv64-unknown-elf-ld",
+    "-C", "link-arg=-melf32lriscv",
     "-C", "relocation-model=static",
     "-C", "link-arg=-Tlayout.ld",
 ]

Further, change the .start section's alignment, as GNU LD does not support specifying a section alignment constraint without a preceding section:

@@ -73,7 +73,8 @@ SECTIONS {
     /* Runtime header. Contains values the linker knows that the runtime needs
      * to look up. */
-    .start ALIGN(4) : {
+    . = ALIGN(4);
+    .start : {
         /* We combine rt_header and _start into a single section. If we don't,
          * elf2tab does not parse the ELF file correctly for unknown reasons.
          */

This is the segment layout of the resulting binary, which is correct and no longer references the ELF header itself:

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x10004000 0x10004000 0x00094 0x00900 RW  0x1000
  LOAD           0x001000 0x20030000 0x20030000 0x00f50 0x00f50 R E 0x1000
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x10

 Section to Segment mapping:
  Segment Sections...
   00     .stack
   01     .start .text .rodata
   02

(In this case, the .stack section is included in the final binary in a segment of size 0x900, to account for the segment alignment constraints. This also points to the ELF-internal offset of 0x000000, so referencing the ELF header. Yet this is fine in this case, as the .stack symbol within that region is beyond the allocated MemSiz offset (at an offset of 0x800), so zero-initialized as intended. The .stack should not at all end in the final binary, and never at a PhysAddr / load address within the RAM section; however this is an orthogonal issue.)

Rust's default linker (llvm-ld) as used with the Rust toolchain versions of at
least `2022-06-10`, `2023-01-26`, and `2023-06-27` can produce broken ELF
binaries when the RAM region's start address is not well-aligned to a 4kB
boundary. Unfortunately, this behavior is rather tricky to debug: instead of
refusing to produce a binary or producing a corrupt output, it generates an ELF
file which includes a segment that points to the ELF file's header
itself. elf2tab will include this segment in the final binary (as it is set to
be loaded), producing broken TBFs. This (overrideable) check is designed to warn
users that the linker may be misbehaved under these conditions.

To reproduce this on this current git revision, with a Rust toolchain version
`2023-06-27`, change the `opentitan.ld` layout file to add an offset of 0x800
bytes to the RAM segment's start address:

    @@ -5,7 +5,7 @@ MEMORY {
        * the kernel binary, check for the actual address of APP_MEMORY!
        */
       FLASH (X) : ORIGIN = 0x20030000, LENGTH = 32M
    -  RAM   (W) : ORIGIN = 0x10004000, LENGTH = 512K
    +  RAM   (W) : ORIGIN = 0x10004000 + 0x800, LENGTH = 512K - 0x800
     }

     TBF_HEADER_SIZE = 0x60;

Further, remove the `.tbf_header` reservation:

    @@ -67,7 +67,7 @@ SECTIONS {
         /* Add a section where elf2tab will place the TBF headers, so that the rest
          * of the FLASH sections are in the right locations. */
         .tbf_header (NOLOAD) : {
    -        . = . + TBF_HEADER_SIZE;
    +        /* . = . + TBF_HEADER_SIZE; */
         } > FLASH

         /* Runtime header. Contains values the linker knows that the runtime needs

Now, compile the `console` example for the `opentitan` target. When looking at
the generated ELF file, it contains a segment at (an ELF-file) offset
`0x000000`, set to be `LOAD`ed, with a non-zero size. This segment thus points
to the ELF file header itself.

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      PHDR           0x000034 0x10004034 0x10004034 0x000c0 0x000c0 R   0x4
      LOAD           0x000000 0x10004000 0x10004000 0x000f4 0x000f4 R   0x1000
      LOAD           0x001000 0x20030000 0x20030000 0x00d64 0x00d64 R E 0x1000
      LOAD           0x001d64 0x20030d64 0x20030d64 0x001ec 0x001ec R   0x1000
      LOAD           0x002800 0x10004800 0x20030f50 0x00100 0x00100 RW  0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

     Section to Segment mapping:
      Segment Sections...
       00
       01
       02     .start .text
       03     .rodata
       04     .stack
       05

When switching to the GNU LD instead, we get a correct ELF file. For that, make
the following change to `.cargo/config`:

    @@ -9,6 +9,8 @@ rtv7em = "rthumbv7em"
     # Common settings for all embedded targets
     [target.'cfg(any(target_arch = "arm", target_arch = "riscv32"))']
     rustflags = [
    +    "-C", "linker=riscv64-unknown-elf-ld",
    +    "-C", "link-arg=-melf32lriscv",
         "-C", "relocation-model=static",
         "-C", "link-arg=-Tlayout.ld",
     ]

Further, change the `.start` section's alignment, as GNU LD does not support
specifying a section alignment constraint without a preceding section:

    @@ -73,7 +73,8 @@ SECTIONS {
         /* Runtime header. Contains values the linker knows that the runtime needs
          * to look up.
          */
    -    .start ALIGN(4) : {
    +    . = ALIGN(4);
    +    .start : {
             /* We combine rt_header and _start into a single section. If we don't,
              * elf2tab does not parse the ELF file correctly for unknown reasons.
              */

This is the segment layout of the resulting binary, which is correct and no
longer references the ELF header itself:

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x000000 0x10004000 0x10004000 0x00094 0x00900 RW  0x1000
      LOAD           0x001000 0x20030000 0x20030000 0x00f50 0x00f50 R E 0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x10

     Section to Segment mapping:
      Segment Sections...
       00     .stack
       01     .start .text .rodata
       02

(In this case, the .stack section is included in the final binary in a segment
of size `0x900`, to account for the segment alignment constraints. The `.stack`
symbol is placed at the intended offset though. The `.stack` should not at all
end in the final binary, and never at a `PhysAddr` / load address within the RAM
section; however this is an orthogonal issue.)
@lschuermann lschuermann requested a review from jrvanwhy June 29, 2023 12:33
lschuermann pushed a commit to lschuermann/libtock-rs that referenced this pull request Jun 29, 2023
Previously, `.stack` and `.bss` sections were simply instructed to be
placed into RAM. Without an explicitly specified load address or load
address region, this means that their ELF-segment's `PhysAddr` will be
set to its `VirtAddr` (and hence point into RAM):

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x20030000 0x20030000 0x00000 0x00060 R   0x1000
      LOAD           0x001060 0x20030060 0x20030060 0x00d64 0x00d64 R E 0x1000
      LOAD           0x001dc4 0x20030dc4 0x20030dc4 0x001ec 0x001ec R   0x1000
      LOAD           0x002000 0x10004000 0x10004000 0x00000 0x00100 RW  0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

     Section to Segment mapping:
      Segment Sections...
       00     .tbf_header
       01     .start .text
       02     .rodata
       03     .stack
       04

When, for any reason, these segments contain any actual
data (`FileSiz` != 0), for instance under certain alignment
constraints[1], this will cause a ROM-loader such as elf2tab to place
them at their specified `PhysAddr` offset. As a result, if
`ORIGIN(FLASH)` precedes `ORIGIN(RAM)`, the binary is blown up due to
padding. If `ORIGIN(RAM)` precedes `ORIGIN(FLASH)`, the `rt_header`
internal offsets are incorrect because the actual FLASH load address
is now offset by `ORIGIN(FLASH) - ORIGIN(RAM)`. With this change, no
`LOAD`able segment will have a `PhysAddr` outside of the `FLASH`
memory region. This does not have any effect on zero-sized
sections (they will not increase flash usage):

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x20030000 0x20030000 0x00fb0 0x00fb0 RWE 0x1000
      LOAD           0x000800 0x10004800 0x20030fb0 0x00000 0x00100 RW  0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x10

     Section to Segment mapping:
      Segment Sections...
       00     .tbf_header .start .text .rodata
       01     .stack
       02

The reason why this worked in the past was that `elf2tab` skipped
segments with a `FileSiz` == 0, and did so even before calculating
paddings. We should probably not rely on such an implementation detail
of our loader, which can further break if the linker were to insert
any padding into a segment.

[1]: See tock#477 for an example.
Copy link
Copy Markdown
Collaborator

@jrvanwhy jrvanwhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, padding to a multiple of 4kB is super wasteful. I worked super hard to make libtock-rs fit into a few hundred bytes...

I guess I'm glad we can use the GNU linker instead.

@jrvanwhy
Copy link
Copy Markdown
Collaborator

bors r+

IDK if Bors still works atm, we'll see.

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Jul 10, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors Bot merged commit d064279 into tock:master Jul 10, 2023
bors Bot added a commit that referenced this pull request Jul 10, 2023
478: runtime/libtock_layout.ld: use FLASH-segment LMAs for all sections r=jrvanwhy a=lschuermann

This change ensures that no ELF-segment will have a `PhysAddr` which points outside of the `FLASH` memory region. This is important for ROM-loaders such as elf2tab to reliably produce correct binaries.

### Motivation

Previously, `.stack` and `.bss` sections were simply instructed to be placed into RAM. Without an explicitly specified load address or load address region, this means that their ELF-segment's `PhysAddr` will be set to its `VirtAddr` (and hence point into RAM):

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x20030000 0x20030000 0x00000 0x00060 R   0x1000
      LOAD           0x001060 0x20030060 0x20030060 0x00d64 0x00d64 R E 0x1000
      LOAD           0x001dc4 0x20030dc4 0x20030dc4 0x001ec 0x001ec R   0x1000
      LOAD           0x002000 0x10004000 0x10004000 0x00000 0x00100 RW  0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0

     Section to Segment mapping:
      Segment Sections...
       00     .tbf_header
       01     .start .text
       02     .rodata
       03     .stack
       04

When, for any reason, these segments contain any actual data (`FileSiz` != 0), for instance under certain alignment
constraints[1], this will cause a ROM-loader such as elf2tab to place them at their specified `PhysAddr` offset. As a result, if `ORIGIN(FLASH)` precedes `ORIGIN(RAM)`, the binary is blown up due to padding. If `ORIGIN(RAM)` precedes `ORIGIN(FLASH)`, the `rt_header` internal offsets are incorrect because the actual FLASH load address is now offset by `ORIGIN(FLASH) - ORIGIN(RAM)`. With this change, no `LOAD`able segment will have a `PhysAddr` outside of the `FLASH` memory region. This does not have any effect on zero-sized sections (they will not increase flash usage):

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x20030000 0x20030000 0x00fb0 0x00fb0 RWE 0x1000
      LOAD           0x000800 0x10004800 0x20030fb0 0x00000 0x00100 RW  0x1000
      GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00000 RW  0x10

     Section to Segment mapping:
      Segment Sections...
       00     .tbf_header .start .text .rodata
       01     .stack
       02

The reason why this worked in the past was that `elf2tab` skipped segments with a `FileSiz` == 0, and did so even before calculating paddings. We should probably not rely on such an implementation detail of our loader, which can further break if the linker were to insert any padding into a segment.

[1]: See #477 for an example.

Co-authored-by: Leon Schuermann <leons@opentitan.org>
lschuermann pushed a commit to lschuermann/elf2tab that referenced this pull request Jul 13, 2023
This reverts commit d89f703.
Ultimately, the underlying issue motivating these changes seems to be
a linker bug, as documented in libtock-rs PR tock/libtock-rs#477 [1].
Indeed, we should follow the physical addresses as specified in the
ELF file to generate padding, meaning that the previous code was
correct.

However, there's two invariants which elf2tab should ensure still:

- the libtock-rs bug seems to indicate that the section placement in
  ELF files does not necessarily have to correlate to their physical
  (load) address. It appears to be valid for ELF files to list
  sections in an arbitrary order, and thus elf2tab should presumably
  sort sections by their physical address (intended load address),
  and then walk them and insert padding as necessary.

- the confusing behavior which lead to this incorrect bugfix was
  that elf2tab generated a very large (> 256MB) TBF. To ensure that
  this does not happen, we can introduce sanity checks.

  elf2tab acts as the Tock application loader, by placing apps into
  memory such that they can be executed in place. This means that
  the physical address of all LOADed segments must be within the
  board's FLASH address space. We can have elf2tab issue a warning
  if there is an excessive amount of space (>= 4kB) between the
  physical (load) end address of one segment, and the start address
  of the next. This prevents linker script bugs from creeping into
  elf2tab undetected, and cause it to blindly generate nonsensical
  binaries.

[1]: tock/libtock-rs#477
@bradjc
Copy link
Copy Markdown
Contributor

bradjc commented Jul 31, 2023

make raspberry_pi_pico EXAMPLE=console
LIBTOCK_PLATFORM=raspberry_pi_pico cargo run --example console  \
		--target=thumbv6m-none-eabi --release
   Compiling libtock v0.1.0 (/Users/bradjc/git/libtock-rs)
error: linking with `rust-lld` failed: exit status: 1
  |
  = note: "rust-lld" "-flavor" "gnu" "/var/folders/_4/n3tmwxks3318tsb36d913f600000gn/T/rustcCgQTDB/symbols.o" "/Users/bradjc/git/libtock-rs/target/thumbv6m-none-eabi/release/examples/console-b8d0ccdcc2019f4a.console.958763e2-cgu.0.rcgu.o" "--as-needed" "-L" "/Users/bradjc/git/libtock-rs/target/thumbv6m-none-eabi/release/deps" "-L" "/Users/bradjc/git/libtock-rs/target/release/deps" "-L" "/Users/bradjc/git/libtock-rs/target/thumbv6m-none-eabi/release/build/libtock_runtime-99ba9d61b296ff5e/out" "-L" "/Users/bradjc/.rustup/toolchains/nightly-2022-06-10-x86_64-apple-darwin/lib/rustlib/thumbv6m-none-eabi/lib" "--start-group" "--end-group" "-Bstatic" "/Users/bradjc/.rustup/toolchains/nightly-2022-06-10-x86_64-apple-darwin/lib/rustlib/thumbv6m-none-eabi/lib/libcompiler_builtins-be4ad9a9d51b230c.rlib" "-Bdynamic" "--eh-frame-hdr" "-znoexecstack" "-L" "/Users/bradjc/.rustup/toolchains/nightly-2022-06-10-x86_64-apple-darwin/lib/rustlib/thumbv6m-none-eabi/lib" "-o" "/Users/bradjc/git/libtock-rs/target/thumbv6m-none-eabi/release/examples/console-b8d0ccdcc2019f4a" "--gc-sections" "-O1" "-Tlayout.ld"
  = note: rust-lld: error:
          Start of RAM region must be well-aligned to a 4kB boundary for LLVM's lld to
          work. Refer to https://github.com/tock/libtock-rs/pull/477 for more
          information. Set LIBTOCKRS_OVERRIDE_RAM_ORIGIN_CHECK = 1 to override this check
          (e.g., when using a different linker).

          rust-lld: error:
          Start of RAM region must be well-aligned to a 4kB boundary for LLVM's lld to
          work. Refer to https://github.com/tock/libtock-rs/pull/477 for more
          information. Set LIBTOCKRS_OVERRIDE_RAM_ORIGIN_CHECK = 1 to override this check
          (e.g., when using a different linker).


error: could not compile `libtock` due to previous error
make: *** [raspberry_pi_pico] Error 101

What do I do?

@lschuermann
Copy link
Copy Markdown
Member Author

@bradjc Merge #476. That fixes this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants