Messages in this thread | | | Date | Wed, 25 May 2022 13:02:28 -0700 | From | Kees Cook <> | Subject | Re: [RFC PATCH v2 20/21] x86: Add support for CONFIG_CFI_CLANG |
| |
On Mon, May 16, 2022 at 10:37:23PM +0200, Peter Zijlstra wrote: > On Mon, May 16, 2022 at 12:39:19PM -0700, Sami Tolvanen wrote: > > > > > With the current compiler patch, LLVM sets up function arguments after > > > > the CFI check. if it's a problem, we can look into changing that. > > > > > > Yes, please fix that. Again see that same patch for why this is a > > > problem. Objtool can trivially find retpoline calls, but finding this > > > kCFI gadget is going to be hard work. If you ensure they're > > > unconditionally stuck together, then the problem goes away find one, > > > finds the other. > > > > You can use .kcfi_traps to locate the check right now, but I agree, > > it's not quite ideal. > > Oohh, indeed. > [...]
Hi Peter,
Sami investigated moving the CFI check after arg setup, and found that to do it means making LLVM's CFI logic no longer both architecture and call-type agnostic. The CFI logic needs put at a lower level, requiring it be done in per-architecture code, and then additionally per-call-type. (And by per-call-type, AIUI, this means various types of indirect calls: standard, tail-call, etc.) Sami has it all working for x86, but I'm concerned about the risk/benefit in doing it this way. I only see down-sides:
- Linux cannot enable CFI for a new architecture without also implementing it within LLVM first.
- LLVM CFI code becomes more complex, making it harder to maintain/bug-fix/etc.
I actually see the first issue as the larger problem: I want to make it easy for the kernel to use CFI, rather than harder. :P The first user of CFI already cleared the way for other architectures by adjusting the bulk of core kernel code, etc, so adding another architecture should be as trivial as possible, and not require yet newer releases of LLVM, etc, etc.
So, since using .kcfi_traps already solves the issue for finding the checks, can we not require moving the CFI check? I think it would be a mistake to have to do that.
-Kees
-- Kees Cook
| |