Don't unpack kernel headers or look in tmp #3156
Conversation
|
Do we need another patch release after we land this? Considering I didn't really fix the problem before. cc @danobi |
I would prefer that. Since it's a CVE, it'd make the life of us distro packagers easier. FWIW, I think that we should also do a regular release soon, preferably with this merged. |
| LOG(WARNING) << "Could not find kernel headers in " << ksrc << " or " | ||
| << kobj | ||
| << ". To specify a particular path to kernel headers, set the " | ||
| "env variables BPFTRACE_KERNEL_SOURCE and, optionally, " | ||
| "BPFTRACE_KERNEL_BUILD if the kernel was built in a " | ||
| "different directory than its source. To create kernel " | ||
| "headers run 'modprobe kheaders', which will create a tar " | ||
| "file at /sys/kernel/kheaders.tar.xz"; |
There was a problem hiding this comment.
I imagine that in most cases bpftrace doesn't need kheaders (can use BTF data instead); would it make sense to only print this warning if kernel headers end up being required?
A couple of cases come to mind:
- Bpftrace script unconditionally includes one or more kernel headers (e.g.
#include <linux/sched.h>without gating onBPFTRACE_HAVE_BTF) - Conflicts between user-defined and BTF types
Related: commit 91db77c5
cc @danobi
There was a problem hiding this comment.
This is a good point. I agree that we should not print the warning by default as there's a number of situations where kheaders are not necessary.
There was a problem hiding this comment.
How about as a stop-gap (in the interest of getting this patched), I just reinstate the !bpftrace.feature_->has_btf() check as a conditional to show the warning?
There was a problem hiding this comment.
I may remember it wrong (it's been a while since I wrote that code) but IMHO kheaders are needed iff ClangParser runs so we should use the same condition as in clang_parser.cpp:663:
if (program->c_definitions.empty() && bpftrace.btf_set_.empty())It'd be nice if we could reuse it.
An alternative would be to issue the warning from ClangParser when it fails but that may be even more complicated.
There was a problem hiding this comment.
I moved a few things around but I'm using this same check before even calling get_kernel_dirs so the warning should only be shown in that case.
jordalgo
force-pushed
the
kheaders
branch
2 times, most recently
from
df6a68f
to
1730a16
Compare
Looking in shared writeable locations for kernel headers is inherently risky even bpftrace does the unpacking. Remove this functionality and let the user specify the path to these headers if we can't find them in known locations. References: bpftrace#3033 bpftrace#3154
| @@ -660,9 +660,6 @@ bool ClangParser::parse(ast::Program *program, | |||
| StderrSilencer silencer; | |||
| silencer.silence(); | |||
| #endif | |||
| if (program->c_definitions.empty() && bpftrace.btf_set_.empty()) | |||
There was a problem hiding this comment.
moved this into main
There was a problem hiding this comment.
Nice, I like this approach.
Technically speaking, ClangParser may run successfully without kheaders (e.g. when user only defines custom types) but I'm ok with issuing a warning for the sake of fixing this. In most cases, BTF is present and so no warning will be shown.
Ah ok, do we have a way to check for this already? |
I don't think so. |
|
Ok, gonna merge in a few hours unless there are any objections. |
Looking in shared writeable locations for kernel headers is inherently risky even bpftrace does the unpacking. Remove this functionality and let the user specify the path to these headers if we can't find them in known locations. References: bpftrace#3033 bpftrace#3154 Co-authored-by: Jordan Rome <jordalgo@fedoraproject.org>
Commit 14cb69d added the "/bpftrace/include" prefix to headers generated by bpftrace (__btf_generated_header.h, clang_workarounds.h), however commit bc73244 ("Don't unpack kernel headers or look in tmp (bpftrace#3156)") broke the latter when moving around code in main.cpp. Reintroduce the prefix.
Commit 14cb69d added the "/bpftrace/include" prefix to headers generated by bpftrace (__btf_generated_header.h, clang_workarounds.h), however, commit bc73244 ("Don't unpack kernel headers or look in tmp (bpftrace#3156)") broke the latter when moving code around in main.cpp. Reintroduce the prefix.
Looking in shared writeable locations for kernel headers is inherently risky even bpftrace does the unpacking. Remove this functionality and let the user specify the path to these headers if we can't find them in known locations. References: #3033 #3154 Co-authored-by: Jordan Rome <jordalgo@fedoraproject.org>
Looking in shared writeable locations for kernel
headers is inherently risky even bpftrace does
the unpacking. Remove this functionality and let
the user specify the path to these headers if
we can't find them in known locations.
References:
#3033
#3154
Checklist
man/adoc/bpftrace.adocCHANGELOG.md