Hi,
This patch adjusts capability relocations against two classes of symbols: - Symbols which point into sections which may be accessed via the PCC. - Symbols without size information.
For the latter, we emit a warning and adjust the capability relocation and fragment such that the resulting capability has bounds permitting access to the entire section. This matches the behaviour of LLD.
For the former, we adjust them as described in the following explanation.
The Morello ABI accesses global data using ADR and ADRP, and has no special indirection to jump to other functions. Given this, the PCC must maintain its bounds and base so that during execution loading global data and jumping to other functions can be done without worrying about the current PCC permissions and bounds.
To implement this, all capabilities that could be loaded into the PCC (via BLR or similar) must have a bounds and base according to the PCC. This must span all global data and text sections (i.e. .got, .text, .got.plt and the like). There is already code finding the range that the PCC should span, this patch records the information in a variable that we can query later.
There are two places where we create a relocation requesting a capability to be initialised at runtime. When handling relocations which request a capability from the GOT, and when handling a CAPINIT relocation. This patch adjusts both.
We can't tell from inspection which symbols would be loaded into the PCC, but we know that those symbols must point into a section which is executable. Hence we do this operation for all symbols which point into an executable section.
Most RELATIVE relocations don't use the addend. Rather the VA and size we want are put in the relative fragment and the addend is zero. This is because the *base* of the capability usually matches the VA we want that capability initialised to. In these possibly-code symbols we want the base of the capability bounds to be the base of the PCC, and the VA to be something very different. Hence we make use of the addend in the RELA relocations to encode this offset.
Note on implementation:
c64_fixup_frag takes the base and size of a capability we want to request from the runtime and checks that these are exactly representable in a capability. This patch changes many of the capabilities we request from the runtime to have the same bounds (those of the PCC). We leave the check to look at the bounds requested by the symbol rather than to check the PCC bounds multiple times. That means that if a symbol that points into an executable section has incorrect bounds then this will trigger a linker error even though it will cause no security problem when this executes. This is a trade-off between getting extra checks that the compiler is handling object bounds sizes and erroring on non-problematic code.
We have a compatibility hack that if a symbol is defined in the linker script to be directly after a given section but is *named* something like __.*_start or __start_.* then we treat it as if it is defined at the very start of the next section. The new behaviour introduced in this patch needs to take account of the above compatibility hack.
Hence we define a helper function to perform the relevent adjustment. It would be nice to also use the helper function so that it can be used in elfNN_c64_resize_sections, but that function will already need updating to account for the fact that zero-sized symbols found in the input will span entire sections. Hence we leave this function for then, since the manner in which we would use this helper function may change.
This patch also updates the entire testsuite according to these changes. In some places the original test no longer checks what it wanted, since the base of all symbols pointing into executable sections are now the same. There we add extra symbols and things to check so we ensure that this behaviour of PCC bounds is seen and that the original behaviour is still seen on non-executable sections.
This commit also includes a few tidy-ups:
We adjust the base and limit that are checked in c64_fixup_frag. Originally this would calculate the base as value + addend. As discussed above the way we treat capabilities in Morello is such that the value determines the base and the addend determines the initial value pointing from that base. Hence the check that these capabilities had correct bounds was not correct.
We add an extra assertion in final_link_relocate for robustness purposes. There is an existing bug in the assembler where GOT relocations against local symbols can be turned into relocations against the relevant section symbol plus an addend. This is problematic for multiple reasons, one being that the linker implementation does not have any way to associate different GOT entries with the same symbol but multiple offsets. In fact the linker ignores any offset. Here we simply add an assertion that this never happens. It turns a silent pre-existing error into a noisy one.
Regression tested on aarch64-none-elf, OK for users/ARM/morello-binutils-gdb-master?
Thanks, Alex
2022-01-19 Alex Coplan alex.coplan@arm.com Matthew Malcolmson matthew.malcolmson@arm.com
bfd/ChangeLog:
* elfnn-aarch64.c (pcc_low): New. (pcc_high): New. (elfNN_c64_resize_sections): Update new global variables pcc_{low,high} instead of local variables to track PCC span. (enum c64_section_perm_type): New. (c64_symbol_section_adjustment): New. (c64_fixup_frag): Rework to calculate size appropriately for symbols that need adjustment. (c64_symbol_adjust): New. Use it ... (elfNN_aarch64_final_link_relocate): ... here.
ld/ChangeLog:
* testsuite/ld-aarch64/aarch64-elf.exp: Add new tests. * testsuite/ld-aarch64/emit-relocs-morello-6.d: New test. * testsuite/ld-aarch64/emit-relocs-morello-6.s: Assembly. * testsuite/ld-aarch64/emit-relocs-morello-6b.d: New test. * testsuite/ld-aarch64/emit-relocs-morello-7.d: New test. * testsuite/ld-aarch64/emit-relocs-morello-7.ld: Linker script thereof. * testsuite/ld-aarch64/emit-relocs-morello-7.s: Assembly. * testsuite/ld-aarch64/morello-capinit.d: New test. * testsuite/ld-aarch64/morello-capinit.ld: Linker script. * testsuite/ld-aarch64/morello-capinit.s: Assembly. * testsuite/ld-aarch64/morello-sizeless-global-syms.d: New test. * testsuite/ld-aarch64/morello-sizeless-global-syms.s: Assembly. * testsuite/ld-aarch64/morello-sizeless-got-syms.d: New test. * testsuite/ld-aarch64/morello-sizeless-got-syms.s: Assembly. * testsuite/ld-aarch64/morello-sizeless-local-syms.d: New test. * testsuite/ld-aarch64/morello-sizeless-local-syms.s: Assembly.
On 21 Jan 2022, at 14:10, Alex Coplan via Gnu-morello gnu-morello@op-lists.linaro.org wrote:
Hi,
This patch adjusts capability relocations against two classes of symbols:
- Symbols which point into sections which may be accessed via the PCC.
- Symbols without size information.
For the latter, we emit a warning and adjust the capability relocation and fragment such that the resulting capability has bounds permitting access to the entire section. This matches the behaviour of LLD.
For the former, we adjust them as described in the following explanation.
The Morello ABI accesses global data using ADR and ADRP, and has no special indirection to jump to other functions. Given this, the PCC must maintain its bounds and base so that during execution loading global data and jumping to other functions can be done without worrying about the current PCC permissions and bounds.
To implement this, all capabilities that could be loaded into the PCC (via BLR or similar) must have a bounds and base according to the PCC. This must span all global data and text sections (i.e. .got, .text, .got.plt and the like). There is already code finding the range that the PCC should span, this patch records the information in a variable that we can query later.
There are two places where we create a relocation requesting a capability to be initialised at runtime. When handling relocations which request a capability from the GOT, and when handling a CAPINIT relocation. This patch adjusts both.
We can't tell from inspection which symbols would be loaded into the PCC, but we know that those symbols must point into a section which is executable. Hence we do this operation for all symbols which point into an executable section.
Most RELATIVE relocations don't use the addend. Rather the VA and size we want are put in the relative fragment and the addend is zero. This is because the *base* of the capability usually matches the VA we want that capability initialised to. In these possibly-code symbols we want the base of the capability bounds to be the base of the PCC, and the VA to be something very different. Hence we make use of the addend in the RELA relocations to encode this offset.
Note on implementation:
c64_fixup_frag takes the base and size of a capability we want to request from the runtime and checks that these are exactly representable in a capability. This patch changes many of the capabilities we request from the runtime to have the same bounds (those of the PCC). We leave the check to look at the bounds requested by the symbol rather than to check the PCC bounds multiple times. That means that if a symbol that points into an executable section has incorrect bounds then this will trigger a linker error even though it will cause no security problem when this executes. This is a trade-off between getting extra checks that the compiler is handling object bounds sizes and erroring on non-problematic code.
We have a compatibility hack that if a symbol is defined in the linker script to be directly after a given section but is *named* something like __.*_start or __start_.* then we treat it as if it is defined at the very start of the next section. The new behaviour introduced in this patch needs to take account of the above compatibility hack.
Hence we define a helper function to perform the relevent adjustment. It would be nice to also use the helper function so that it can be used in elfNN_c64_resize_sections, but that function will already need updating to account for the fact that zero-sized symbols found in the input will span entire sections. Hence we leave this function for then, since the manner in which we would use this helper function may change.
This patch also updates the entire testsuite according to these changes. In some places the original test no longer checks what it wanted, since the base of all symbols pointing into executable sections are now the same. There we add extra symbols and things to check so we ensure that this behaviour of PCC bounds is seen and that the original behaviour is still seen on non-executable sections.
This commit also includes a few tidy-ups:
We adjust the base and limit that are checked in c64_fixup_frag. Originally this would calculate the base as value + addend. As discussed above the way we treat capabilities in Morello is such that the value determines the base and the addend determines the initial value pointing from that base. Hence the check that these capabilities had correct bounds was not correct.
We add an extra assertion in final_link_relocate for robustness purposes. There is an existing bug in the assembler where GOT relocations against local symbols can be turned into relocations against the relevant section symbol plus an addend. This is problematic for multiple reasons, one being that the linker implementation does not have any way to associate different GOT entries with the same symbol but multiple offsets. In fact the linker ignores any offset. Here we simply add an assertion that this never happens. It turns a silent pre-existing error into a noisy one.
Regression tested on aarch64-none-elf, OK for users/ARM/morello-binutils-gdb-master?
Thanks, Alex
Thanks for resending with the patch this time. This does indeed totally break spatial memory safety if a variable is defined that has 0 size, which can be done in many ways. Please fix this to actually do what LLD does, because this is completely different and unsafe.
Jess
2022-01-19 Alex Coplan alex.coplan@arm.com Matthew Malcolmson matthew.malcolmson@arm.com
bfd/ChangeLog:
- elfnn-aarch64.c (pcc_low): New.
(pcc_high): New. (elfNN_c64_resize_sections): Update new global variables pcc_{low,high} instead of local variables to track PCC span. (enum c64_section_perm_type): New. (c64_symbol_section_adjustment): New. (c64_fixup_frag): Rework to calculate size appropriately for symbols that need adjustment. (c64_symbol_adjust): New. Use it ... (elfNN_aarch64_final_link_relocate): ... here.
ld/ChangeLog:
- testsuite/ld-aarch64/aarch64-elf.exp: Add new tests.
- testsuite/ld-aarch64/emit-relocs-morello-6.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-6.s: Assembly.
- testsuite/ld-aarch64/emit-relocs-morello-6b.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.ld: Linker script thereof.
- testsuite/ld-aarch64/emit-relocs-morello-7.s: Assembly.
- testsuite/ld-aarch64/morello-capinit.d: New test.
- testsuite/ld-aarch64/morello-capinit.ld: Linker script.
- testsuite/ld-aarch64/morello-capinit.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-global-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-global-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-got-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-got-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-local-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-local-syms.s: Assembly.
<patch.txt>-- Gnu-morello mailing list -- gnu-morello@op-lists.linaro.org To unsubscribe send an email to gnu-morello-leave@op-lists.linaro.org
Hi Jess,
On 21/01/2022 15:57, jessica.clarke@cl.cam.ac.uk wrote:
On 21 Jan 2022, at 14:10, Alex Coplan via Gnu-morello gnu-morello@op-lists.linaro.org wrote:
Hi,
This patch adjusts capability relocations against two classes of symbols:
- Symbols which point into sections which may be accessed via the PCC.
- Symbols without size information.
For the latter, we emit a warning and adjust the capability relocation and fragment such that the resulting capability has bounds permitting access to the entire section. This matches the behaviour of LLD.
For the former, we adjust them as described in the following explanation.
The Morello ABI accesses global data using ADR and ADRP, and has no special indirection to jump to other functions. Given this, the PCC must maintain its bounds and base so that during execution loading global data and jumping to other functions can be done without worrying about the current PCC permissions and bounds.
To implement this, all capabilities that could be loaded into the PCC (via BLR or similar) must have a bounds and base according to the PCC. This must span all global data and text sections (i.e. .got, .text, .got.plt and the like). There is already code finding the range that the PCC should span, this patch records the information in a variable that we can query later.
There are two places where we create a relocation requesting a capability to be initialised at runtime. When handling relocations which request a capability from the GOT, and when handling a CAPINIT relocation. This patch adjusts both.
We can't tell from inspection which symbols would be loaded into the PCC, but we know that those symbols must point into a section which is executable. Hence we do this operation for all symbols which point into an executable section.
Most RELATIVE relocations don't use the addend. Rather the VA and size we want are put in the relative fragment and the addend is zero. This is because the *base* of the capability usually matches the VA we want that capability initialised to. In these possibly-code symbols we want the base of the capability bounds to be the base of the PCC, and the VA to be something very different. Hence we make use of the addend in the RELA relocations to encode this offset.
Note on implementation:
c64_fixup_frag takes the base and size of a capability we want to request from the runtime and checks that these are exactly representable in a capability. This patch changes many of the capabilities we request from the runtime to have the same bounds (those of the PCC). We leave the check to look at the bounds requested by the symbol rather than to check the PCC bounds multiple times. That means that if a symbol that points into an executable section has incorrect bounds then this will trigger a linker error even though it will cause no security problem when this executes. This is a trade-off between getting extra checks that the compiler is handling object bounds sizes and erroring on non-problematic code.
We have a compatibility hack that if a symbol is defined in the linker script to be directly after a given section but is *named* something like __.*_start or __start_.* then we treat it as if it is defined at the very start of the next section. The new behaviour introduced in this patch needs to take account of the above compatibility hack.
Hence we define a helper function to perform the relevent adjustment. It would be nice to also use the helper function so that it can be used in elfNN_c64_resize_sections, but that function will already need updating to account for the fact that zero-sized symbols found in the input will span entire sections. Hence we leave this function for then, since the manner in which we would use this helper function may change.
This patch also updates the entire testsuite according to these changes. In some places the original test no longer checks what it wanted, since the base of all symbols pointing into executable sections are now the same. There we add extra symbols and things to check so we ensure that this behaviour of PCC bounds is seen and that the original behaviour is still seen on non-executable sections.
This commit also includes a few tidy-ups:
We adjust the base and limit that are checked in c64_fixup_frag. Originally this would calculate the base as value + addend. As discussed above the way we treat capabilities in Morello is such that the value determines the base and the addend determines the initial value pointing from that base. Hence the check that these capabilities had correct bounds was not correct.
We add an extra assertion in final_link_relocate for robustness purposes. There is an existing bug in the assembler where GOT relocations against local symbols can be turned into relocations against the relevant section symbol plus an addend. This is problematic for multiple reasons, one being that the linker implementation does not have any way to associate different GOT entries with the same symbol but multiple offsets. In fact the linker ignores any offset. Here we simply add an assertion that this never happens. It turns a silent pre-existing error into a noisy one.
Regression tested on aarch64-none-elf, OK for users/ARM/morello-binutils-gdb-master?
Thanks, Alex
Thanks for resending with the patch this time.
Yeah, sorry about that. The mailing list was apparently configured with a 40 KB message size limit, so my initial message was withheld. I tried re-sending with the patch as a gzip-compressed message, but it appears the mailing list stripped the attachment when delivering the message (it doesn't appear in the archive either). My original message with the uncompressed attachment was then subsequently released, which is the one you replied to here.
This does indeed totally break spatial memory safety if a variable is defined that has 0 size, which can be done in many ways. Please fix this to actually do what LLD does, because this is completely different and unsafe.
I agree that this approach isn't great from a security perspective (hence making the linker warn whenever we do this in the patch). I've just checked what LLD does, and it looks like you're right in that we don't follow the LLD behaviour. Thanks for pointing that out. Taking the test file morello-sizeless-local-syms.s added with the patch, I'm seeing the following when linking with LLD:
$ ~/morello/llvm/morello-toolchain/bin/clang -target aarch64 -march=morello+c64 -mabi=purecap ld/testsuite/ld-aarch64/morello-sizeless-local-syms.s -nostdlib ld.lld: warning: could not determine size of cap reloc against local object baz
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x0)) referenced by object (in GOT) ptr3 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr3))
ld.lld: warning: could not determine size of cap reloc against local object bar
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x4)) referenced by object (in GOT) ptr2 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr2))
ld.lld: warning: could not determine size of cap reloc against local object foo
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x8)) referenced by object (in GOT) ptr1 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr1))
$ ~/morello/llvm/morello-toolchain/bin/llvm-readelf --cap-relocs lld.exe CHERI __cap_relocs [ 0x80020310 Base: 0x80030360 (ptr1+0) Length: 16 Perms: (RWDATA) 0x80020320 Base: 0x80030350 (ptr2+0) Length: 16 Perms: (RWDATA) 0x80020330 Base: 0x80030340 (ptr3+0) Length: 16 Perms: (RWDATA) 0x80030340 (ptr3) Base: 0x80030370 (baz+0) Length: 12 Perms: (RWDATA) 0x80030350 (ptr2) Base: 0x80030374 (bar+0) Length: 8 Perms: (RWDATA) 0x80030360 (ptr1) Base: 0x80030378 (foo+0) Length: 4 Perms: (RWDATA) ] CHERI __cap_relocs [ 0x80020310 Base: 0x80030360 (ptr1+0) Length: 16 Perms: (RWDATA) 0x80020320 Base: 0x80030350 (ptr2+0) Length: 16 Perms: (RWDATA) 0x80020330 Base: 0x80030340 (ptr3+0) Length: 16 Perms: (RWDATA) 0x80030340 (ptr3) Base: 0x80030370 (baz+0) Length: 12 Perms: (RWDATA) 0x80030350 (ptr2) Base: 0x80030374 (bar+0) Length: 8 Perms: (RWDATA) 0x80030360 (ptr1) Base: 0x80030378 (foo+0) Length: 4 Perms: (RWDATA) ] $ ~/morello/llvm/morello-toolchain/bin/llvm-objdump -D lld.exe -j .bss
lld.exe: file format elf64-littleaarch64
Disassembly of section .bss:
0000000080030370 <baz>: ...
0000000080030374 <bar>: ...
0000000080030378 <foo>: ...
looking at the "Length" field in the __cap_relocs dump, it looks like the relocations permit access up to the end of the section. Our patch instead permits access to the entire section (both forwards and backwards from the symbol). We didn't intend to deviate from the LLD behaviour here. For some reason I got the impression from an internal discussion that LLD adjusted the base/addend as we do here.
Quoting your reply on the other thread: (https://op-lists.linaro.org/archives/list/gnu-morello@op-lists.linaro.org/me...)
LLD will only change the size of zero-sized symbols if they have a name that looks like a section start symbol (see isSectionStartSymbol in lld/ELF/Arch/Cheri.h).
from the above example, it doesn't look like your description of the LLD behaviour matches what LLD is doing either. Does the LLD behaviour in the example look unexpected? What should we be looking to do in GNU LD?
To provide additional context: the motivating example we have for this patch is the __TMC_LIST__ symbol from libgcc's crtstuff.c (which is compiled to produce crtbegin.o and linked into every executable built by GCC): https://gcc.gnu.org/git/?p=gcc.git%3Ba=blob%3Bf=libgcc/crtstuff.c%3Bh=b98b86...
The __TMC_LIST__ symbol is an array of size zero, and I believe the libgcc code expects to be able to access beyond the symbol (but within the containing section). As it stands (i.e. without this patch), GNU LD fails to link binaries built by GCC due to the lack of size information on the __TMC_LIST__ symbol. We need to resolve this in some way. One option would be to drop the diagnostic and just give zero bounds on the symbol, reworking the libgcc code if necessary. We didn't go down this route initially as following (what we thought was) the LLD behaviour seemed to solve our immediate problem.
Thanks, Alex
Jess
2022-01-19 Alex Coplan alex.coplan@arm.com Matthew Malcolmson matthew.malcolmson@arm.com
bfd/ChangeLog:
- elfnn-aarch64.c (pcc_low): New.
(pcc_high): New. (elfNN_c64_resize_sections): Update new global variables pcc_{low,high} instead of local variables to track PCC span. (enum c64_section_perm_type): New. (c64_symbol_section_adjustment): New. (c64_fixup_frag): Rework to calculate size appropriately for symbols that need adjustment. (c64_symbol_adjust): New. Use it ... (elfNN_aarch64_final_link_relocate): ... here.
ld/ChangeLog:
- testsuite/ld-aarch64/aarch64-elf.exp: Add new tests.
- testsuite/ld-aarch64/emit-relocs-morello-6.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-6.s: Assembly.
- testsuite/ld-aarch64/emit-relocs-morello-6b.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.ld: Linker script thereof.
- testsuite/ld-aarch64/emit-relocs-morello-7.s: Assembly.
- testsuite/ld-aarch64/morello-capinit.d: New test.
- testsuite/ld-aarch64/morello-capinit.ld: Linker script.
- testsuite/ld-aarch64/morello-capinit.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-global-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-global-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-got-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-got-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-local-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-local-syms.s: Assembly.
<patch.txt>-- Gnu-morello mailing list -- gnu-morello@op-lists.linaro.org To unsubscribe send an email to gnu-morello-leave@op-lists.linaro.org
On 24 Jan 2022, at 17:12, Alex Coplan alex.coplan@arm.com wrote:
Hi Jess,
On 21/01/2022 15:57, jessica.clarke@cl.cam.ac.uk wrote:
On 21 Jan 2022, at 14:10, Alex Coplan via Gnu-morello gnu-morello@op-lists.linaro.org wrote:
Hi,
This patch adjusts capability relocations against two classes of symbols:
- Symbols which point into sections which may be accessed via the PCC.
- Symbols without size information.
For the latter, we emit a warning and adjust the capability relocation and fragment such that the resulting capability has bounds permitting access to the entire section. This matches the behaviour of LLD.
For the former, we adjust them as described in the following explanation.
The Morello ABI accesses global data using ADR and ADRP, and has no special indirection to jump to other functions. Given this, the PCC must maintain its bounds and base so that during execution loading global data and jumping to other functions can be done without worrying about the current PCC permissions and bounds.
To implement this, all capabilities that could be loaded into the PCC (via BLR or similar) must have a bounds and base according to the PCC. This must span all global data and text sections (i.e. .got, .text, .got.plt and the like). There is already code finding the range that the PCC should span, this patch records the information in a variable that we can query later.
There are two places where we create a relocation requesting a capability to be initialised at runtime. When handling relocations which request a capability from the GOT, and when handling a CAPINIT relocation. This patch adjusts both.
We can't tell from inspection which symbols would be loaded into the PCC, but we know that those symbols must point into a section which is executable. Hence we do this operation for all symbols which point into an executable section.
Most RELATIVE relocations don't use the addend. Rather the VA and size we want are put in the relative fragment and the addend is zero. This is because the *base* of the capability usually matches the VA we want that capability initialised to. In these possibly-code symbols we want the base of the capability bounds to be the base of the PCC, and the VA to be something very different. Hence we make use of the addend in the RELA relocations to encode this offset.
Note on implementation:
c64_fixup_frag takes the base and size of a capability we want to request from the runtime and checks that these are exactly representable in a capability. This patch changes many of the capabilities we request from the runtime to have the same bounds (those of the PCC). We leave the check to look at the bounds requested by the symbol rather than to check the PCC bounds multiple times. That means that if a symbol that points into an executable section has incorrect bounds then this will trigger a linker error even though it will cause no security problem when this executes. This is a trade-off between getting extra checks that the compiler is handling object bounds sizes and erroring on non-problematic code.
We have a compatibility hack that if a symbol is defined in the linker script to be directly after a given section but is *named* something like __.*_start or __start_.* then we treat it as if it is defined at the very start of the next section. The new behaviour introduced in this patch needs to take account of the above compatibility hack.
Hence we define a helper function to perform the relevent adjustment. It would be nice to also use the helper function so that it can be used in elfNN_c64_resize_sections, but that function will already need updating to account for the fact that zero-sized symbols found in the input will span entire sections. Hence we leave this function for then, since the manner in which we would use this helper function may change.
This patch also updates the entire testsuite according to these changes. In some places the original test no longer checks what it wanted, since the base of all symbols pointing into executable sections are now the same. There we add extra symbols and things to check so we ensure that this behaviour of PCC bounds is seen and that the original behaviour is still seen on non-executable sections.
This commit also includes a few tidy-ups:
We adjust the base and limit that are checked in c64_fixup_frag. Originally this would calculate the base as value + addend. As discussed above the way we treat capabilities in Morello is such that the value determines the base and the addend determines the initial value pointing from that base. Hence the check that these capabilities had correct bounds was not correct.
We add an extra assertion in final_link_relocate for robustness purposes. There is an existing bug in the assembler where GOT relocations against local symbols can be turned into relocations against the relevant section symbol plus an addend. This is problematic for multiple reasons, one being that the linker implementation does not have any way to associate different GOT entries with the same symbol but multiple offsets. In fact the linker ignores any offset. Here we simply add an assertion that this never happens. It turns a silent pre-existing error into a noisy one.
Regression tested on aarch64-none-elf, OK for users/ARM/morello-binutils-gdb-master?
Thanks, Alex
Thanks for resending with the patch this time.
Yeah, sorry about that. The mailing list was apparently configured with a 40 KB message size limit, so my initial message was withheld. I tried re-sending with the patch as a gzip-compressed message, but it appears the mailing list stripped the attachment when delivering the message (it doesn't appear in the archive either). My original message with the uncompressed attachment was then subsequently released, which is the one you replied to here.
This does indeed totally break spatial memory safety if a variable is defined that has 0 size, which can be done in many ways. Please fix this to actually do what LLD does, because this is completely different and unsafe.
I agree that this approach isn't great from a security perspective (hence making the linker warn whenever we do this in the patch). I've just checked what LLD does, and it looks like you're right in that we don't follow the LLD behaviour. Thanks for pointing that out. Taking the test file morello-sizeless-local-syms.s added with the patch, I'm seeing the following when linking with LLD:
$ ~/morello/llvm/morello-toolchain/bin/clang -target aarch64 -march=morello+c64 -mabi=purecap ld/testsuite/ld-aarch64/morello-sizeless-local-syms.s -nostdlib ld.lld: warning: could not determine size of cap reloc against local object baz
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x0)) referenced by object (in GOT) ptr3 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr3))
ld.lld: warning: could not determine size of cap reloc against local object bar
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x4)) referenced by object (in GOT) ptr2 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr2))
ld.lld: warning: could not determine size of cap reloc against local object foo
defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(.bss+0x8)) referenced by object (in GOT) ptr1 defined in local.c (/tmp/morello-sizeless-local-syms-37ddd7.o:(ptr1))
$ ~/morello/llvm/morello-toolchain/bin/llvm-readelf --cap-relocs lld.exe CHERI __cap_relocs [ 0x80020310 Base: 0x80030360 (ptr1+0) Length: 16 Perms: (RWDATA) 0x80020320 Base: 0x80030350 (ptr2+0) Length: 16 Perms: (RWDATA) 0x80020330 Base: 0x80030340 (ptr3+0) Length: 16 Perms: (RWDATA) 0x80030340 (ptr3) Base: 0x80030370 (baz+0) Length: 12 Perms: (RWDATA) 0x80030350 (ptr2) Base: 0x80030374 (bar+0) Length: 8 Perms: (RWDATA) 0x80030360 (ptr1) Base: 0x80030378 (foo+0) Length: 4 Perms: (RWDATA) ] CHERI __cap_relocs [ 0x80020310 Base: 0x80030360 (ptr1+0) Length: 16 Perms: (RWDATA) 0x80020320 Base: 0x80030350 (ptr2+0) Length: 16 Perms: (RWDATA) 0x80020330 Base: 0x80030340 (ptr3+0) Length: 16 Perms: (RWDATA) 0x80030340 (ptr3) Base: 0x80030370 (baz+0) Length: 12 Perms: (RWDATA) 0x80030350 (ptr2) Base: 0x80030374 (bar+0) Length: 8 Perms: (RWDATA) 0x80030360 (ptr1) Base: 0x80030378 (foo+0) Length: 4 Perms: (RWDATA) ] $ ~/morello/llvm/morello-toolchain/bin/llvm-objdump -D lld.exe -j .bss
lld.exe: file format elf64-littleaarch64
Disassembly of section .bss:
0000000080030370 <baz>: ...
0000000080030374 <bar>: ...
0000000080030378 <foo>: ...
looking at the "Length" field in the __cap_relocs dump, it looks like the relocations permit access up to the end of the section. Our patch instead permits access to the entire section (both forwards and backwards from the symbol). We didn't intend to deviate from the LLD behaviour here. For some reason I got the impression from an internal discussion that LLD adjusted the base/addend as we do here.
It shouldn’t do that, that’s clearly still broken. I’d be immediately suspicious of https://git.morello-project.org/morello/llvm-project/-/blob/morello/dev/lld/..., which I’ve never understood the point of and has always seemed dubious but I’ve not touched. Just like I said to Malcom earlier, I advise you to check what CHERI-RISC-V does, as that’s been carefully thought through by us. I would not trust Morello LLVM to match it in all cases, with the differences tending to be bugs.
Quoting your reply on the other thread: (https://op-lists.linaro.org/archives/list/gnu-morello@op-lists.linaro.org/me...)
LLD will only change the size of zero-sized symbols if they have a name that looks like a section start symbol (see isSectionStartSymbol in lld/ELF/Arch/Cheri.h).
from the above example, it doesn't look like your description of the LLD behaviour matches what LLD is doing either. Does the LLD behaviour in the example look unexpected? What should we be looking to do in GNU LD?
Ideally whatever CHERI-RISC-V does, which should also be what Morello LLVM does for Morello, but isn’t always. I would advise against copying Morello LLVM’s bugs, but there may be cases where that is the pragmatic thing to do; preferably such bugs would instead just be fixed in Morello LLVM rather than existing in multiple places that need fixing later.
To provide additional context: the motivating example we have for this patch is the __TMC_LIST__ symbol from libgcc's crtstuff.c (which is compiled to produce crtbegin.o and linked into every executable built by GCC): https://gcc.gnu.org/git/?p=gcc.git%3Ba=blob%3Bf=libgcc/crtstuff.c%3Bh=b98b86...
The __TMC_LIST__ symbol is an array of size zero, and I believe the libgcc code expects to be able to access beyond the symbol (but within the containing section). As it stands (i.e. without this patch), GNU LD fails to link binaries built by GCC due to the lack of size information on the __TMC_LIST__ symbol. We need to resolve this in some way. One option would be to drop the diagnostic and just give zero bounds on the symbol, reworking the libgcc code if necessary. We didn't go down this route initially as following (what we thought was) the LLD behaviour seemed to solve our immediate problem.
I mean, I would just not support GNU Transactional Memory, but otherwise I’d probably say it’s fair to add name.startswith(“__”) && name.endswith(“_LIST__”) as another case to the section start symbol heuristic. That or just use a normal linker set rather than a home-grown one like the legacy .ctors; i.e. use a valid C identifier for the section name and use the normal __start_/__stop_ symbols. There isn’t really much of a reason not to other than ABI compatibility, but this is a new ABI.
Jess
Thanks, Alex
Jess
2022-01-19 Alex Coplan alex.coplan@arm.com Matthew Malcolmson matthew.malcolmson@arm.com
bfd/ChangeLog:
- elfnn-aarch64.c (pcc_low): New.
(pcc_high): New. (elfNN_c64_resize_sections): Update new global variables pcc_{low,high} instead of local variables to track PCC span. (enum c64_section_perm_type): New. (c64_symbol_section_adjustment): New. (c64_fixup_frag): Rework to calculate size appropriately for symbols that need adjustment. (c64_symbol_adjust): New. Use it ... (elfNN_aarch64_final_link_relocate): ... here.
ld/ChangeLog:
- testsuite/ld-aarch64/aarch64-elf.exp: Add new tests.
- testsuite/ld-aarch64/emit-relocs-morello-6.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-6.s: Assembly.
- testsuite/ld-aarch64/emit-relocs-morello-6b.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.d: New test.
- testsuite/ld-aarch64/emit-relocs-morello-7.ld: Linker script thereof.
- testsuite/ld-aarch64/emit-relocs-morello-7.s: Assembly.
- testsuite/ld-aarch64/morello-capinit.d: New test.
- testsuite/ld-aarch64/morello-capinit.ld: Linker script.
- testsuite/ld-aarch64/morello-capinit.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-global-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-global-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-got-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-got-syms.s: Assembly.
- testsuite/ld-aarch64/morello-sizeless-local-syms.d: New test.
- testsuite/ld-aarch64/morello-sizeless-local-syms.s: Assembly.
<patch.txt>-- Gnu-morello mailing list -- gnu-morello@op-lists.linaro.org To unsubscribe send an email to gnu-morello-leave@op-lists.linaro.org
gnu-morello@op-lists.linaro.org
-
Alex Coplan -
jessica.clarke＠cl.cam.ac.uk