Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp195048rwd; Mon, 15 May 2023 22:59:51 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4SAcnNlhStONoGIJKXA5jYZMySuLwxOkNfdv3M4VvpWDCu28sCJvBmMvSgznQ3x3Lff096 X-Received: by 2002:a05:6a20:160a:b0:103:558c:513 with SMTP id l10-20020a056a20160a00b00103558c0513mr26390591pzj.57.1684216791151; Mon, 15 May 2023 22:59:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684216791; cv=none; d=google.com; s=arc-20160816; b=PoanZ2Hl90jiRGW1qt31tWCsgyTQ/YtIec2euExn2ubNCZpkS0MkhoVWO+oeGz6AGe cDNoNXRBBAW1NXM5vhrqCda9hueOiwoZdJQQ6mJ6OgxzTWCP3rTo7VhtOhpDbruvhJk5 d7mvwbvlW03+WS7WMl91n9xMFvQr8R7M7h4T7Yr/upgZtgw5A8Et/8fqzK5p5g42NRH/ 5sJnYoIki8yOo6pre+tHOo2C5nwJKNIr4zEvaBKi6WUyx6jDPaTI2c8x3mcS3Bi6XSPm jtncIcNGYNa5CGop7ZJJSfTA/8SQYyKxDYmJ4JoaSxs696joiYrTSlnj/6H7Aiu7X/zG n3SQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date :dkim-signature; bh=cU6MD85oPN2f2M0uSxRYWIzMO9+PWaNoKJmYeDDbfzo=; b=eTt0xvmFaR1QmW+22EDoPS+qutT7R9o8N205nN267A80p4ReCZxflGbnTRjjDdH+xO 8t+rlTt/lQKFuENAH57Vp4FbGfLuKWGol91MZ9oCYelPo0Ev8EgQxORgMnsZNmR/h/dQ l0rzMzZQWH3P2I5VhSn9H0lsdHTHh53cMl0LCw18afXpTbKGfpuwnMSd6tXnHR8qJexz ck5b4XZD+OB8P6ZnxU+tS35U3CMdWDuxkk2LqhmHM0DxNykB+oaDATTLaVCDpowrr0iq yYSZJ+LKNYF4VtNw/rA643smNHrvTU+md22o8XHsHyAJbAobFF5ll0C6jW1TaHrcWZHh FYDQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=nA86npDt; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id v184-20020a6389c1000000b00520a074168csi17274990pgd.454.2023.05.15.22.59.36; Mon, 15 May 2023 22:59:51 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=nA86npDt; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229662AbjEPFtQ (ORCPT + 99 others); Tue, 16 May 2023 01:49:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55354 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229510AbjEPFtO (ORCPT ); Tue, 16 May 2023 01:49:14 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 18CF03C32; Mon, 15 May 2023 22:49:13 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A309662F25; Tue, 16 May 2023 05:49:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 95401C433D2; Tue, 16 May 2023 05:49:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1684216152; bh=x1pTlw1V19z0aDBofMrV/rBF4TFc3StzNYctHqoKgiM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=nA86npDtnLbRixJeEJg4inJyd169D0VJZZ0/sLDEFPzRPkAe7vbxJMs2z2lbPG5km q6WbR7Rs4E52slK0McHeP0OyGPuNF+ExqWT5d3ufMurZH9QEo/ZNoaYHlVuElFxaSG S0vxArdko6V6Kaxgc812/k7NIYblAYXd/UR4l410vUMJ1DrAVlwZyjVeVx0Tn0P3n+ WvIW2rRvuKfKWZATQlTz+siXoCIM43k2k3uv2PLjUs4ElBJ1z3i3VuoLp99BCdKtiU MPEPHHqEQsfSQUen/dA9KH1krrySABXFMYaD80jLM8zUuNr4XBGuTfieqCno/Ui6k5 UTuTpfTQTWUCQ== Date: Tue, 16 May 2023 14:49:08 +0900 From: Masami Hiramatsu (Google) To: Yonghong Song Cc: Ze Gao , Jiri Olsa , Song Liu , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Steven Rostedt , Masami Hiramatsu , Ze Gao , bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Subject: Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid recursive trap Message-Id: <20230516144908.f062ab19327634fc40cfc3b7@kernel.org> In-Reply-To: References: <20230510122045.2259-1-zegao@tencent.com> <6308b8e0-8a54-e574-a312-0a97cfbf810c@meta.com> X-Mailer: Sylpheed 3.8.0beta1 (GTK+ 2.24.33; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 12 May 2023 07:29:02 -0700 Yonghong Song wrote: > > > On 5/11/23 10:53 PM, Ze Gao wrote: > > Yes, Jiri. Thanks for pointing it out. It's true that not all probe > > blacklisted functions should be banned from bpf_kprobe. > > > > I tried some of them, and all kprobe blacklisted symbols I hooked > > works fine except preempt_count_{sub, add}. > > so the takeaway here is preempt_cout_{sub, add} must be rejected at > > least for now since kprobe_multi_link_prog_run > > ( i.e., the fprobe handler) and rethook_trampoline_handler( i.e. the > > rethook handler) calls preempt_cout_{sub, add}. > > > > I'm considering providing a general fprobe_blacklist framework just > > like what kprobe does to allow others to mark > > functions used inside fprobe handler or rethook handler as NOFPROBE to > > avoid potential stack recursion. But only after > > I figure out how ftrace handles recursion problems currently and why > > it fails in the case I ran into. > > A fprobe_blacklist might make sense indeed as fprobe and kprobe are > quite different... Thanks for working on this. No, I don't like fprobe_blacklist, because you can filter user given function with /available_filter_functions :) If the function is not listed there, you can not put fprobe on it. IOW, kprobe_multi_link_prog_run only covers those functions. (white-list) At the tooling side, it should check whether the probe is defined for single function or multiple functions, and use kprobe-blacklist (single) or available_filter_functions (multiple). Thank you, > > > > > Thanks > > Ze > > > > On Thu, May 11, 2023 at 1:28 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 > >>>> --- > >>>> 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 > >> > >> 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; -- Masami Hiramatsu (Google)