[PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls

Fri Jan 20 02:13:37 UTC 2023

On Fri, Jan 20, 2023 at 2:43 AM Casey Schaufler <casey at schaufler-ca.com> wrote:
>
> On 1/19/2023 3:10 PM, KP Singh wrote:
> > LSM hooks are currently invoked from a linked list as indirect calls
> > which are invoked using retpolines as a mitigation for speculative
> > attacks (Branch History / Target injection)  and add extra overhead which
> > is especially bad in kernel hot paths:
> >
> > security_file_ioctl:
> >    0xffffffff814f0320 <+0>:   endbr64
> >    0xffffffff814f0324 <+4>:   push   %rbp
> >    0xffffffff814f0325 <+5>:   push   %r15
> >    0xffffffff814f0327 <+7>:   push   %r14
> >    0xffffffff814f0329 <+9>:   push   %rbx
> >    0xffffffff814f032a <+10>:  mov    %rdx,%rbx
> >    0xffffffff814f032d <+13>:  mov    %esi,%ebp
> >    0xffffffff814f032f <+15>:  mov    %rdi,%r14
> >    0xffffffff814f0332 <+18>:  mov    $0xffffffff834a7030,%r15
> >    0xffffffff814f0339 <+25>:  mov    (%r15),%r15
> >    0xffffffff814f033c <+28>:  test   %r15,%r15
> >    0xffffffff814f033f <+31>:  je     0xffffffff814f0358 <security_file_ioctl+56>
> >    0xffffffff814f0341 <+33>:  mov    0x18(%r15),%r11
> >    0xffffffff814f0345 <+37>:  mov    %r14,%rdi
> >    0xffffffff814f0348 <+40>:  mov    %ebp,%esi
> >    0xffffffff814f034a <+42>:  mov    %rbx,%rdx
> >
> >>>> 0xffffffff814f034d <+45>:  call   0xffffffff81f742e0 <__x86_indirect_thunk_array+352>
> >     Indirect calls that use retpolines leading to overhead, not just due
> >     to extra instruction but also branch misses.
> >
> >    0xffffffff814f0352 <+50>:  test   %eax,%eax
> >    0xffffffff814f0354 <+52>:  je     0xffffffff814f0339 <security_file_ioctl+25>
> >    0xffffffff814f0356 <+54>:  jmp    0xffffffff814f035a <security_file_ioctl+58>
> >    0xffffffff814f0358 <+56>:  xor    %eax,%eax
> >    0xffffffff814f035a <+58>:  pop    %rbx
> >    0xffffffff814f035b <+59>:  pop    %r14
> >    0xffffffff814f035d <+61>:  pop    %r15
> >    0xffffffff814f035f <+63>:  pop    %rbp
> >    0xffffffff814f0360 <+64>:  jmp    0xffffffff81f747c4 <__x86_return_thunk>
> >
> > The indirect calls are not really needed as one knows the addresses of
> > enabled LSM callbacks at boot time and only the order can possibly
> > change at boot time with the lsm= kernel command line parameter.
> >
> > An array of static calls is defined per LSM hook and the static calls
> > are updated at boot time once the order has been determined.
> >
> > A static key guards whether an LSM static call is enabled or not,
> > without this static key, for LSM hooks that return an int, the presence
> > of the hook that returns a default value can create side-effects which
> > has resulted in bugs [1].
> >
> > With the hook now exposed as a static call, one can see that the
> > retpolines are no longer there and the LSM callbacks are invoked
> > directly:
> >
> > security_file_ioctl:
> >    0xffffffff814f0dd0 <+0>:   endbr64
> >    0xffffffff814f0dd4 <+4>:   push   %rbp
> >    0xffffffff814f0dd5 <+5>:   push   %r14
> >    0xffffffff814f0dd7 <+7>:   push   %rbx
> >    0xffffffff814f0dd8 <+8>:   mov    %rdx,%rbx
> >    0xffffffff814f0ddb <+11>:  mov    %esi,%ebp
> >    0xffffffff814f0ddd <+13>:  mov    %rdi,%r14
> >
> >>>> 0xffffffff814f0de0 <+16>:  jmp    0xffffffff814f0df1 <security_file_ioctl+33>
> >     Static key enabled for selinux_file_ioctl
> >
> >>>> 0xffffffff814f0de2 <+18>:  jmp    0xffffffff814f0e08 <security_file_ioctl+56>
> >    Static key enabled for bpf_lsm_file_ioctl. This is something that is
> >    changed to default to false to avoid the existing side effect issues
> >    of BPF LSM [1]
> >
> >    0xffffffff814f0de4 <+20>:  xor    %eax,%eax
> >    0xffffffff814f0de6 <+22>:  xchg   %ax,%ax
> >    0xffffffff814f0de8 <+24>:  pop    %rbx
> >    0xffffffff814f0de9 <+25>:  pop    %r14
> >    0xffffffff814f0deb <+27>:  pop    %rbp
> >    0xffffffff814f0dec <+28>:  jmp    0xffffffff81f767c4 <__x86_return_thunk>
> >    0xffffffff814f0df1 <+33>:  endbr64
> >    0xffffffff814f0df5 <+37>:  mov    %r14,%rdi
> >    0xffffffff814f0df8 <+40>:  mov    %ebp,%esi
> >    0xffffffff814f0dfa <+42>:  mov    %rbx,%rdx
> >
> >>>>   0xffffffff814f0dfd <+45>:        call   0xffffffff814fe820 <selinux_file_ioctl>
> >    Direct call to SELinux.
> >
> >    0xffffffff814f0e02 <+50>:  test   %eax,%eax
> >    0xffffffff814f0e04 <+52>:  jne    0xffffffff814f0de8 <security_file_ioctl+24>
> >    0xffffffff814f0e06 <+54>:  jmp    0xffffffff814f0de2 <security_file_ioctl+18>
> >    0xffffffff814f0e08 <+56>:  endbr64
> >    0xffffffff814f0e0c <+60>:  mov    %r14,%rdi
> >    0xffffffff814f0e0f <+63>:  mov    %ebp,%esi
> >    0xffffffff814f0e11 <+65>:  mov    %rbx,%rdx
> >
> >>>>  0xffffffff814f0e14 <+68>: call   0xffffffff8123b7d0 <bpf_lsm_file_ioctl>
> >    Direct call to bpf_lsm_file_ioctl
> >
> >    0xffffffff814f0e19 <+73>:  test   %eax,%eax
> >    0xffffffff814f0e1b <+75>:  jne    0xffffffff814f0de8 <security_file_ioctl+24>
> >    0xffffffff814f0e1d <+77>:  jmp    0xffffffff814f0de4 <security_file_ioctl+20>
> >    0xffffffff814f0e1f <+79>:  endbr64
> >    0xffffffff814f0e23 <+83>:  mov    %r14,%rdi
> >    0xffffffff814f0e26 <+86>:  mov    %ebp,%esi
> >    0xffffffff814f0e28 <+88>:  mov    %rbx,%rdx
> >    0xffffffff814f0e2b <+91>:  pop    %rbx
> >    0xffffffff814f0e2c <+92>:  pop    %r14
> >    0xffffffff814f0e2e <+94>:  pop    %rbp
> >    0xffffffff814f0e2f <+95>:  ret
> >    0xffffffff814f0e30 <+96>:  int3
> >    0xffffffff814f0e31 <+97>:  int3
> >    0xffffffff814f0e32 <+98>:  int3
> >    0xffffffff814f0e33 <+99>:  int3
> >
> > There are some hooks that don't use the call_int_hook and
> > call_void_hook. These hooks are updated to use a new macro called
> > security_for_each_hook
>
> There has got to be a simpler way to do this. Putting all the code
> for an extraordinary hook into a macro scares the bedickens out of me.
> The call_{int,void}_hook macros work fine for the simple cases, but
> that's because they are the simple cases. What would the hooks that use
> your new macro look like if you coded them directly?

It cannot be coded directly, the number of possible LSMs is determined
at compile time using CONFIG_* macros, so directly coding the slots
would become even more cumbersome and error prone (e.g. missing out
stuff when a new hook is added etc) and updating the static calls at
boot time without a compile time enforced macro would make the logic
even more complex.

Also note, what I dumped here is assembly at runtime, this looks very
different at compile time: Here's the compile time assembly:

ecurity_file_ioctl:
   0xffffffff814f0e90 <+0>: endbr64
   0xffffffff814f0e94 <+4>: push   %rbp
   0xffffffff814f0e95 <+5>: push   %r14
   0xffffffff814f0e97 <+7>: push   %rbx
   0xffffffff814f0e98 <+8>: mov    %rdx,%rbx
   0xffffffff814f0e9b <+11>: mov    %esi,%ebp
   0xffffffff814f0e9d <+13>: mov    %rdi,%r14
   0xffffffff814f0ea0 <+16>: xchg   %ax,%ax
   0xffffffff814f0ea2 <+18>: xchg   %ax,%ax
   0xffffffff814f0ea4 <+20>: xor    %eax,%eax
   0xffffffff814f0ea6 <+22>: xchg   %ax,%ax
   0xffffffff814f0ea8 <+24>: pop    %rbx
   0xffffffff814f0ea9 <+25>: pop    %r14
   0xffffffff814f0eab <+27>: pop    %rbp
   0xffffffff814f0eac <+28>: jmp    0xffffffff81f767c4 <__x86_return_thunk>
   0xffffffff814f0eb1 <+33>: endbr64
   0xffffffff814f0eb5 <+37>: mov    %r14,%rdi
   0xffffffff814f0eb8 <+40>: mov    %ebp,%esi
   0xffffffff814f0eba <+42>: mov    %rbx,%rdx
   0xffffffff814f0ebd <+45>: call   0xffffffff81f784f0
<__SCT__lsm_static_call_file_ioctl_0>
   0xffffffff814f0ec2 <+50>: test   %eax,%eax
   0xffffffff814f0ec4 <+52>: jne    0xffffffff814f0ea8 <security_file_ioctl+24>
   0xffffffff814f0ec6 <+54>: jmp    0xffffffff814f0ea2 <security_file_ioctl+18>
   0xffffffff814f0ec8 <+56>: endbr64
   0xffffffff814f0ecc <+60>: mov    %r14,%rdi
   0xffffffff814f0ecf <+63>: mov    %ebp,%esi
   0xffffffff814f0ed1 <+65>: mov    %rbx,%rdx
   0xffffffff814f0ed4 <+68>: call   0xffffffff81f784f8
<__SCT__lsm_static_call_file_ioctl_1>
   0xffffffff814f0ed9 <+73>: test   %eax,%eax
   0xffffffff814f0edb <+75>: jne    0xffffffff814f0ea8 <security_file_ioctl+24>
   0xffffffff814f0edd <+77>: jmp    0xffffffff814f0ea4 <security_file_ioctl+20>
   0xffffffff814f0edf <+79>: endbr64
   0xffffffff814f0ee3 <+83>: mov    %r14,%rdi
   0xffffffff814f0ee6 <+86>: mov    %ebp,%esi
   0xffffffff814f0ee8 <+88>: mov    %rbx,%rdx
   0xffffffff814f0eeb <+91>: pop    %rbx
   0xffffffff814f0eec <+92>: pop    %r14
   0xffffffff814f0eee <+94>: pop    %rbp
   0xffffffff814f0eef <+95>: jmp    0xffffffff81f78500
<__SCT__lsm_static_call_file_ioctl_2>

>
> >  where the lsm_callback is directly invoked as an
> > indirect call. Currently, there are no performance sensitive hooks that
> > use the security_for_each_hook macro. However, if, some performance
> > sensitive hooks are discovered, these can be updated to use
> > static calls with loop unrolling as well using a custom macro.
>
> Or how about no macro at all? I would really like to see what the code
> would look like without this level of macro processing.
>

One can inline the static slot generation logic, but the code would
become quite complex and tricky to write without macros. Open for
suggestions.