Some small fixes and possible valuable information #291

alexander-pick · 2023-10-12T08:06:30Z

Description

I was hunting down a global oobr triggered in kasan. Tuned out the modprobe has to be run from sudo -s instead just using sudo only to make it work. The only noticeable trace is the error dumping to dmesg if it fails. I assume this happens if kptr_restrict is set and the user who runs modprobe has no CP_SYSLOG permissions.

During the process I made a few changes which might be valuable:

reflected the above situation to README
added some lines to remove static keywords add-exports.sh, otherwise my 6.x kernel refused to compile (using clang)
added some checks to avoid hashes of zero sized sections so it's visible in debug log if this happens by setting the hash to 0xFFFFFFFF
fixed CONFIG_SECURITY_SELINUX vs. CONFIG_SECURITY_SELINUX_DEVELOP which would enable sel_write_enforce
fixes a typo in P_LKRG_EXPLOIT_DETECTION_SECURTIY_PTRACE_ACCESS_H
fixed some types for clarification
removed a sizeof as we can safely assume 8 for uint64_t and the next line shares this assumption by using & 7
added a case 0 to the hash algorithm to satisfy clang and avoid warnings
clarified a warning which I found to be misleading (I build in-tree, but as a module)

How Has This Been Tested?

Tested the build a lot of times on multiple systems with different configuration

* fixed typo in include guard * fixed some other oob read and added a warning to debug output * clarified types in signature code * clarified readme as "sudo modprobe ..." will interfer with kptr_restrict if user has no CP_SYSLOG

README

Adam-pi3 · 2023-10-13T23:21:19Z

scripts/add-exports.sh

+sed -i 's/static int change_page_attr_set_clr/int change_page_attr_set_clr/g'  arch/x86/mm/pat/set_memory.c
+sed -i 's/static struct dentry \*lookup_fast/struct dentry \*lookup_fast/g' fs/namei.c
+sed -i 's/static long do_seccomp/long do_seccomp/g' kernel/seccomp.c
+sed -i 's/static void __seccomp_filter_release(/void __seccomp_filter_release(/g' kernel/seccomp.c


It's already set at line 9 of this script.

My additions remove the static keywords, line 9 adds the export. If static remains the module will fail to compile properly:

ERROR: modpost: vmlinux: local symbol 'change_page_attr_set_clr' was exported ERROR: modpost: vmlinux: local symbol '__put_seccomp_filter' was exported ERROR: modpost: vmlinux: local symbol 'do_seccomp' was exported ERROR: modpost: vmlinux: local symbol 'lookup_fast' was exported

Essentially my changes stop this from happening.

Ah, thanks. Can you group the symbols together? e.g., when operating on __seccomp_filter_release we likely want to do all the changes at once and not in 2-3 different places. I assume, your changes can me merged into the original lines manipulating on that specific symbol. For other symbols just to work on them at the same place in file.

Adam-pi3 · 2023-10-13T23:21:43Z

scripts/add-exports.sh

@@ -8,3 +8,9 @@ echo "EXPORT_SYMBOL(lookup_fast);" >> fs/namei.c
 # keep symbol export inside the ifdef block defining the symbol
 sed -i '/static void __seccomp_filter_release.*/iEXPORT_SYMBOL(__put_seccomp_filter);\n' kernel/seccomp.c
 echo "EXPORT_SYMBOL(do_seccomp);" >> kernel/seccomp.c
+# fixes modpost errors
+sed -i 's/static int change_page_attr_set_clr/int change_page_attr_set_clr/g'  arch/x86/mm/pat/set_memory.c


We already export that symbol, why do you need it?

See above, it fixes modpost errors due to local syms.

Adam-pi3 · 2023-10-13T23:22:30Z

scripts/add-exports.sh

@@ -8,3 +8,9 @@ echo "EXPORT_SYMBOL(lookup_fast);" >> fs/namei.c
 # keep symbol export inside the ifdef block defining the symbol
 sed -i '/static void __seccomp_filter_release.*/iEXPORT_SYMBOL(__put_seccomp_filter);\n' kernel/seccomp.c
 echo "EXPORT_SYMBOL(do_seccomp);" >> kernel/seccomp.c
+# fixes modpost errors
+sed -i 's/static int change_page_attr_set_clr/int change_page_attr_set_clr/g'  arch/x86/mm/pat/set_memory.c
+sed -i 's/static struct dentry \*lookup_fast/struct dentry \*lookup_fast/g' fs/namei.c


It's also exported

See above, it removes the static keyword, not adds an export, it's just to solve modpost errors.

Adam-pi3 · 2023-10-13T23:22:37Z

scripts/add-exports.sh

+# fixes modpost errors
+sed -i 's/static int change_page_attr_set_clr/int change_page_attr_set_clr/g'  arch/x86/mm/pat/set_memory.c
+sed -i 's/static struct dentry \*lookup_fast/struct dentry \*lookup_fast/g' fs/namei.c
+sed -i 's/static long do_seccomp/long do_seccomp/g' kernel/seccomp.c


Adam-pi3 · 2023-10-13T23:45:01Z

src/modules/hashing/p_lkrg_fast_hash.c

@@ -26,11 +26,11 @@ uint128_t p_global_siphash_key;
 inline void p_lkrg_siphash(const uint8_t *in, const size_t inlen, const uint8_t *k,
                           uint8_t *out, const size_t outlen);

-notrace uint64_t p_lkrg_fast_hash(const char *p_data, unsigned int p_len) {
+notrace uint64_t p_lkrg_fast_hash(const unsigned char *p_data, unsigned int p_len) {


All code paths I found pass an unsigned char* (even casted to one) not a char* so corrected this to match the inputs. In the end it won't matter as the pointer is a pointer but I cam across it a few times looking through the code.

src/modules/hashing/p_lkrg_fast_hash.c

Adam-pi3 · 2023-10-13T23:47:43Z

src/modules/hashing/p_lkrg_fast_hash.c

@@ -65,25 +65,25 @@ notrace inline void p_lkrg_siphash(const uint8_t *in, const size_t inlen, const

   switch (left) {
      case 7:
-         b |= ((uint64_t)in[6]) << 48;
+         b |= ((uint64_t)end[6]) << 48;


Why? looks wrong, CC: @solardiz

All implementations I know use end not in, but once in reached = end in the for loop it should be the same. That was just changed for readability and to avoid a moving pointer.

Just reevaluated it a few times. in and end are equal after the loop, as in get bumped again but not processed by the for. So writing either end or in is pretty much the same. My reference for using end was mainly the Android / Linux kernel version here:

https://android.googlesource.com/kernel/common/+/3af7a2f61023/lib/siphash.c
https://github.com/torvalds/linux/blob/master/lib/siphash.c

I would leave it to @solardiz as he is more competent in that field

Adam-pi3 · 2023-10-13T23:47:55Z

src/modules/hashing/p_lkrg_fast_hash.h

@@ -78,6 +78,6 @@ typedef struct uint128_t {

 extern uint128_t p_global_siphash_key;

-uint64_t p_lkrg_fast_hash(const char *data, unsigned int len);
+uint64_t p_lkrg_fast_hash(const unsigned char *data, unsigned int len);


Same as the comment for the .c:

All code paths I found pass an unsigned char* (even casted to one) not a char* so corrected this to match the inputs. In the end it won't matter as the pointer is a pointer but I cam across it a few times looking through the code.

Adam-pi3 · 2023-10-13T23:50:35Z

src/modules/print_log/p_lkrg_print_log.h

@@ -137,6 +137,8 @@
   case P_LOG_FLOOD:                                                                       \
      p_print_ret = printk(KERN_DEBUG   P_LKRG_SIGNATURE "FLOOD: " p_fmt "\n", ## p_args); \
      break;                                                                               \
+   default:                                                                                \


I don't think it's needed since we manually inject specific type of log. If we decide to 'default' anyway, it should be an invasive break not silent break. E.g., it should include #error macro breaking compilation. @solardiz what do you think?

clang will shelf out a warning if you have no default in there. As I compile in-tree with CONFIG_WERROR the build will fail and I get a lot of warnings from clang about it. Other than that it doesn\t do much at all.

Can you add more context for that 'default' statement per my original comment?

Adam, I think we can add a default: to pacify compiler warnings. We just need to think where exactly to put it best. We can't do a #error since this would only be detected after the preprocessor stage (even if still at compile-time). We could maybe do a static_assert, but I think this is too minor a use case to start using it in this codebase.

Here is a snippet with some warning clang will give you for each p_print_log as reference. As clang being a pretty wide spread compiler these days I think it is important to deal with it. Technically you can alternatively add a case 0 but default caches other undefined settings as well.

In file included from /work/kernel/lkrg/src/modules/database/arch/arm/p_arm_metadata.c:29: In file included from /work/kernel/lkrg/src/modules/database/arch/arm/../../../../p_lkrg_main.h:435: In file included from /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/exploit_detection/p_exploit_detection.h:583: /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/exploit_detection/arch/x86/p_ed_x86_arch.h:186:25: warning: no case matching constant switch condition '0' p_print_log(P_LOG_ALERT, "BLOCK: CPU: SMAP got disabled, re-enabling"); ^~~~~~~~~~~ /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/print_log/p_lkrg_print_log.h:92:21: note: expanded from macro 'P_LOG_ALERT' #define P_LOG_ALERT 0 ^ /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/print_log/p_lkrg_print_log.h:112:12: note: expanded from macro 'p_print_log' switch (p_level) { \ ^~~~~~~ In file included from /work/kernel/lkrg/src/modules/database/arch/arm/p_arm_metadata.c:29: In file included from /work/kernel/lkrg/src/modules/database/arch/arm/../../../../p_lkrg_main.h:435: In file included from /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/exploit_detection/p_exploit_detection.h:583: /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/exploit_detection/arch/x86/p_ed_x86_arch.h:199:25: warning: no case matching constant switch condition '0' p_print_log(P_LOG_ALERT, "ALLOW: CPU: SMAP got disabled, accepting"); ^~~~~~~~~~~ /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/print_log/p_lkrg_print_log.h:92:21: note: expanded from macro 'P_LOG_ALERT' #define P_LOG_ALERT 0 ^ /work/kernel/lkrg/src/modules/database/arch/arm/../../../../modules/print_log/p_lkrg_print_log.h:112:12: note: expanded from macro 'p_print_log' switch (p_level) { \ ^~~~~~~ 15 warnings generated.

alexander-pick · 2023-10-21T11:18:16Z

Is there anything else I could help with in regards of this PR? Were the comments helpful so far?

some cosmetics fixed clang warnign with optimize flush_workqueue warning fixed, not optimal but better than werror for now

solardiz · 2023-10-21T17:22:39Z

Hi. Thank you @alexander-pick for contributing. I'm sorry I don't yet comment on the individual issues. I think that this PR as a whole is not to be merged(*), but there's valuable information here and many of the individual changes should get in, some in revised form.

(*) too many assorted changes, some are questionable, the separation into commits is historical rather than logical, commit author names are placeholders, there are merge commits instead of rebases

Adam-pi3 · 2023-10-22T19:20:01Z

I just replied to some of the comments. Sorry for delay - had busy week.

solardiz · 2023-10-23T22:36:04Z

Tuned out the modprobe has to be run from sudo -s instead just using sudo only to make it work. The only noticeable trace is the error dumping to dmesg if it fails. I assume this happens if kptr_restrict is set and the user who runs modprobe has no CP_SYSLOG permissions.

This makes no sense to me, but perhaps I am missing something. First, why would these different ways to use sudo affect availability of CAP_SYSLOG to modprobe? Then, why would CAP_SYSLOG matter here? Finally, what's the presumably relevant change here - is it not passing the command on sudo command-line, or is it using the -s option?

This looks like a maybe-workaround for some issue (or non-issue) that probably remains. Is this your understanding, too?

If so, perhaps this isn't a reliable workaround either. I really doubt we want to have it, and certainly we don't want to have the weird references to these guessed details in LKRG messages.

solardiz

I've made some comments. Overall, I think instead of this PR with a lot in it, we need separate PRs (initially just one, then more) for the different kinds of changes - e.g., start with fixing the typos and changing wording of messages and using proper committer name there - we'll merge that, then proceed to the next PR with slightly less trivial changes, etc. Eventually, we'd have decided on these all (accepted some, revised others, rejected the rest). Thanks!

solardiz · 2023-10-23T22:39:46Z

src/modules/exploit_detection/p_exploit_detection.c

   flush_workqueue(system_unbound_wq);
+#endif


Why is the change from flush_workqueue to __flush_workqueue needed, and why only on 6.6+? Is this possibly to workaround the warning? If so, that's not 6.6+ specific, is it?

Flushing the global workqueue will trigger a warning now. The discussion about it is from 2022 and it was introduced with 6.6rc1.

Reference:
torvalds/linux@20bdeda
https://github.com/torvalds/linux/blob/d88520ad73b79e71e3ddf08de335b8520ae41c5c/include/linux/workqueue.h#L619

solardiz · 2023-10-23T22:42:14Z

src/modules/integrity_timer/verify_kprobes/p_verify_kprobes.c

@@ -28,7 +28,7 @@ static struct kretprobe p_lkrg_dummy_kretprobe = {
    .entry_handler = p_lkrg_dummy_entry,
 };

-__attribute__((optimize(0)))
+__attribute__ ((optnone))


Why is the change from attribute__((optimize(0))) to __attribute__ ((optnone))?

The flag will be cause an unknown attribute warning with clang, optnone works. Possibly it needs to be changed to for full compatibility:

#ifdef __clang__ __attribute__ ((optnone)) #endif #ifdef __GNUC__ __attribute__((optimize(0))) #endif

solardiz · 2023-10-23T22:45:38Z

src/modules/print_log/p_lkrg_print_log.h

@@ -137,6 +137,8 @@
   case P_LOG_FLOOD:                                                                       \
      p_print_ret = printk(KERN_DEBUG   P_LKRG_SIGNATURE "FLOOD: " p_fmt "\n", ## p_args); \
      break;                                                                               \
+   default:                                                                                \


Adam, I think we can add a default: to pacify compiler warnings. We just need to think where exactly to put it best. We can't do a #error since this would only be detected after the preprocessor stage (even if still at compile-time). We could maybe do a static_assert, but I think this is too minor a use case to start using it in this codebase.

solardiz · 2023-10-23T22:53:13Z

For supporting zero-sized sections, I think we need to revise the hash function itself instead of its call sites. This also side-steps the question of which sections can or cannot be zero-size.

solardiz · 2023-10-23T22:56:21Z

added some checks to avoid hashes of zero sized sections so it's visible in debug log if this happens by setting the hash to 0xFFFFFFFF

What's the problem and goal of change here? Did anything misbehave at all? Or is it simply an enhancement to be able to see which sections are zero-sized? If so, why is that desirable?

solardiz · 2023-10-23T23:03:32Z

src/modules/exploit_detection/p_exploit_detection.c

@@ -162,7 +162,7 @@ static const struct p_functions_hooks {
     "Won't enforce validation on 'generic_permission'",
     1
   },
-#ifdef CONFIG_SECURITY_SELINUX
+#ifdef CONFIG_SECURITY_SELINUX_DEVELOP
   { "sel_write_enforce",


Assuming that this change from CONFIG_SECURITY_SELINUX to CONFIG_SECURITY_SELINUX_DEVELOP is correct, we also need to make it in src/modules/exploit_detection/syscalls/p_sel_write_enforce/p_sel_write_enforce.c. Otherwise we'll have dead code in some builds.

You can see it here that CONFIG_SECURITY_SELINUX_DEVELOP is the correct flag enabling this function:

Line 122:
https://github.com/torvalds/linux/blob/master/security/selinux/include/security.h

Good point about the dead code.

solardiz · 2023-10-23T23:05:25Z

src/modules/database/p_database.c

+                                                      (unsigned int)p_db.kernel_ex_table.p_size);
+   } else {
+      p_debug_log(P_LOG_DEBUG,
+          "hashing of _ex_table failed, is kptr_restrict set or CAP_SYSLOG missing?");


Why would kptr_restrict set or CAP_SYSLOG missing result in sections appearing to be zero-sized? This really makes no sense to me, in any of these places.

Well this is what I see with sudo only, works with sudo -s:

cat /proc/kallsyms | grep rodata 0000000000000000 T mark_rodata_ro 0000000000000000 D __start_rodata 0000000000000000 D rodata_enabled 0000000000000000 R __end_rodata 0000000000000000 R __end_rodata_aligned 0000000000000000 R __end_rodata_hpage_align 0000000000000000 d rodata_resource 0000000000000000 t set_debug_rodata 0000000000000000 d __setup_str_set_debug_rodata 0000000000000000 d __setup_set_debug_rodata 0000000000000000 t hash_from_kernel_rodata [lkrg]

I assumed kptr_restrict is the issue here as the module also had some issues. I didn't debug it in detail, another possible issue might be some issue with the kprobe hack used to get get_kallsyms_address. So I basically added some input validation to avoid a null pointer. The message is just info I added to see if something went bad.

About CAP_SYSLOG:
https://sysctl-explorer.net/kernel/kptr_restrict/

* corrected optimize attribute for clang/gcc

solardiz · 2023-10-25T12:53:58Z

@alexander-pick First of all, thank you for your effort. As I already mentioned, this PR as a whole is not merge-able. Do you want to start creating separate PRs for the different kinds of changes here, or should I or Adam be creating our own PRs based on your suggested changes here and crediting you indirectly (e.g., as Suggested-by or Co-authored-by, perhaps with your review)? Thanks again.

alexander-pick · 2023-10-25T19:43:21Z

No problem, you can do it as you want and credit me in some way as you feel it's best done. The PR originated in work I did for myself and some review I did for curiosity, that is why it sadly got a bit mixed up. But I felt like some of it might be valuable and hope it helps.

Your Name and others added 5 commits October 9, 2023 18:01

fixes for 6.x kernel

91a3564

fixed oob read if rodata is sized 0

ccef753

* fixed clang warnings

cabb63a

* fixed typo in include guard * fixed some other oob read and added a warning to debug output * clarified types in signature code * clarified readme as "sudo modprobe ..." will interfer with kptr_restrict if user has no CP_SYSLOG

small rollback, some small changes and type fixes

78f63e7

Merge branch 'lkrg-org:main' into main

9e3e979

Adam-pi3 requested changes Oct 13, 2023

View reviewed changes

Your Name added 2 commits October 21, 2023 18:33

now using kernel own siphash()

9d728a6

some cosmetics fixed clang warnign with optimize flush_workqueue warning fixed, not optimal but better than werror for now

Merge branch 'main' of https://github.com/alexander-pick/lkrg

a9b3e65

solardiz reviewed Oct 23, 2023

View reviewed changes

alexander-pick added 4 commits October 24, 2023 14:13

* fixed version for siphash

191c76d

* corrected optimize attribute for clang/gcc

Merge branch 'lkrg-org:main' into main

45d302a

removed the sudo -s on pi3 request

92b277d

adjusted for portability as discussed with pi3

bf379cc

Merge branch 'lkrg-org:main' into main

2565a3a

Some small fixes and possible valuable information #291

Are you sure you want to change the base?

Some small fixes and possible valuable information #291

Conversation

alexander-pick commented Oct 12, 2023

Description

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-pick Oct 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-pick Oct 24, 2023 • edited

Choose a reason for hiding this comment

alexander-pick commented Oct 21, 2023

solardiz commented Oct 21, 2023

Adam-pi3 commented Oct 22, 2023

solardiz commented Oct 23, 2023

solardiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solardiz commented Oct 23, 2023

solardiz commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solardiz commented Oct 25, 2023

alexander-pick commented Oct 25, 2023

alexander-pick Oct 14, 2023 •

edited

alexander-pick Oct 24, 2023 •

edited