Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid recursive trap

[Yonghong Song <yhs@meta.com>]

On 5/10/23 10:27 AM, Jiri Olsa wrote:
> On Wed, May 10, 2023 at 07:13:58AM -0700, Yonghong Song wrote:
>>
>>
>> On 5/10/23 5:20 AM, Ze Gao wrote:
>>> 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 <zegao@tencent.com>
>>> ---
>>>    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'?

> 
> jirka
> 
>>
>>> +			/* 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;