Fixed
Status Update
No update yet.
Comments
pe...@arm.com
Thanks for reporting the potential problem. My instinct is that this is best discussed in public forums (LLVM issues, Discourse, binutils) rather than embargoed within the LLVM security group. My reasoning:
* This will need some discussion with binutils to change the default linker script, or at least make some awareness that there is a tradeoff of between security and relaxation so people can choose whether to merge read-only and read-write .sdata.
* IIUC the data that is going into .sdata is essentially constant pool entries, which is unlikely to be security sensitive data. There is obviously a risk that an attacker could modify these constants to subvert the behaviour of a program, although if they have already have arbitrary write access to read-write data it may not give the attacker a significant bump in what damage they can do.
There is an LLVM Security Group meeting tomorrow (16th January) hopefully we can discuss with enough people there to make a recommendation on whether this needs to be embargoed or whether it can be discussed in public.
* This will need some discussion with binutils to change the default linker script, or at least make some awareness that there is a tradeoff of between security and relaxation so people can choose whether to merge read-only and read-write .sdata.
* IIUC the data that is going into .sdata is essentially constant pool entries, which is unlikely to be security sensitive data. There is obviously a risk that an attacker could modify these constants to subvert the behaviour of a program, although if they have already have arbitrary write access to read-write data it may not give the attacker a significant bump in what damage they can do.
There is an LLVM Security Group meeting tomorrow (16th January) hopefully we can discuss with enough people there to make a recommendation on whether this needs to be embargoed or whether it can be discussed in public.
st...@gerhold.net
I think the question of whether the default linker script in binutils should be changed is somewhat separate to my report.
My point here is basically that if I have some program and intentionally link it with a custom linker script that keeps constant pool entries (i.e. .srodata) mapped read-only at runtime then this program will currently be more "secure" when built with GCC than when built with LLVM. In that case the default linker script in binutils is not involved.
I am not sure either if this slight difference in security is significant enough to keep this issue private. Let me know if I should post the description on GitHub instead!
My point here is basically that if I have some program and intentionally link it with a custom linker script that keeps constant pool entries (i.e. .srodata) mapped read-only at runtime then this program will currently be more "secure" when built with GCC than when built with LLVM. In that case the default linker script in binutils is not involved.
I am not sure either if this slight difference in security is significant enough to keep this issue private. Let me know if I should post the description on GitHub instead!
pe...@arm.com
Issue was discussed at the LLVM security group on 16th January. Consensus of the people on the call was that this is best discussed in public forums such as github or discourse rather than being embargoed. We agree that LLVM should use the read-only section type. We'll leave the issue non-public for another week to see if there are any other members that would like to keep this non-public.
st...@gerhold.net
Since 5 weeks have passed without any objection, can we make this issue public now? I will post the same issue again on GitHub but was waiting for you to remove the Restrict-View-SecurityTeam first.
ma...@google.com
[Empty comment from Monorail migration]
ma...@google.com
View restriction removed; sorry for the delay.
kr...@arm.com
stephan@gerhold.net: Did you raise a public github issue for this?
If so, could you share a pointer to the raised issue here?
We can then close this ticket/issue.
If so, could you share a pointer to the raised issue here?
We can then close this ticket/issue.
st...@gerhold.net
The issue was indeed fixed by https://github.com/llvm/llvm-project/pull/82214 . For the example in my original report (reference [2]), the constants are now placed in .srodata as expected. I discovered the open pull request when I was about to open the GitHub issue, so I never actually submitted it.
Thanks for the reminder, feel free to mark the ticket as fixed!
Thanks for the reminder, feel free to mark the ticket as fixed!
kr...@arm.com
Thank you for the quick reply!
Description
### Issue ###
The implementation of this concept is not entirely consistent in GCC and LLVM. GCC places small data in .sdata, .sbss and .srodata, while LLVM currently only produces .sdata and .sbss (but not .srodata). One of the effects of this is that LLVM places allocated constants in the writeable .sdata section instead of a read-only section (e.g. .rodata or .srodata). Consider e.g.:
uint64_t multiply_const_pool(uint64_t in) {
return in * 1832593672667189573ULL;
}
On GCC 12.3.0 the constant number is placed in the read-only .srodata.cst8 section:
.text
multiply_const_pool:
lui a5,%hi(.LC0)
ld a5,%lo(.LC0)(a5)
mul a0,a0,a5
ret
.section .srodata.cst8,"aM",@progbits,8 <-------
.align 3
.LC0:
.dword 1832593672667189573
On at least LLVM/clang 14+ the constant number is placed in the writeable .sdata section (older versions may also be affected but do not allocate a constant for this example):
.text
multiply_const_pool:
lui a1, %hi(.LCPI0_0)
ld a1, %lo(.LCPI0_0)(a1)
mul a0, a0, a1
ret
.section .sdata,"aw",@progbits <-------
.p2align 3, 0x0
.LCPI0_0:
.quad 1832593672667189573
The "small data sections" are enabled by default in GCC and LLVM but can be disabled using -msmall-data-limit=0. In that case LLVM does place the constant in a read-only section:
.section .rodata.cst8,"aM",@progbits,8 <-------
.p2align 3, 0x0
.LCPI0_0:
.quad 1832593672667189573
This can be reproduced using Compiler Explorer with different compiler versions [2]. (Note that GCC >= 13 and clang < 14 do not allocate a constant for this example.)
### Security Implications ###
The security implications of this are a bit difficult to judge. Linking both programs with the default linker script in GNU ld will actually produce similar results for both GCC and LLVM because .srodata and .sdata are just merged together in practice (i.e. .srodata will still end up being writeable in the linked binary):
/* We want the small data sections together, so single-instruction offsets
can access them all, and initialized data all before uninitialized, so
we can shorten the on-disk segment size. */
.sdata :
{
__SDATA_BEGIN__ = .;
*(.srodata.cst16) *(.srodata.cst8) *(.srodata.cst4) *(.srodata.cst2) *(.srodata .srodata.*)
*(.sdata .sdata.* .gnu.linkonce.s.*)
}
This does not have to be the case though. For example, in the Linux kernel .srodata is placed next to .rodata and linker relaxation is effectively only used for .sdata [3]. This will often be beneficial for security hardening in order to have .srodata mapped as read-only at runtime (although I have not checked if this is actually the case for Linux at the moment). In this situation, the same code compiled with LLVM currently has reduced security properties compared to GCC, because the LLVM output provides no way to differentiate between the constants and actual writeable data.
Having the constants writeable at runtime is obviously not a security issue by itself, but could be potentially abused to cause further damage when combined with another security vulnerability.
### Fix ###
Ideally LLVM would place these constants in .srodata like GCC. This would make it possible to use a custom linker script to favor security hardening (by having constants read-only at runtime) at the price of slightly less optimization potential. In some cases performance might even improve if this allows accesses to more important mutable data to be simplified, or because rodata does not need to be copied from ROM.
A quick fix to reduce the effect would be to change the default -msmall-data-limit to 0. This may have slight implications for performance or code size but avoids the issue as shown above. It seems like this was even discussed before [4] (independent of the security implications described in this report) but no agreement was found so far.
### Discussion ###
Rust is also affected by this [5] in a particularly strange way: The relocations necessary for linker relaxation to work are not generated by default (must be enabled with "-C target-feature=+relax" which is only available on nightly). Nevertheless, the small data sections are still generated and can only be disabled with a low-level option in very recent nightly versions [6] ("-Z llvm_module_flag=SmallDataLimit:u32:0:error"). In other words, there does not seem to be a way to workaround this problem with a stable Rust version.
It is difficult to fully understand the output of compilers nowadays, so I expect that most people will be simply unaware of this problem. In many cases they might even prefer the security hardening over the limited performance benefit of linker relaxation for the constants. I actually just noticed this by coincidence because I expected the writeable .data segment of a minimal Rust application from me to be empty (I intentionally avoided global mutable state when writing it).
### References ###
[1]:
[2]:
[3]:
[4]:
[5]:
[6]: