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;
Message-ID: <1195c4bd-ef54-2f1d-b079-2a11af42c62f@meta.com>
Date:   Wed, 10 May 2023 13:20:30 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.10.1
Subject: Re: [PATCH] bpf: reject blacklisted symbols in kprobe_multi to avoid
 recursive trap
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     Ze Gao <zegao2021@gmail.com>, Song Liu <song@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Ze Gao <zegao@tencent.com>, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
References: <20230510122045.2259-1-zegao@tencent.com>
 <6308b8e0-8a54-e574-a312-0a97cfbf810c@meta.com> <ZFvUH+p0ebcgnwEg@krava>
Content-Language: en-US
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <ZFvUH+p0ebcgnwEg@krava>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WlpZWjFhSGdTclRnWFRKa0dBZFZNSGRMb25zc2grREtPQVpyWFV3YkZYUEJz?=
 =?utf-8?B?bTFZSWd5Vnk4ZFdxWmpKMGo0aTkrbTBpZ1JKWjBRNjBkRVNkZVdVc2piL0gv?=
 =?utf-8?B?c0lFUDJrSXMrRk5UVWdhS04vV3RyZkZzZmU5aXZSZTNyOWF5RVhNcmJzVzZz?=
 =?utf-8?B?c29zSFdMeDVhYmNxWElhd1dRZzdTQ3lhN2VHbkVPa1lseW1pUmhVVjFUaHpy?=
 =?utf-8?B?WHIwL2xFaFBKSXg3MDkwWUQyWElTdlBoZVg5R2ZDTHplWGtpaG55SmYzZkRY?=
 =?utf-8?B?bklqZElqS01MTGJDWklOaTJMTVNjZ0RqNTI3eFNSaG1qbWljdkJRckk0R25P?=
 =?utf-8?B?TTlqc0F4Y3AwcjM1RXBGUUdUYmJ3aUZmc3F1aTYya2dQQ2FwWmswRGhrcEhZ?=
 =?utf-8?B?aVNoWlRvRXZiak9Qa1FDczFpMlVMN1ZqNUxQMzVLTG1YVGl5OTFmazByc0tn?=
 =?utf-8?B?VGlOMEdLWWh3bXBEUDBFZWp6dXJOM2pWeFVaME5GWHhMdkhHUnNQWjQ1YU1i?=
 =?utf-8?B?NG82N09JNW12cHA5OGtSR2xpZ1NmeEpLMVJ3R21yMm5NK1oreG1QbmNHZlJQ?=
 =?utf-8?B?S1dmcC9aamFRMnF5elZUSjJvRGFucGxPUnhwTWdveVZUYlFKQmd3Uy9sVHFz?=
 =?utf-8?B?anhlZTJ4UFpodmE0ZWM3S3VuaUJ2UEVDUXltYlkvV0YvL2IxbWd3QUVPODZS?=
 =?utf-8?B?YittN3RacEx4TTZ1NWZsandkK2xHbVNOTitLaXo5Y1RHdGd5c2RWOW92STQ5?=
 =?utf-8?B?S2E1YWs1WEtpb0FMMjBBS3UrNFpjeTN1eDQrSXpxeG5HVXdQODlmNE10MTZr?=
 =?utf-8?B?dnJXM2JxMmVFYm9Wc01BYjU5ODJTMzhWaUFVcjEzNmVkOWtRR2tpVFhpdVhR?=
 =?utf-8?B?MkE1bll5MlNhQ2F6TWN4WXJORkZtL0xUOXhJM0xMWVpKZlV3bGhFV1pLdGVQ?=
 =?utf-8?B?Sk03TjJMWmxHYmczNGtPaFpPUkVIa1Vjb3UzSTV2OHF4eHM4djI2bXI0T20r?=
 =?utf-8?B?RCtwUmN5VHpTWWV5dHdtYkVPMTkrVGFOeVpqVW5tNmFTa1lyM0tGUXhqQXlZ?=
 =?utf-8?B?VEZNeEFLYW9VVEp4WTJDNTZjdFl6MFVJTmRZY09CUVExcUJEUHJLWm9Xa3BC?=
 =?utf-8?B?WDY5bW52bXliRldtZzdBaU1aYzZVak11V2N3OWpkQ3JlU1QzaGJkVC9rcjJH?=
 =?utf-8?B?MHEzVVA4K1FGS0pmUi9udGFobzkyZTNoN1BRV0J2YTZjZnROc3BWeENkWCta?=
 =?utf-8?B?ekdTdjVrTTNrUHdkb20rVU5TTEgvOU0yNkRTNnRWWGlxZTlpNDJHd21qMzV6?=
 =?utf-8?B?YlhCUHkyb0JaQnNLRUt2K2FYWndpY1pTczJyTm5nUjBvbGNsMnBIQjVlWGtU?=
 =?utf-8?B?OE8yU1VncnEzcDJOcDJiYmJNdkJjb3JzcWQ2azVtdGhycGJoNUZFdmlPN2li?=
 =?utf-8?B?ZEtyKzhLU3hhTHAvL3pDc3llN0lnT2hRdkR5Y2l1cjdEUjVZWm5Bb0dTVVVZ?=
 =?utf-8?B?R3ZyeEVOTUFackJzSU1vMXA2WTBudUtiTHcrNXpsVVpwc2VKS0FXd0h1ZGw1?=
 =?utf-8?B?Um4vekM5bFh6SWdGNjFxaml0UkVvSU5iK0tUOVB3eVRISlkzNUVKdVIzZlYy?=
 =?utf-8?B?QlNLNkZNdkFFNzF1RWRtSlFVaHlaZWhWRXZJOWJkTHpUT1I1amt4MkdJdm1o?=
 =?utf-8?B?MzVnT0VmeE11blJjWllZNE5jREpJSk5nWUFOSXhEOTZycU9ReDNTUjUyUkN0?=
 =?utf-8?B?VVJWb2p4Y0tVYzdqZk43R0VOTjNhN1RqYTd1YWorRlY3UGdXR21lRkROVFdh?=
 =?utf-8?B?YVJudytUTnJiYXEwMUlBUDA1QXI4VERHTmxaMGFQdEd3U2VGOXBFL0R1RkIr?=
 =?utf-8?B?aXZpenhCN0NNZ2NZMmprOFlaL2JGS2pyL0N4RDNXVDNjMlBsMUx5S0YxNGZq?=
 =?utf-8?B?RXQ5a1Z4NC83WWhNbzAwNWwvaUZWdElVb3YxUHdDNFJEd0NkM3Q1enI4K1Zl?=
 =?utf-8?B?QkZIWktoUWpUcm5lcWowRDJQK3o5MzFEL2I3NW50dDMxTERac0lKQ1FLZzg5?=
 =?utf-8?B?T3FtdXJ2N09WdWFvcWllYVZ3cjQyZnBhTDQrbFd1VFlGRFA2M0tmN0M0YzdO?=
 =?utf-8?B?bi9NTno4a1pJaGxGTlFNNFV4cmNKbVl4azV0Z2NnTU1NQ05jNmx5eUVGMnZS?=
 =?utf-8?B?bnc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d39e2250-071a-4f54-b751-08db519402c4
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2023 20:20:34.9115
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KdtOiI8CZQp0Z5GRkz+/tsSP3gxklSvHyISA5ZA/IIjTmi6jb6a0sF62wmuLyDJP
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR15MB3859
X-Proofpoint-GUID: bWN-3fyqV8N449bdHH6upYe3g5-R4rKe
X-Proofpoint-ORIG-GUID: bWN-3fyqV8N449bdHH6upYe3g5-R4rKe
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-10_04,2023-05-05_01,2023-02-09_01
X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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;