Add the stack protector for LoongArch64#12677
Conversation
Added LoongArch64 stack cookie interrupt instance, calling `CpuBreakpoint` to stop the CPU. Signed-off-by: Chao Li <lichao@loongson.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn>
Added LoongArch64 support. Currently, LoongArch64 doesn't supports TRNG, it uses the `RDTIME` instruction and a xorshif64 alorithms to compose a PRNG(RngLib). It may supports the SE TRNG in the future, and will change to the TRNG when SE support becomes available. Signed-off-by: Chao Li <lichao@loongson.cn> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn>
LoongArch64 GCC or CLANG currently does not support the parameter `-mstack-protector-guard=global`, but if `-fstack-protector` is enabled, the guard is global. The `-mstack-protector-guard` may be get supportted in the next GCC release, possibly GCC17. Signed-off-by: Chao Li <lichao@loongson.cn> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Guillermo Antonio Palomino Sosa <guillermo.a.palomino.sosa@intel.com> Cc: Yuwei Chen <yuwei.chen@intel.com> Cc: Poncho Figueroa <poncho.figueroa.esqueda@intel.com> Cc: Mike Beaton <mjsbeaton@gmail.com>
379e785 to
7897a51
Compare
os-d
left a comment
There was a problem hiding this comment.
StackCheckLib is moving to a different model that will have no dependencies in this PR: #12182. The review has stalled and I have not kicked it because I have other higher priority PRs that I'm asking that set of reviewers to prioritize, but as soon as those are in (I'm hoping this week), I'll return to this.
Can this change wait for that PR? Or can you test on top of that PR (it also needs this BaseTools change: #12373). I don't have a way to test LoongArch64, so ideally we can refactor StackCheckLib and then have you come in after that, testing with the new design.
At the very least, can we remove RngLib as a dependency? That does not work with the new model of having no dependencies and dropping the LTO disabling.
Hmm... let's me clarify: do you mean that the StackCheckLib shouldn't depend anything? In other words, it should like be a |
Correct. But I just looked back at the code and I see you did not add RngLib for StackCheckLib itself, only the dynamic entry points, so that does meet the goal. The dynamic entry points can have dependencies, though again I’ll ask: is it safe to have RngLib as a dependency for every module? I.e. you have it on the DxeCore Entry Point lib, does RngLib have any library constructors (at least in planned instances, it can’t be guaranteed)? Does it call out to any other functionality? If either of those is true or could be true, it can’t be used in all entry points. Is it possible to reproduce the RngLib functionality in the shared assembly? Or is it too platform specific? It may also be that LoongArch64 doesn’t use dynamic stack cookies if it can’t generate random numbers easily. |
Yes, I simply called the RngLib function once at the EntryPoint, and let's me talk about that why me imported the As mentioned above, |
Sure and sharing the code is reasonable. My concern is only around dependencies and particular for the core entry points.
The concern is if RngLib has a constructor, it won't be called on DXE Core entry. If RngLib used a protocol, it would be broken on DXE Core entry. If RngLib has any assumptions it is only being used after core services are available, it would be broken. I took a look at BaseRngLib, which I assume is what you assume will be used? That looks okay to use for LoongArch64, there is a constructor, which won't get called until later, but it just returns EFI_SUCCESS in the LoongArch64 case. However, a platform can use any instance and from the core side, we can't control that. But I'm okay to chalk that up to platform misconfiguration. But this is a dangerous area, a lot of assumptions don't hold here, so I wanted to follow up on it. All that being said, I don't have any intersection with LoongArch64 platforms, so as long as you deem this appropriate and the library stays scoped to LoongArch64 (as it already is), I'm okay with this going in. I'm also fine for this to go in first before the CLANGPDB support/refactor PR goes in, but I will reach out for you to test LoongArch64 at that time. |
I seemingly see your point: you are concerned that if the constructor in |
Exactly. Or that another instance of RngLib is chosen that calls out to another library that expects a constructor to run or attempts to use a dynamic PCD or protocol, etc. So, again, at this stage BaseRngLib is safe to use for LoongArch64 and I will defer to you on whether the risk that a platform switches to using a different RngLib can just be chalked up to platform misconfiguration and if your future plans for RNG align with using RngLib. |
Description
Patch1: Added LoongArch64 stack cookie interrupt instance, calling
CpuBreakpointto stop the CPU.Patch2: Add LoongArch64 support for dynamic stack cookie entry point library
Patch3: Enable stack protector for LoongArch64
How This Was Tested
Build a module using the
-stack-protector-all, and try to tamper with the "guard value" after it has been loaded theguardbut before it is compared, it shouldbreakthe CPU and tell us the stack has been changed.Integration Instructions
N/A