Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chibios: mark boot2 bootlader data readonly #21986

Merged
merged 1 commit into from
Sep 13, 2023
Merged

chibios: mark boot2 bootlader data readonly #21986

merged 1 commit into from
Sep 13, 2023

Conversation

t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Sep 12, 2023 •
edited
Loading

It does not need to be writable, also if the .boot2 section is writable it forces some other sections to be writable, too.

Fixes the following warning with GNU ld 2.41:

/usr/lib/gcc/arm-none-eabi/13.2.0/../../../../arm-none-eabi/bin/ld: warning: .build/1upkeyboards_sweet16v2_kb2040_default.elf has a LOAD segment with RWX permissions

ELF layout before:

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x001000 0x10000000 0x10000000 0x0a858 0x0a858 RWE 0x1000
  LOAD           0x00c000 0x20000000 0x1000a858 0x0081c 0x0081c R E 0x1000
  LOAD           0x000820 0x20000820 0x1000b074 0x00000 0x0189c RW  0x1000
  LOAD           0x000c00 0x20040c00 0x1000b074 0x00000 0x00120 RW  0x1000
  LOAD           0x0000bc 0x200020bc 0x200020bc 0x00000 0x3df44 RW  0x1000
  LOAD           0x000000 0x20040000 0x20040000 0x00000 0x00c00 RW  0x1000
  LOAD           0x000000 0x20041000 0x20041000 0x00000 0x00c00 RW  0x1000

 Section to Segment mapping:
  Segment Sections...
   00     .boot2 .vectors .text .rodata
   01     .data
   02     .bss
   03     .ram4
   04     .heap
   05     .mstack .pstack
   06     .c1_mstack .c1_pstack

ELF layout after:

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x001000 0x10000000 0x10000000 0x0a858 0x0a858 R E 0x1000
  LOAD           0x00c000 0x20000000 0x1000a858 0x0081c 0x0081c R E 0x1000
  LOAD           0x000820 0x20000820 0x1000b074 0x00000 0x0189c RW  0x1000
  LOAD           0x000c00 0x20040c00 0x1000b074 0x00000 0x00120 RW  0x1000
  LOAD           0x0000bc 0x200020bc 0x200020bc 0x00000 0x3df44 RW  0x1000
  LOAD           0x000000 0x20040000 0x20040000 0x00000 0x00c00 RW  0x1000
  LOAD           0x000000 0x20041000 0x20041000 0x00000 0x00c00 RW  0x1000

 Section to Segment mapping:
  Segment Sections...
   00     .boot2 .vectors .text .rodata
   01     .data
   02     .bss
   03     .ram4
   04     .heap
   05     .mstack .pstack
   06     .c1_mstack .c1_pstack
  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@t-8ch
chibios: mark boot2 bootlader data readonly
6ec13a7 
It does not need to be writable, also if the .boot2 section is writable
it forces some other sections to be writable, too.

Fixes the following warning with GNU ld 2.41:

/usr/lib/gcc/arm-none-eabi/13.2.0/../../../../arm-none-eabi/bin/ld: warning: .build/1upkeyboards_sweet16v2_kb2040_default.elf has a LOAD segment with RWX permissions

ELF layout before:

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x10000000 0x10000000 0x0a858 0x0a858 RWE 0x1000
      LOAD           0x00c000 0x20000000 0x1000a858 0x0081c 0x0081c R E 0x1000
      LOAD           0x000820 0x20000820 0x1000b074 0x00000 0x0189c RW  0x1000
      LOAD           0x000c00 0x20040c00 0x1000b074 0x00000 0x00120 RW  0x1000
      LOAD           0x0000bc 0x200020bc 0x200020bc 0x00000 0x3df44 RW  0x1000
      LOAD           0x000000 0x20040000 0x20040000 0x00000 0x00c00 RW  0x1000
      LOAD           0x000000 0x20041000 0x20041000 0x00000 0x00c00 RW  0x1000

     Section to Segment mapping:
      Segment Sections...
       00     .boot2 .vectors .text .rodata
       01     .data
       02     .bss
       03     .ram4
       04     .heap
       05     .mstack .pstack
       06     .c1_mstack .c1_pstack

ELF layout after:

    Program Headers:
      Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
      LOAD           0x001000 0x10000000 0x10000000 0x0a858 0x0a858 R E 0x1000
      LOAD           0x00c000 0x20000000 0x1000a858 0x0081c 0x0081c R E 0x1000
      LOAD           0x000820 0x20000820 0x1000b074 0x00000 0x0189c RW  0x1000
      LOAD           0x000c00 0x20040c00 0x1000b074 0x00000 0x00120 RW  0x1000
      LOAD           0x0000bc 0x200020bc 0x200020bc 0x00000 0x3df44 RW  0x1000
      LOAD           0x000000 0x20040000 0x20040000 0x00000 0x00c00 RW  0x1000
      LOAD           0x000000 0x20041000 0x20041000 0x00000 0x00c00 RW  0x1000

     Section to Segment mapping:
      Segment Sections...
       00     .boot2 .vectors .text .rodata
       01     .data
       02     .bss
       03     .ram4
       04     .heap
       05     .mstack .pstack
       06     .c1_mstack .c1_pstack
@github-actions github-actions bot added the core label Sep 12, 2023
@KarlK90
Copy link
Member

KarlK90 commented Sep 12, 2023 •
edited
Loading

@t-8ch thanks for the investigation! Unfortunately this only catches the warning for 1upkeyboards/sweet16v2/kb2040 and similiar cases which happen to nudge the linker to put the .bss region in a different section then .data which contains execute from ram code. Which is not only the BOOT2_ROM_RAM blob, but also everything mapped to the data.vtable and data.time_critical sections. When compiling the handwired/onekey/rp2040 example the fix falls apart again as the linker puts .bss and .data again into the same load section which is then RWE and produces rwx warning again:

❯ arm-none-eabi-readelf -lW  .build/handwired_onekey_rp2040_default.elf

Elf file type is EXEC (Executable file)
Entry point 0x100002c5
There are 7 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x001000 0x10000000 0x10000000 0x05850 0x05850 R E 0x1000
  LOAD           0x007000 0x20000000 0x10005850 0x00714 0x01d10 RWE 0x1000
  LOAD           0x000d10 0x20001d10 0x10005f64 0x00000 0x00004 RW  0x1000
  LOAD           0x000c00 0x20040c00 0x10005f64 0x00000 0x00120 RW  0x1000
  LOAD           0x000d14 0x20001d14 0x20001d14 0x00000 0x3e2ec RW  0x1000
  LOAD           0x000000 0x20040000 0x20040000 0x00000 0x00c00 RW  0x1000
  LOAD           0x000000 0x20041000 0x20041000 0x00000 0x00c00 RW  0x1000

 Section to Segment mapping:
  Segment Sections...
   00     .boot2 .vectors .text .rodata 
   01     .data .bss 
   02     .ram0 
   03     .ram4 
   04     .heap 
   05     .mstack .pstack 
   06     .c1_mstack .c1_pstack

I'm currently not sure how to fix this warning properly without resorting to workarounds as described here https://stackoverflow.com/questions/19259458/how-to-control-the-section-to-segment-mapping-in-an-elf-file.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 12, 2023

@KarlK90
I looked into that case, too. So far the only "fix" I could come up with was to just disable the warning.
Which is also what pico-sdk themselves are doing.

In any case the change in this PR should make the code more correct in any case.

@KarlK90
Copy link
Member

KarlK90 commented Sep 12, 2023

@KarlK90 I looked into that case, too. So far the only "fix" I could come up with was to just disable the warning. Which is also what pico-sdk themselves are doing.

Yes, we don't control the toolchains (yet?) so adding the suppressing flag isn't a viable option so far.

In any case the change in this PR should make the code more correct in any case.

Absolutely, no doubt about that :-).

@KarlK90 KarlK90 requested a review from a team September 12, 2023 22:04
@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 13, 2023

Yes, we don't control the toolchains (yet?) so adding the suppressing flag isn't a viable option so far.

I'm not sure:

qmk compile -e EXTRALDFLAGS=-Wl,--no-warn-rwx-segment works.
The final linking step is defined in builddefs/common_rules.mk.

@tzarc
Copy link
Member

tzarc commented Sep 13, 2023 •
edited
Loading

Not on CI:

./util/docker_cmd.sh qmk compile -kb 1upkeyboards/sweet16v2/kb2040 -km default -e EXTRALDFLAGS=-Wl,--no-warn-rwx-segment
Linking: .build/1upkeyboards_sweet16v2_kb2040_default.elf                                           [ERRORS]
 |
 | /usr/lib/gcc/arm-none-eabi/8.3.1/../../../arm-none-eabi/bin/ld: unrecognized option '--no-warn-rwx-segment'
 | /usr/lib/gcc/arm-none-eabi/8.3.1/../../../arm-none-eabi/bin/ld: use the --help option for usage information
 | collect2: error: ld returned 1 exit status
 |
gmake[1]: *** [builddefs/common_rules.mk:269: .build/1upkeyboards_sweet16v2_kb2040_default.elf] Error 1
gmake: *** [Makefile:392: 1upkeyboards/sweet16v2/kb2040:default] Error 1
Make finished with errors

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 13, 2023

Not on CI:

@tzarc This check and the corresponding flag are fairly new.

@tzarc
Copy link
Member

tzarc commented Sep 13, 2023

Oh I know, I'm just saying we can't just turn it on given that a lot of our target environments don't have knowledge of the flag.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 13, 2023

Something like cc-option from the Linux kernel could be used for autodetection.

@tzarc tzarc merged commit b9f2121 into qmk:develop Sep 13, 2023
4 checks passed
@t-8ch t-8ch deleted the chibios/boot2 branch September 13, 2023 09:19
@t-8ch t-8ch mentioned this pull request Sep 13, 2023
Merged
t-8ch added a commit to t-8ch/qmk_firmware that referenced this pull request Sep 20, 2023
@t-8ch
chibios: disable RWX segment warning on newer GNU lds
d64e81d 
The cc-option function is LGPL-2.1 from uClibc (originally check_gcc).
Adapted to handle -Wl linker arguments.

Also adapt the new function in existing logic where applicable.

See discussion in qmk#21986
Cc @tzarc @KarlK90
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Oct 25, 2023
@t-8ch @mechlovin
chibios: mark boot2 bootlader data readonly (qmk#21986)
53c1c65
christrotter pushed a commit to christrotter/qmk_firmware that referenced this pull request Nov 28, 2023
@t-8ch @christrotter
chibios: mark boot2 bootlader data readonly (qmk#21986)
7e74757
zgagnon pushed a commit to zgagnon/qmk_firmware_waterfowl that referenced this pull request Dec 15, 2023
@t-8ch @zgagnon
chibios: mark boot2 bootlader data readonly (qmk#21986)
fce9b34
future-figs pushed a commit to future-figs/qmk_firmware that referenced this pull request Dec 27, 2023
@t-8ch
chibios: mark boot2 bootlader data readonly (qmk#21986)
e957b75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KarlK90 KarlK90 KarlK90 approved these changes

@tzarc tzarc tzarc approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t-8ch @KarlK90 @tzarc