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: <89159b33-3be4-487b-7647-0cbbd20c233d@meta.com>
Date:   Wed, 10 May 2023 16:54:12 -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
Content-Language: en-US
From:   Yonghong Song <yhs@meta.com>
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>
 <1195c4bd-ef54-2f1d-b079-2a11af42c62f@meta.com>
In-Reply-To: <1195c4bd-ef54-2f1d-b079-2a11af42c62f@meta.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QmIzRDBLdGZISC9xS3JITTZCTTczZXoyZnFQYVJ3RHdxRDU2N2ZOR21JaTJP?=
 =?utf-8?B?Yjhta08rMFhtaVlJbFg1NlV1UmhjZWxxSk9kcVBuc2lmWXRETHRIMGpLN2E4?=
 =?utf-8?B?aUk4WkxvdDVpK1VpQUQySXlpUDdZWDJFVnJjUGx5Q0pRd3VjMEZhUUpLZGgv?=
 =?utf-8?B?RU51SHc1L0o0V2YxS0JXb2lWUlZpNGF4YVJrbWNWcFNNRnYwdW9OT2JGR01U?=
 =?utf-8?B?aWo0QlhhUStzcGpWdmlMaTFXNURjNTNockVTeHdUQXQ2aG1hVk81M0ZuZERy?=
 =?utf-8?B?aFVtUjZjUG9UTlpjbW5DZS9rYkcvYlphUkROWXllSEJIOHZEemJldDlsLzhC?=
 =?utf-8?B?WE1veWdEY2F3OU1FaC80eTRxcDFPK3FFZ1U1cFdYTER4VVdDc0YzaU1ZZFB4?=
 =?utf-8?B?ZGhRM1RPUHNyeFp5bm11bko1YzFidk53U1hUTVNNaEIvL0JET0l0Yk5vYUUz?=
 =?utf-8?B?SU5pMTJRVmlaQ1VTWXBIUTZVUE9OdzY5Z2ZpZXA3RGwvTmxIT3FxSFZmTXp6?=
 =?utf-8?B?UzRlcUhFSTNuOWJFZ2F3Wm9JYkZhNWtSMDBXTHMxd1pDTUhVTEx5MHhRamNj?=
 =?utf-8?B?SStzK3hIRE5jQXYvSklxNkc0SW15YWxtMzM4cC9mV25JYWx0bFJCdWFSMTNQ?=
 =?utf-8?B?ZHN2MmZVSy9BRGJCUzBHdC81a2hJSjZZU0J3VzgrSmp6VjBpNTRkYkNNY0sy?=
 =?utf-8?B?VlBRUlVrVEZGOWF6cEd4K3JpNU1ZRFpKbnFDMkVpeTAyUjR2NXFTa1NZUkx4?=
 =?utf-8?B?M3ZzcWhoT3ZoTDI4VzYvcWw2OWhjdzNZR2VnZVQvcVdCdTBRRm5TdU9iTU9r?=
 =?utf-8?B?WXNyQVZDZUwvaHl2dWI2b0NDb282L3pyTFBFMjNnWWpLZ3FCSHduSnMvR3pu?=
 =?utf-8?B?Q1Y4RzRhOWwyRkRsejBzQ0VOZGtkK0RBZDFrSkZOa0hyekRYSURMWktqTUtV?=
 =?utf-8?B?d1dlb1J0M2ZSTTRicnVYa2tSMjd5aS9VRDhxWm8vL1NRVUtGRFg3dmFCYUNC?=
 =?utf-8?B?aU9RUmVPaEJJSnpFOXVVYzVlWk0wNWcrNUtjZ0NFdHZPS3FFZ0Y1SGFwWWUr?=
 =?utf-8?B?UUxUcExHd0RKdnBhaEQxcnFZM3FvZERwQ2d5T2d5ZHQrN2pjZ2JHazV5L01Q?=
 =?utf-8?B?OHBNT2FXWjlHQkYrVjdYajF6dzY3d1dSanR0eW15aElOcDRaS1dlM2lkT1RW?=
 =?utf-8?B?U2xEWEQrTmttQS9EMGtKKy95SlNoRjIwdGpFa1oydVFoaHhzUnhCUmdZbnhO?=
 =?utf-8?B?RXJ3aTB0MFNWYWNRQ1hlUXVSRkpKbzZuelFlQmsxUDBEdEZiMWdBVE9Samxp?=
 =?utf-8?B?SzBQN2F3WDJuV3BDT0dFVFFNbUEvL2hwWFdvRjhuZjVVUVRSeDVoN05BRHph?=
 =?utf-8?B?cmNYbUFydVk0RURYWkVnYW9QRERDeXRTa2pSU29tTW1CQXo2VWRPYTYwQW5T?=
 =?utf-8?B?bDEvYzE2Z0ZzNW9zR2owNTlYeEt3STY1ZktpQS9XZmo1UlgzVnlGT1lXYlNP?=
 =?utf-8?B?L1NzWnRDbXlIZkhNa0wzYmJtaE1VNS9pZTZ5M0ZlRk5BYXAzK0h2OTNaYXoy?=
 =?utf-8?B?MXFEOUdvaGRYd1lwWFVkeUdDQUx0MEFWWlVsRG1reXA1WU9uUEVkZVd2eWtt?=
 =?utf-8?B?eVBUeVIwR0N4M2EwRUN1RTJpa0NqK1FteTFtRDZ1bE0zRy9qU0MzVTIzQ3pN?=
 =?utf-8?B?dWEyNWl5TldEUmh4T25rTzZCMjl6Vi9xZXUvblI1R1gvazVyOG54b240RDhZ?=
 =?utf-8?B?bTFjdXd6emFpbzVtQUtROHorQ2t5NFh3VktCV1ZwV1NSQkZ3NzhMYXVvK1lC?=
 =?utf-8?B?OU80dDlyNWFNOUlTclJ0Qk5STTU2dDNKajFtL1o5M0dqdUs2eXE4aEpaY2Va?=
 =?utf-8?B?QUZ5QzJ1VW1Qc3ZJV1pRVjRrYWRuRkVsWXVJTFdraWhKQVlWZUdRMnorNXRC?=
 =?utf-8?B?Y2o5R2EvOEVEbU5WN2kvU2tyeXhGbTFqYWNwWUN3c2FMUE4rbXlwQ1RjWWx3?=
 =?utf-8?B?QkU3bnk2K2g3bTNqc2NvMFBWNzFRM0s0MXd1cWxlaUM3RURUWC90OCtMRFFG?=
 =?utf-8?B?bFpDRUFKYUhab0xoTk9BdmdHSWZ0MU1BT1NNYW10cjM4enhtT1RmdmlZQUxa?=
 =?utf-8?B?bTA3Z3dDM0FTTGg1dDJ0ZmtYSjNUZ2UyVDR1OEdjOXBLR2p1b1F0MmhFNklU?=
 =?utf-8?B?QVE9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 63a14f94-3172-432d-5f8a-08db51b1dcae
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2023 23:54:15.8290
 (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: bzJ0gRecLSlcod35iKhyhivZ7tw+AtsvGPO1bOZmMGQ8jbkqR/NAI+piT2600SG8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR15MB3924
X-Proofpoint-ORIG-GUID: oZtrdEr3YgHwqbt5e17M1pgr8XUgNsF2
X-Proofpoint-GUID: oZtrdEr3YgHwqbt5e17M1pgr8XUgNsF2
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 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 1:20 PM, Yonghong Song wrote:
> 
> 
> 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'?

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?

> 
>>
>> 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;