Closed solardiz closed 1 year ago
I guess some of our Linux 5.6+ checks should now also apply to RHEL 8.something+, but we need to figure out exactly which and starting with what RHEL 8 release.
We have:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5,6,0)
#if !P_LKRG_KERNEL_HAS_VAR_LEN_JUMP_LABEL
typedef struct _p_text_poke_loc {
s32 rel_addr; /* addr := _stext + rel_addr */
s32 rel32;
u8 opcode;
const u8 text[POKE_MAX_OPCODE_SIZE];
} p_text_poke_loc;
#else
typedef struct _p_text_poke_loc {
/* addr := _stext + rel_addr */
s32 rel_addr;
s32 disp;
u8 len;
u8 opcode;
const u8 text[POKE_MAX_OPCODE_SIZE];
/* see text_poke_bp_batch() */
u8 old;
} p_text_poke_loc;
#endif
#else
typedef struct text_poke_loc p_text_poke_loc;
#endif
and we started introducing the above logic and more for 5.6+ in 0f7c6350a844c4a65a6860bff1172035e3cccae3 - this is now to be reviewed against RHEL8 backports.
According to https://access.redhat.com/articles/3078 RHEL 8.6 was released on 2022-05-10 and already included 4.18.0-372.9.1. Here we have the problem on 4.18.0-372.32.1.el8_6, so still within RHEL 8.6 (rather than in a revision that would look like it's getting closer to 8.7 - I'd expect the 372 to change for that). This also means we wouldn't be able to distinguish these kernels by our usual RHEL macro checks.
I assume Alma had updated to 8.6+ months ago as well. I didn't verify whether this is the case or not, but would be surprised if not. We should check the logs on our older Alma CI jobs.
We should check the logs on our older Alma CI jobs.
I just did - our recent successful build was on 4.18.0-372.26.1.el8_6. So something broke between that and 4.18.0-372.32.1.el8_6. I'm not sure there's anything for us to #if
for to detect that difference. We can probably download and review the SRPMs to find out.
So it's https://almalinux.pkgs.org/8/almalinux-baseos-x86_64/kernel-devel-4.18.0-372.26.1.el8_6.x86_64.rpm.html vs. https://almalinux.pkgs.org/8/almalinux-baseos-x86_64/kernel-devel-4.18.0-372.32.1.el8_6.x86_64.rpm.html
Somehow the .src.rpm is only downloadable at the URL given in 26, and gives a Not Found in 32 for now (not propagated yet?), but the kernel-devel binary package is downloadable from both - and that's where the headers we build against are.
$ diff -ur x26/usr/src/kernels/4.18.0-372.26.1.el8_6.x86_64/ x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/ | diffstat | fgrep -vw binary
diff: x26/usr/src/kernels/4.18.0-372.26.1.el8_6.x86_64/scripts/dtc/include-prefixes/nios2: No such file or directory
diff: x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/scripts/dtc/include-prefixes/nios2: No such file or directory
diff: x26/usr/src/kernels/4.18.0-372.26.1.el8_6.x86_64/scripts/dtc/include-prefixes/powerpc: No such file or directory
diff: x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/scripts/dtc/include-prefixes/powerpc: No such file or directory
x26/usr/src/kernels/4.18.0-372.26.1.el8_6.x86_64/arch/x86/include/asm/alternative-asm.h |only
x26/usr/src/kernels/4.18.0-372.26.1.el8_6.x86_64/arch/x86/include/asm/spec_ctrl.h |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/.config | 12
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/Makefile | 63
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/Makefile.rhelver | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/Module.symvers | 13
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/System.map |189973 +++++-----
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/Kconfig | 93
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/Makefile | 20
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/boot/compressed/Makefile | 1
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/entry/Makefile | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/entry/vdso/Makefile | 1
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/alternative.h | 126
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/cpufeatures.h | 18
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/disabled-features.h | 21
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/ftrace.h | 8
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/insn-eval.h | 4
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/insn.h | 26
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/irqflags.h | 4
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/kprobes.h | 14
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/linkage.h | 22
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/msr-index.h | 13
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/nospec-branch.h | 102
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/paravirt.h | 21
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/paravirt_types.h | 13
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/qspinlock_paravirt.h | 4
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/smap.h | 5
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/sync_core.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/text-patching.h | 125
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/arch/x86/kernel/Makefile | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/auto.conf | 12
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/auto.conf.cmd | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/cc/has/return |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/cpu/ibpb |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/cpu/ibrs |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/cpu/unret |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/kernel.release | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/rethunk.h |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/speculation |only
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/config/tristate.conf | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/generated/asm-offsets.h | 6
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/generated/autoconf.h | 10
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/generated/compile.h | 4
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/generated/uapi/linux/version.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/generated/utsrelease.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/cpu.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/cpufreq.h | 15
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/ftrace.h | 23
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/kvm_host.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/objtool.h | 11
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/common_hsi.h | 141
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/eth_common.h | 1
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/fcoe_common.h | 362
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/iscsi_common.h | 360
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/nvmetcp_common.h | 18
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_chain.h | 97
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_eth_if.h | 21
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_if.h | 277
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_iscsi_if.h | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_ll2_if.h | 42
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/qed_nvmetcp_if.h | 17
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/linux/qed/rdma_common.h | 1
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/include/net/dst_metadata.h | 14
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/lib/Kconfig | 2
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/lib/Kconfig.debug | 36
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/lib/Kconfig.kgdb | 8
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/scripts/Makefile.build | 4
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/scripts/mod/devicetable-offsets.s | 1228
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/security/Kconfig | 11
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/vmlinux.h | 112
x32/usr/src/kernels/4.18.0-372.32.1.el8_6.x86_64/vmlinux.id | 2
89 files changed, 96675 insertions(+), 96886 deletions(-)
As expected struct text_poke_loc
is gone from 4.18.0-372.32.1.el8_6.x86_64/arch/x86/include/asm/text-patching.h
. Curiously, the unexported interfaces like that are getting added to 4.18.0-372.32.1.el8_6.x86_64/vmlinux.h
, but I doubt we want to include that.
Removed:
-struct text_poke_loc {
- void *addr;
- int len;
- s32 rel32;
- u8 opcode;
- const u8 text[POKE_MAX_OPCODE_SIZE];
-};
Added (in vmlinux.h
):
+struct text_poke_loc {
+ s32 rel_addr;
+ s32 rel32;
+ u8 opcode;
+ const u8 text[5];
+};
A few #define
are added between these revisions as well, including of ASM_RET
, which we already check for. However, we set P_LKRG_KERNEL_HAS_VAR_LEN_JUMP_LABEL
when we see ASM_RET
and that'd actually disable (if we were on 5.6+) the new struct layout seen above (in 32's vmlinux.h
). So not the right thing to check for here? Need to find something else.
There's also:
#define INT3_INSN_SIZE 1
#define INT3_INSN_OPCODE 0xCC
+#define RET_INSN_SIZE 1
+#define RET_INSN_OPCODE 0xC3
+
#define CALL_INSN_SIZE 5
#define CALL_INSN_OPCODE 0xE8
#define JMP32_INSN_SIZE 5
#define JMP32_INSN_OPCODE 0xE9
#define JMP8_INSN_SIZE 2
#define JMP8_INSN_OPCODE 0xEB
+#define DISP32_SIZE 4
whereas we have:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0)
if ( (p_tmp->opcode == CALL_INSN_OPCODE
|| p_tmp->opcode == JMP32_INSN_OPCODE
|| p_tmp->opcode == INT3_INSN_OPCODE
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 10, 0)
|| p_tmp->opcode == RET_INSN_OPCODE
#endif
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 14, 0)
|| p_tmp->opcode == JMP8_INSN_OPCODE
#endif
Maybe we were already missing support for JMP8_INSN_OPCODE
, which RHEL8 had introduced earlier? Maybe we should enable this whole 5.6+ piece based on availability of those opcode macros - at least on RHEL?
JMP8_INSN_OPCODE
is also present in /lib/modules/4.18.0-348.23.1.el8_5.x86_64/build/arch/x86/include/asm/text-patching.h
on my Alma 8.5 test system, which has been running LKRG for some months now with no issues (but with little activity). So either we didn't actually need to support it or the issue somehow was not triggered for me.
The exact same problem has started occurring with Rocky Linux 8 as well, confirming this indeed comes from RHEL 8 upstream.
I think that might be the best way to go and if we detect JMP8_INSN_OPCODE
we can extrapolate that we need to define struct text_poke_loc
by ourselves. Although, we have 2 versions of that and I guess we need to distinguish if it is RHEL build, then use the newest one
if we detect
JMP8_INSN_OPCODE
we can extrapolate that we need to definestruct text_poke_loc
by ourselves.
@Adam-pi3 I think you misread what I wrote. LKRG built fine on Alma 8.5, which already has JMP8_INSN_OPCODE
, but didn't meet our checks for needing to define struct text_poke_loc
by ourselves. Which means that system did provide struct text_poke_loc
.
The reason I brought up JMP8_INSN_OPCODE
was different: it appeared on RHEL 8 earlier, but we did not check for it in code. So maybe we missed required support for this kind of jump labels... or maybe the kernel just defined that macro, but did not use such jump labels yet (which is indirectly confirmed by my test system never reporting a kernel integrity issue so far).
Then there's similar concern about RET_INSN_OPCODE
on newer RHEL 8 kernels (the same ones that this issue is about, but it's a separate potential issue).
In other words, getting things to build with the right struct is one issue. But also supporting all the required jump label types is another.
Is it safe to support a kind of jump labels that the kernel does not actually use? If so, we can replace those kernel version checks with opcode macro checks. Should we?
I've been playing with 3 kernel versions:
I found that only the newest one has declared DISP32_SIZE
and I prepared the patch for LKRG taking this into account and I was able to correctly compile and run it for all 3 kernels and I've verified JUMP_LABEL works as intended.
I spent too much time analyzing why my patch was not working at first implementation of my changes. It turns out that RHEL indeed hide the struct text_poke_loc
and updated it's definition, although they did NOT use the latest version of it. I assume that newer RHEL kernels will update the structure definition at some point (and backport other patches) and we would need to do a small adjustment to the proposed patch (but now it should be trivial).
In the end, my working patch looks as follow:
$ git diff
diff --git a/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.c b/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.c
index 84628e3..2e4c83f 100644
--- a/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.c
+++ b/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.c
@@ -72,14 +72,17 @@ notrace int p_arch_jump_label_transform_apply_entry(struct kretprobe_instance *p
for (p_jl_batch_nr = 0; p_cnt < p_nr; p_cnt++) {
p_tmp = (p_text_poke_loc *)&P_SYM(p_tp_vec)[p_jl_batch_nr*sizeof(p_text_poke_loc)];
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0) || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
if ( (p_tmp->opcode == CALL_INSN_OPCODE
|| p_tmp->opcode == JMP32_INSN_OPCODE
|| p_tmp->opcode == INT3_INSN_OPCODE
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 10, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 10, 0) || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
|| p_tmp->opcode == RET_INSN_OPCODE
#endif
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 14, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 14, 0) || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
|| p_tmp->opcode == JMP8_INSN_OPCODE
#endif
) &&
@@ -101,7 +104,8 @@ notrace int p_arch_jump_label_transform_apply_entry(struct kretprobe_instance *p
#endif
p_jl_batch_addr[p_jl_batch_nr++] =
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0) || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
(unsigned long)p_tmp->rel_addr +
(unsigned long)p_db.kernel_stext.p_addr;
#else
diff --git a/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.h b/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.h
index cfbf354..6551683 100644
--- a/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.h
+++ b/src/modules/database/JUMP_LABEL/p_arch_jump_label_transform_apply/p_arch_jump_label_transform_apply.h
@@ -38,6 +38,10 @@
#include <asm/linkage.h> /* for ASM_RET */
+#if defined(RHEL_RELEASE_CODE) && defined(DISP32_SIZE)
+ #define P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL 1
+#endif
+
/*
* This can be extended to other LTS or active branches if and when they
* receive the variable length JUMP_LABEL feature backport, although the
@@ -55,8 +59,10 @@
#include <asm/text-patching.h>
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(5,6,0)
- #if !P_LKRG_KERNEL_HAS_VAR_LEN_JUMP_LABEL
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(5,6,0) || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
+ #if !P_LKRG_KERNEL_HAS_VAR_LEN_JUMP_LABEL || \
+ defined(P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL)
typedef struct _p_text_poke_loc {
s32 rel_addr; /* addr := _stext + rel_addr */
s32 rel32;
Let me know what do you think, but if you agree to go with that path, I will prepare PR
Now I'm thinking if we should add extra check here:
+#if defined(RHEL_RELEASE_CODE) && defined(DISP32_SIZE)
+ #define P_LKRG_KERNEL_RHEL_VAR_LEN_JUMP_LABEL 1
+#endif
to also take into account RHEL version being 8.6+
@Adam-pi3 This looks reasonable to me, and I think it's not necessary to specifically check 8.6+, although you may. Please do prepare a PR and let's see what our CI tests say. Thank you!
This just started happening today, only noticed in forks of this repo so far:
https://github.com/solardiz/lkrg/actions/runs/3330536896/jobs/5509151190
My guess is it's some RHEL8 change that we now need to adapt to, but I didn't look into this for real yet.