On 5/10/23 1:20 PM, Yonghong Song wrote:

On 5/10/23 10:27 AM, Jiri Olsa wrote:

quoted

On Wed, May 10, 2023 at 07:13:58AM -0700, Yonghong Song wrote:

quoted

On 5/10/23 5:20 AM, Ze Gao wrote:

quoted

BPF_LINK_TYPE_KPROBE_MULTI attaches kprobe programs through fprobe,
however it does not takes those kprobe blacklisted into consideration,
which likely introduce recursive traps and blows up stacks.

this patch adds simple check and remove those are in kprobe_blacklist
from one fprobe during bpf_kprobe_multi_link_attach. And also
check_kprobe_address_safe is open for more future checks.

note that ftrace provides recursion detection mechanism, but for kprobe
only, we can directly reject those cases early without turning to 
ftrace.

Signed-off-by: Ze Gao <redacted>
---
   kernel/trace/bpf_trace.c | 37 +++++++++++++++++++++++++++++++++++++
   1 file changed, 37 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9a050e36dc6c..44c68bc06bbd 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -2764,6 +2764,37 @@ static int get_modules_for_addrs(struct 

module ***mods, unsigned long *addrs, u3
       return arr.mods_cnt;
   }
+static inline int check_kprobe_address_safe(unsigned long addr)
+{
+    if (within_kprobe_blacklist(addr))
+        return -EINVAL;
+    else
+        return 0;
+}
+
+static int check_bpf_kprobe_addrs_safe(unsigned long *addrs, int num)
+{
+    int i, cnt;
+    char symname[KSYM_NAME_LEN];
+
+    for (i = 0; i < num; ++i) {
+        if (check_kprobe_address_safe((unsigned long)addrs[i])) {
+            lookup_symbol_name(addrs[i], symname);
+            pr_warn("bpf_kprobe: %s at %lx is blacklisted\n", 
symname, addrs[i]);

So user request cannot be fulfilled and a warning is issued and some
of user requests are discarded and the rest is proceeded. Does not
sound a good idea.

Maybe we should do filtering in user space, e.g., in libbpf, check
/sys/kernel/debug/kprobes/blacklist and return error
earlier? bpftrace/libbpf-tools/bcc-tools all do filtering before
requesting kprobe in the kernel.

also fprobe uses ftrace drectly without paths in kprobe, so I wonder
some of the kprobe blacklisted functions are actually safe

Could you give a pointer about 'some of the kprobe blacklisted
functions are actually safe'?

Thanks Jiri for answering my question. it is not clear whether
kprobe blacklist == fprobe blacklist, probably not.

You mentioned:
   note that ftrace provides recursion detection mechanism,
   but for kprobe only
Maybe the right choice is to improve ftrace to provide recursion
detection mechanism for fprobe as well?

quoted

jirka

quoted

quoted

+            /* mark blacklisted symbol for remove */
+            addrs[i] = 0;
+        }
+    }
+
+    /* remove blacklisted symbol from addrs */
+    for (i = 0, cnt = 0; i < num; ++i) {
+        if (addrs[i])
+            addrs[cnt++]  = addrs[i];
+    }
+
+    return cnt;
+}
+
   int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, 
struct bpf_prog *prog)
   {
       struct bpf_kprobe_multi_link *link = NULL;

@@ -2859,6 +2890,12 @@ int bpf_kprobe_multi_link_attach(const union 

bpf_attr *attr, struct bpf_prog *pr
       else
           link->fp.entry_handler = kprobe_multi_link_handler;
+    cnt = check_bpf_kprobe_addrs_safe(addrs, cnt);
+    if (!cnt) {
+        err = -EINVAL;
+        goto error;
+    }
+
       link->addrs = addrs;
       link->cookies = cookies;
       link->cnt = cnt;