Re: [PATCH v5 2/8] lkdtm/powerpc: Add test to hijack a patch mapping

On Wed, Aug 11, 2021 at 12:57:00PM -0500, Christopher M. Riedl wrote:

On Thu Aug 5, 2021 at 4:13 AM CDT, Christophe Leroy wrote:

quoted

Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :

quoted

When live patching with STRICT_KERNEL_RWX the CPU doing the patching
must temporarily remap the page(s) containing the patch site with +W
permissions. While this temporary mapping is in use, another CPU could
write to the same mapping and maliciously alter kernel text. Implement a
LKDTM test to attempt to exploit such an opening during code patching.
The test is implemented on powerpc and requires LKDTM built into the
kernel (building LKDTM as a module is insufficient).

The LKDTM "hijack" test works as follows:

   1. A CPU executes an infinite loop to patch an instruction. This is
      the "patching" CPU.
   2. Another CPU attempts to write to the address of the temporary
      mapping used by the "patching" CPU. This other CPU is the
      "hijacker" CPU. The hijack either fails with a fault/error or
      succeeds, in which case some kernel text is now overwritten.
[...]
+#if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
+	defined(CONFIG_PPC))

I think this test shouldn't be limited to CONFIG_PPC and shouldn't be
limited to CONFIG_STRICT_KERNEL_RWX. It should be there all the time.

Agreed: if the machinery exists to provide this defense on even one
arch/config/whatever combo, I'd like LKDTM to test for it. This lets use
compare defenses across different combinations more easily, and means
folks must answer questions like "why doesn't $combination provide
$defense?"

quoted

Also why limiting it to IS_BUILTIN(CONFIG_LKDTM) ?

The test needs read_cpu_patching_addr() which definitely cannot be
exposed outside of the kernel (ie. builtin).

FWIW, I'm okay with this. There isn't a solution that feels entirely
"right", so either a build-time requirement like this, or using an
exception for modules like this:

arch/x86/kernel/cpu/common.c:#if IS_MODULE(CONFIG_LKDTM)
arch/x86/kernel/cpu/common.c-EXPORT_SYMBOL_GPL(native_write_cr4);
arch/x86/kernel/cpu/common.c-#endif

I think neither is great. Another idea is maybe using a name-spaced
export, like:

EXPORT_SYMBOL_NS_GPL(native_write_cr4, LKDTM);

But that still means it gets exposed to malicious discovery, so probably
not.

I suspect the best is to just do the BUILTIN check, since building LKDTM
as a module on a _production_ kernel is rare if it exists at all. The
only downside is needing to completely reboot to perform updated tests,
but then, I frequently find myself breaking the kernel badly on bad
tests, so I have to reboot anyway. ;)

-Kees

-- 
Kees Cook