-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some small fixes and possible valuable information #291
Conversation
* 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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already set at line 9 of this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already export that symbol, why do you need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, it fixes modpost errors due to local syms.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also exported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, it removes the static keyword, not adds an export, it's just to solve modpost errors.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? looks wrong, CC: @solardiz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave it to @solardiz as he is more competent in that field
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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: \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more context for that 'default' statement per my original comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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 |
I just replied to some of the comments. Sorry for delay - had busy week. |
This makes no sense to me, but perhaps I am missing something. First, why would these different ways to use 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
flush_workqueue(system_unbound_wq); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -28,7 +28,7 @@ static struct kretprobe p_lkrg_dummy_kretprobe = { | |||
.entry_handler = p_lkrg_dummy_entry, | |||
}; | |||
|
|||
__attribute__((optimize(0))) | |||
__attribute__ ((optnone)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the change from attribute__((optimize(0)))
to __attribute__ ((optnone))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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: \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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? |
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that CONFIG_SECURITY_SELINUX_DEVELOP
is the correct flag to check, but the source code link in the comment above isn't for sel_write_enforce
. Here's the correct snippet, from git blame security/selinux/selinuxfs.c
:
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 134) #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
1872981b51dac (Eric Paris 2008-04-17 14:15:45 -0400 135) static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 136) size_t count, loff_t *ppos)
I used git blame
to ensure there was no recent change - in other words, that this check would be valid across our range of supported kernels.
(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?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About CAP_SYSLOG:
https://sysctl-explorer.net/kernel/kptr_restrict/
* corrected optimize attribute for clang/gcc
@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. |
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. |
I've proceeded to take pieces from this PR into separate commits and am testing them in my fork of the repo. I use |
My commits so far passed tests, so pushed to this repo as well. There are still many more things to take from this PR. |
See #291 Co-authored-by: Alexander Pick <[email protected]>
See lkrg-org#291 although it doesn't make clear whether any such clashes were observed yet. Co-authored-by: Alexander Pick <[email protected]>
For SipHash, I see several kinds of changes here:
Regarding 1, I am not sure. On one hand, it's good to make LKRG smaller (in terms of compiled code) and use what's maintained upstream. On the other, we'll need to maintain and test our own implementation anyway (for older kernels) and using the kernel's may go against our security model. We compare those hashes in case the kernel or a module are modified - to hopefully detect that and respond to it. If we use the kernel's SipHash, we trust it, including with the per-boot random key passed to it as a function argument. So we'd possibly make it a bit easier for a kernel rootkit or such to be undetected by LKRG, as it could easily intercept SipHash and compute/return updated hashes that would be valid for the key we use. They could probably intercept LKRG's SipHash just as well, but at least that's something we should defend against in future versions of LKRG (make it harder to locate). For a kernel's exported symbol, we would have no reasonable recourse other than stopping to use it. For 2, I'm merging this change (simplified). For 3, I'm adding an For 4, I am not sure. I'm not doing anything yet. If we're to clean up the code, we could as well make even more clean-ups. For example, if/when we switch to Linux kernel's coding style, we could maybe have the code almost match what's used there, including indentation. |
Actually, I see nothing to revise - it appears to work just fine on empty input as-is: #include <stdio.h>
#include <stdint.h>
#include "p_lkrg_fast_hash.h"
int main(void)
{
printf("%llx\n", (unsigned long long)p_lkrg_fast_hash(NULL, 0));
return 0;
} prints @alexander-pick Can you please explain why you wanted to handle zero-sized sections specially? I see currently no reason, but maybe I'm missing something. |
Searching for this value finds it in https://github.com/havenwood/digest-sip_hash so either it's correct (I hope) or we have a common bug. ;-) |
@alexander-pick I think I'm done revising and merging what was needed from this PR. Can you please take a look and test the current revision (main branch in this repo) and confirm if it's OK to close this PR as completed? If you're still getting any compiler warnings, please let us know. Thank you! |
Description
I was hunting down a global oobr triggered in kasan. Tuned out the modprobe has to be run from
sudo -s
instead just usingsudo
only to make it work. The only noticeable trace is the error dumping to dmesg if it fails. I assume this happens ifkptr_restrict
is set and the user who runsmodprobe
has noCP_SYSLOG
permissions.During the process I made a few changes which might be valuable:
README
add-exports.sh
, otherwise my 6.x kernel refused to compile (using clang)CONFIG_SECURITY_SELINUX
vs.CONFIG_SECURITY_SELINUX_DEVELOP
which would enablesel_write_enforce
P_LKRG_EXPLOIT_DETECTION_SECURTIY_PTRACE_ACCESS_H
uint64_t
and the next line shares this assumption by using& 7
How Has This Been Tested?
Tested the build a lot of times on multiple systems with different configuration