Message ID | 20221116154712.4115929-1-roberto.sassu@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp214716wru; Wed, 16 Nov 2022 07:51:02 -0800 (PST) X-Google-Smtp-Source: AA0mqf5dUEoY1SkmO/V374ZUeME2Bd+Xx2rVFSJGeDn0xd55serPG6c+Gd97njLVT3xcBVnfUmcC X-Received: by 2002:a17:907:c296:b0:78d:3f87:1725 with SMTP id tk22-20020a170907c29600b0078d3f871725mr17950799ejc.492.1668613862094; Wed, 16 Nov 2022 07:51:02 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668613862; cv=none; d=google.com; s=arc-20160816; b=uB1qFRqkz4AGl85fg40WWiiGcnRb13ZIFC76SaOJpGDvxzQDIIts9XrNZfoDXSzAer KAmmZEP7J5UekAcxiWk2rrSSmQNMFvWvocbMklvFRYEyUUWqwKnEU7HtSDuGNPqZWd10 sWDeoE2GoNDBwmhq6pz6HfNGWkkQORuLeQS5YU2EkYnX/SJTo8MAwvm1K5IB+RzHcAyV UNbPNqNGbbWjU2pPOm4zZWHlnmwTtzLvnZY+/J5VTQoZDuYGposg4nKz9wvWfJTPxS+g 0MkF5UfLUbW/kDJRSMRy1vz/JInHm7ENWYxo5H6xHBKBHZnsNQeSfni8gPQWrWrtl1Zr 7sdg== 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:date:subject:cc:to:from; bh=Dq7EVsHY0d2RILm6XzgN53P997y0Dqq2jURPZ3skz20=; b=bqvpaiwFpYJxWdmH/eYRzCG1vU9/wcQT+iQg4Exp/5OFxeEqYrIhFaOuRIUKsphh7x pRzXbiUmpMi8qdCsqIXXK6Y8mXOPdw+5kuBH4Rp0CmMvAhKQ97Tc1aVxH7Opa5r24Dzf NmA3EzWimRnVh1pPyoaS7huhmsyA5mSAqOmY9xfiFyvSKW5CxOYonZr4cDQuU37Wfcv3 coODU0fAlWk81EImLl0vSDm5aNqzSwPF5rLjwbdmFL8gY5O97qbdxKgFn0sfwg3vkd6q /3PycZEXH7wqZ+Y/XUa7JtKoZvTF+fTfukW1Pkh0MsvWY88H9UhFEtH0uU9xPugdV+mG wTqA== ARC-Authentication-Results: i=1; mx.google.com; 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 Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t23-20020a1709064f1700b007a7b4b41ccasi11035842eju.562.2022.11.16.07.50.37; Wed, 16 Nov 2022 07:51:02 -0800 (PST) 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; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233216AbiKPPsk (ORCPT <rfc822;just.gull.subs@gmail.com> + 99 others); Wed, 16 Nov 2022 10:48:40 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59784 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233202AbiKPPsf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 16 Nov 2022 10:48:35 -0500 Received: from frasgout12.his.huawei.com (frasgout12.his.huawei.com [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1DD8F4C24C; Wed, 16 Nov 2022 07:48:08 -0800 (PST) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4NC6jh0qjRz9v7P5; Wed, 16 Nov 2022 23:42:00 +0800 (CST) Received: from huaweicloud.com (unknown [10.204.63.22]) by APP2 (Coremail) with SMTP id GxC2BwDnGvcVBnVjzoJsAA--.18617S2; Wed, 16 Nov 2022 16:47:44 +0100 (CET) From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com, haoluo@google.com, jolsa@kernel.org, revest@chromium.org, jackmanb@chromium.org, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com Cc: bpf@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Roberto Sassu <roberto.sassu@huawei.com> Subject: [PoC][PATCH] bpf: Call return value check function in the JITed code Date: Wed, 16 Nov 2022 16:47:12 +0100 Message-Id: <20221116154712.4115929-1-roberto.sassu@huaweicloud.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <700dffccdfeeb3d19c5385550e4c84f08c705e19.camel@huaweicloud.com> References: <700dffccdfeeb3d19c5385550e4c84f08c705e19.camel@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: GxC2BwDnGvcVBnVjzoJsAA--.18617S2 X-Coremail-Antispam: 1UD129KBjvJXoW3GF4DuFW5Cw1UKw18Wr17Wrg_yoWDGFW3pa 18JFy5Cr48Xa17X3Z7Ja1kZr4av34kX3y7GFWUGrWFka9IqrykXF1rGF1Yvry5KrZ09w1S yF4Yvryqk3WUX37anT9S1TB71UUUUUDqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvIb4IE77IF4wAFF20E14v26ryj6rWUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxV AFwI0_Gr1j6F4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG 6I80ewAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFV Cjc4AY6r1j6r4UM4x0Y48IcxkI7VAKI48JM4IIrI8v6xkF7I0E8cxan2IY04v7MxkF7I0E n4kS14v26rWY6Fy7MxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I 0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVW8 ZVWrXwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcV CY1x0267AKxVW8JVWxJwCI42IY6xAIw20EY4v20xvaj40_Gr0_Zr1lIxAIcVC2z280aVAF wI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVW8Jr0_Cr1UYxBIdaVFxhVjvjDU0xZFpf 9x07jT7KsUUUUU= X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAgASBF1jj4GEmAAAs4 X-CFilter-Loop: Reflected X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749668449037756850?= X-GMAIL-MSGID: =?utf-8?q?1749668449037756850?= |
Series |
[PoC] bpf: Call return value check function in the JITed code
|
|
Commit Message
Roberto Sassu
Nov. 16, 2022, 3:47 p.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com> eBPF allows certain types of eBPF programs to modify the return value of the functions they attach to. This is used for example by BPF LSM to let security modules make their decision on LSM hooks. The JITed code looks like the following: ret = bpf_lsm_inode_permission_impl1(); // from a security module if (ret) goto out; ... ret = bpf_lsm_inode_permission_implN(); // from a security module if (ret) goto out; ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT out: If ret is not zero, the attachment points of BPF LSM are not executed. For this reason, the return value check cannot be done there. Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check function. Whenever an eBPF program attaches to an LSM hook, the eBPF verifier resolves the address of the check function (whose name is bpf_lsm_<hook name>_ret()) and adds a call to that function just after the out label. If the return value is illegal, the check function changes it back to the default value defined by the LSM infrastructure: ... out: ret = bpf_lsm_inode_permission_ret(ret); In this way, an eBPF program cannot cause illegal return values to be sent to BPF LSM, and to the callers of the LSM infrastructure. This is just a PoC, to validate the idea and get an early feedback. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- arch/arm64/net/bpf_jit_comp.c | 7 ++++--- arch/x86/net/bpf_jit_comp.c | 17 ++++++++++++++++- include/linux/bpf.h | 4 +++- kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++ kernel/bpf/bpf_struct_ops.c | 2 +- kernel/bpf/trampoline.c | 6 ++++-- kernel/bpf/verifier.c | 28 ++++++++++++++++++++++++++-- 7 files changed, 74 insertions(+), 10 deletions(-)
Comments
On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > +{ > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > + return false; > + > + return true; > +} > + > /* For every LSM hook that allows attachment of BPF programs, declare a nop > * function where a BPF program can be attached. > */ > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > +{ \ > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > +} > + > +#include <linux/lsm_hook_defs.h> > +#undef LSM_HOOK > + because lsm hooks is mess of undocumented return values your "solution" is to add hundreds of noninline functions and hack the call into them in JITs ?! That's an obvious no-go. Not sure why you bothered to implement it.
On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > +{ > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > + return false; > > + > > + return true; > > +} > > + > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > * function where a BPF program can be attached. > > */ > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > #include <linux/lsm_hook_defs.h> > > #undef LSM_HOOK > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > +{ \ > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > +} > > + > > +#include <linux/lsm_hook_defs.h> > > +#undef LSM_HOOK > > + > > because lsm hooks is mess of undocumented return values your > "solution" is to add hundreds of noninline functions > and hack the call into them in JITs ?! I revisited the documentation and checked each LSM hook one by one. Hopefully, I completed it correctly, but I would review again (others are also welcome to do it). Not sure if there is a more efficient way. Do you have any idea? Maybe we find a way to use only one check function (by reusing the address of the attachment point?). Regarding the JIT approach, I didn't find a reliable solution for using just the verifier. As I wrote to you, there could be the case where the range can include positive values, despite the possible return values are zero and -EACCES. # ./test_progs-no_alu32 -t libbpf_get_fd *reg = {type = SCALAR_VALUE, off = 0, {range = 0, {map_ptr = 0x0 <fixed_percpu_data>, map_uid = 0}, {btf = 0x0 <fixed_percpu_data>, btf_id = 0}, mem_size = 0, dynptr = {type = BPF_DYNPTR_TYPE_INVALID, first_slot = false}, raw = {raw1 = 0, raw2 = 0}, subprogno = 0}, id = 0, ref_obj_id = 0, var_off = {value = 0, mask = 18446744073709551603}, smin_value = -9223372036854775808, smax_value = 9223372036854775795, umin_value = 0, umax_value = 18446744073709551603, s32_min_value = -2147483648, s32_max_value = 2147483635, u32_min_value = 0, u32_max_value = 4294967283, parent = 0x0 <fixed_percpu_data>, frameno = 0, subreg_def = 0, live = REG_LIVE_WRITTEN, precise = false} The JIT approach instead is 100% reliable, as you check the real value to be returned to BPF LSM. But of course, the performance will be worse this way. If you are able to determine at verification time that an eBPF program is not going to return illegal values, that would be better. I'm not sure it is feasible. Thanks Roberto
On 11/16/2022 7:47 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > eBPF allows certain types of eBPF programs to modify the return value of > the functions they attach to. This is used for example by BPF LSM to let > security modules make their decision on LSM hooks. > > The JITed code looks like the following: > > ret = bpf_lsm_inode_permission_impl1(); // from a security module > if (ret) > goto out; > > .. > > ret = bpf_lsm_inode_permission_implN(); // from a security module > if (ret) > goto out; > > ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT > out: > > If ret is not zero, the attachment points of BPF LSM are not executed. For > this reason, the return value check cannot be done there. > > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check > function. > > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier > resolves the address of the check function (whose name is > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the > out label. If the return value is illegal, the check function changes it > back to the default value defined by the LSM infrastructure: > > .. > > out: > ret = bpf_lsm_inode_permission_ret(ret); As I've mentioned elsewhere, the return value is a small part of the problem you have with eBPF programs and the BPF LSM. Because the LSM infrastructure is inconsistent with regard to return codes, values returned in pointers and use of secids there is no uniform mechanism that I can see to address the "legitimate return" problem. Lets look at one of the ickyest interfaces we have, security_getprocattr(). It returns the size of a string that it has allocated. It puts the pointer to the allocated buffer into a char **value that was passed to it. If bpf_getprocattr() returns a positive number and sets value to NULL Bad Things can happen. If the return value is greater than the size allocated ditto. If it returns an error but allocates a string you get a memory leak. security_secid_to_secctx() has to work in concert with security_release_secctx() to do memory lifecycle management. If secid_to_secctx() allocates memory release_secctx() has to free it, while if secid_to_secctx() doesn't allocate memory it better not. (SELinux allocates memory, Smack does not. It's a real distinction) Your return checker would need to understand a lot more about the behavior of your eBPF programs than what value they return. > > In this way, an eBPF program cannot cause illegal return values to be sent > to BPF LSM, and to the callers of the LSM infrastructure. > > This is just a PoC, to validate the idea and get an early feedback. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > arch/arm64/net/bpf_jit_comp.c | 7 ++++--- > arch/x86/net/bpf_jit_comp.c | 17 ++++++++++++++++- > include/linux/bpf.h | 4 +++- > kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++ > kernel/bpf/bpf_struct_ops.c | 2 +- > kernel/bpf/trampoline.c | 6 ++++-- > kernel/bpf/verifier.c | 28 ++++++++++++++++++++++++++-- > 7 files changed, 74 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c > index 62f805f427b7..5412230c6935 100644 > --- a/arch/arm64/net/bpf_jit_comp.c > +++ b/arch/arm64/net/bpf_jit_comp.c > @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs) > */ > static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, > struct bpf_tramp_links *tlinks, void *orig_call, > - int nargs, u32 flags) > + void *ret_check_call, int nargs, u32 flags) > { > int i; > int stack_size; > @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, > void *image_end, const struct btf_func_model *m, > u32 flags, struct bpf_tramp_links *tlinks, > - void *orig_call) > + void *orig_call, void *ret_check_call) > { > int i, ret; > int nargs = m->nr_args; > @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, > return -ENOTSUPP; > } > > - ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags); > + ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call, > + nargs, flags); > if (ret < 0) > return ret; > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index cec5195602bc..6cd727b4af0a 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog, > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, > const struct btf_func_model *m, u32 flags, > struct bpf_tramp_links *tlinks, > - void *func_addr) > + void *func_addr, void *func_ret_check_addr) > { > int ret, i, nr_args = m->nr_args, extra_nregs = 0; > int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off; > @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > for (i = 0; i < fmod_ret->nr_links; i++) > emit_cond_near_jump(&branches[i], prog, branches[i], > X86_JNE); > + > + if (func_ret_check_addr) { > + emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8); > + > + /* call ret check function */ > + if (emit_call(&prog, func_ret_check_addr, prog)) { > + ret = -EINVAL; > + goto cleanup; > + } > + > + /* remember return value in a stack for bpf prog to access */ > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8); > + memcpy(prog, x86_nops[5], X86_PATCH_SIZE); > + prog += X86_PATCH_SIZE; > + } > } > > if (fexit->nr_links) > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 49f9d2bec401..f3551f7bdc28 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -919,7 +919,7 @@ struct bpf_tramp_image; > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, > const struct btf_func_model *m, u32 flags, > struct bpf_tramp_links *tlinks, > - void *orig_call); > + void *orig_call, void *ret_call); > u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog, > struct bpf_tramp_run_ctx *run_ctx); > void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start, > @@ -974,6 +974,7 @@ struct bpf_trampoline { > struct { > struct btf_func_model model; > void *addr; > + void *ret_check_addr; > bool ftrace_managed; > } func; > /* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF > @@ -994,6 +995,7 @@ struct bpf_trampoline { > struct bpf_attach_target_info { > struct btf_func_model fmodel; > long tgt_addr; > + long tgt_ret_check_addr; > const char *tgt_name; > const struct btf_type *tgt_type; > }; > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c > index 37bcedf5a44e..f7f25d0064dd 100644 > --- a/kernel/bpf/bpf_lsm.c > +++ b/kernel/bpf/bpf_lsm.c > @@ -18,6 +18,17 @@ > #include <linux/ima.h> > #include <linux/bpf-cgroup.h> > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > +{ > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > + return false; > + > + return true; > +} > + > /* For every LSM hook that allows attachment of BPF programs, declare a nop > * function where a BPF program can be attached. > */ > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > +{ \ > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > +} > + > +#include <linux/lsm_hook_defs.h> > +#undef LSM_HOOK > + > #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > BTF_ID(func, bpf_lsm_##NAME) > BTF_SET_START(bpf_lsm_hooks) > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 84b2d9dba79a..22485f0df534 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks, > */ > flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; > return arch_prepare_bpf_trampoline(NULL, image, image_end, > - model, flags, tlinks, NULL); > + model, flags, tlinks, NULL, NULL); > } > > static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index d6395215b849..3c6821b3c08c 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > > err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE, > &tr->func.model, tr->flags, tlinks, > - tr->func.addr); > + tr->func.addr, > + tr->func.ret_check_addr); > if (err < 0) > goto out; > > @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key, > > memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel)); > tr->func.addr = (void *)tgt_info->tgt_addr; > + tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr; > out: > mutex_unlock(&tr->mutex); > return tr; > @@ -1055,7 +1057,7 @@ int __weak > arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, > const struct btf_func_model *m, u32 flags, > struct bpf_tramp_links *tlinks, > - void *orig_call) > + void *orig_call, void *ret_check_call) > { > return -ENOTSUPP; > } > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 5e74f460dfd0..1ad0fe5cefe9 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > { > bool prog_extension = prog->type == BPF_PROG_TYPE_EXT; > const char prefix[] = "btf_trace_"; > - int ret = 0, subprog = -1, i; > + int ret = 0, subprog = -1, i, tname_len; > const struct btf_type *t; > bool conservative = true; > const char *tname; > + char *tname_ret; > struct btf *btf; > - long addr = 0; > + long addr = 0, ret_check_addr = 0; > > if (!btf_id) { > bpf_log(log, "Tracing programs must provide btf_id\n"); > @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > tname); > return -ENOENT; > } > + > + if (prog->expected_attach_type == BPF_LSM_MAC) { > + tname_len = strlen(tname); > + tname_ret = kmalloc(tname_len + 5, GFP_KERNEL); > + if (!tname_ret) { > + bpf_log(log, > + "Cannot allocate memory for %s_ret string\n", > + tname); > + return -ENOMEM; > + } > + > + snprintf(tname_ret, tname_len + 5, "%s_ret", tname); > + ret_check_addr = kallsyms_lookup_name(tname_ret); > + kfree(tname_ret); > + > + if (!ret_check_addr) { > + bpf_log(log, > + "Kernel symbol %s_ret not found\n", > + tname); > + return -ENOENT; > + } > + } > } > > if (prog->aux->sleepable) { > @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > break; > } > tgt_info->tgt_addr = addr; > + tgt_info->tgt_ret_check_addr = ret_check_addr; > tgt_info->tgt_name = tname; > tgt_info->tgt_type = t; > return 0;
Hi Roberto, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on ] url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057 base: the 4th patch of https://lore.kernel.org/r/20221115175652.3836811-5-roberto.sassu%40huaweicloud.com patch link: https://lore.kernel.org/r/20221116154712.4115929-1-roberto.sassu%40huaweicloud.com patch subject: [PoC][PATCH] bpf: Call return value check function in the JITed code config: mips-allyesconfig compiler: mips-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057 git checkout 8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash kernel/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_binder_set_context_mgr_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:30:1: note: in expansion of macro 'LSM_HOOK' 30 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_set_context_mgr, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_binder_transaction_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:32:1: note: in expansion of macro 'LSM_HOOK' 32 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transaction, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_binder_transfer_binder_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:34:1: note: in expansion of macro 'LSM_HOOK' 34 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transfer_binder, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_binder_transfer_file_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:36:1: note: in expansion of macro 'LSM_HOOK' 36 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transfer_file, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_ptrace_access_check_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:38:1: note: in expansion of macro 'LSM_HOOK' 38 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ptrace_access_check, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_ptrace_traceme_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:40:1: note: in expansion of macro 'LSM_HOOK' 40 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ptrace_traceme, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_capget_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:42:1: note: in expansion of macro 'LSM_HOOK' 42 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capget, struct task_struct *target, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_capset_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:45:1: note: in expansion of macro 'LSM_HOOK' 45 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capset, struct cred *new, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_capable_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:48:1: note: in expansion of macro 'LSM_HOOK' 48 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capable, const struct cred *cred, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_quotactl_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:50:1: note: in expansion of macro 'LSM_HOOK' 50 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, quotactl, int cmds, int type, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_quota_on_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:52:1: note: in expansion of macro 'LSM_HOOK' 52 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, quota_on, struct dentry *dentry) | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_syslog_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:53:1: note: in expansion of macro 'LSM_HOOK' 53 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, syslog, int type) | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_settime_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:54:1: note: in expansion of macro 'LSM_HOOK' 54 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, settime, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_vm_enough_memory_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:56:1: note: in expansion of macro 'LSM_HOOK' 56 | LSM_HOOK(int, 0, LSM_RET_ZERO | LSM_RET_ONE, vm_enough_memory, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_bprm_creds_for_exec_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:58:1: note: in expansion of macro 'LSM_HOOK' 58 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_creds_for_exec, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_bprm_creds_from_file_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:60:1: note: in expansion of macro 'LSM_HOOK' 60 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_creds_from_file, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_bprm_check_security_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:62:1: note: in expansion of macro 'LSM_HOOK' 62 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_check_security, | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_bprm_committing_creds_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:64:1: note: in expansion of macro 'LSM_HOOK' 64 | LSM_HOOK(void, LSM_RET_VOID, 0, bprm_committing_creds, struct linux_binprm *bprm) | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_bprm_committed_creds_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:65:1: note: in expansion of macro 'LSM_HOOK' 65 | LSM_HOOK(void, LSM_RET_VOID, 0, bprm_committed_creds, struct linux_binprm *bprm) | ^~~~~~~~ >> kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_fs_context_dup_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:66:1: note: in expansion of macro 'LSM_HOOK' 66 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, fs_context_dup, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_fs_context_parse_param_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:68:1: note: in expansion of macro 'LSM_HOOK' 68 | LSM_HOOK(int, -ENOPARAM, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_alloc_security_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:71:1: note: in expansion of macro 'LSM_HOOK' 71 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_alloc_security, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_delete_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:73:1: note: in expansion of macro 'LSM_HOOK' 73 | LSM_HOOK(void, LSM_RET_VOID, 0, sb_delete, struct super_block *sb) | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_free_security_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:74:1: note: in expansion of macro 'LSM_HOOK' 74 | LSM_HOOK(void, LSM_RET_VOID, 0, sb_free_security, struct super_block *sb) | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_free_mnt_opts_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:75:1: note: in expansion of macro 'LSM_HOOK' 75 | LSM_HOOK(void, LSM_RET_VOID, 0, sb_free_mnt_opts, void *mnt_opts) | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_eat_lsm_opts_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:76:1: note: in expansion of macro 'LSM_HOOK' 76 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_eat_lsm_opts, char *orig, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_mnt_opts_compat_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:78:1: note: in expansion of macro 'LSM_HOOK' 78 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_mnt_opts_compat, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_remount_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:80:1: note: in expansion of macro 'LSM_HOOK' 80 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_remount, struct super_block *sb, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_kern_mount_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:82:1: note: in expansion of macro 'LSM_HOOK' 82 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_kern_mount, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_show_options_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:84:1: note: in expansion of macro 'LSM_HOOK' 84 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_show_options, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_statfs_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:86:1: note: in expansion of macro 'LSM_HOOK' 86 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_statfs, struct dentry *dentry) | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_mount_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:87:1: note: in expansion of macro 'LSM_HOOK' 87 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_mount, const char *dev_name, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_umount_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:90:1: note: in expansion of macro 'LSM_HOOK' 90 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_umount, struct vfsmount *mnt, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_pivotroot_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:92:1: note: in expansion of macro 'LSM_HOOK' 92 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_pivotroot, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_set_mnt_opts_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:94:1: note: in expansion of macro 'LSM_HOOK' 94 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_set_mnt_opts, | ^~~~~~~~ kernel/bpf/bpf_lsm.c:45:14: warning: no previous prototype for 'bpf_lsm_sb_clone_mnt_opts_ret' [-Wmissing-prototypes] 45 | noinline RET bpf_lsm_##NAME##_ret(int ret) \ | ^~~~~~~~ include/linux/lsm_hook_defs.h:97:1: note: in expansion of macro 'LSM_HOOK' 97 | LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_clone_mnt_opts, vim +/bpf_lsm_binder_set_context_mgr_ret +45 kernel/bpf/bpf_lsm.c 43 44 #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > 45 noinline RET bpf_lsm_##NAME##_ret(int ret) \ 46 { \ 47 return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ 48 } 49
On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > > +{ > > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > > * function where a BPF program can be attached. > > > */ > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > > #include <linux/lsm_hook_defs.h> > > > #undef LSM_HOOK > > > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > > +{ \ > > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > > +} > > > + > > > +#include <linux/lsm_hook_defs.h> > > > +#undef LSM_HOOK > > > + > > > > because lsm hooks is mess of undocumented return values your > > "solution" is to add hundreds of noninline functions > > and hack the call into them in JITs ?! > > I revisited the documentation and checked each LSM hook one by one. > Hopefully, I completed it correctly, but I would review again (others > are also welcome to do it). > > Not sure if there is a more efficient way. Do you have any idea? > Maybe we find a way to use only one check function (by reusing the > address of the attachment point?). > > Regarding the JIT approach, I didn't find a reliable solution for using > just the verifier. As I wrote to you, there could be the case where the > range can include positive values, despite the possible return values > are zero and -EACCES. Didn't you find that there are only 12 or so odd return cases. Maybe refactor some of them to something that the verifier can enforce and denylist the rest ? Also denylist those that Casey mentioned like security_secid_to_secctx ?
On 11/16/2022 9:55 AM, Alexei Starovoitov wrote: > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: >> On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: >>> On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu >>> <roberto.sassu@huaweicloud.com> wrote: >>>> +static bool is_ret_value_allowed(int ret, u32 ret_flags) >>>> +{ >>>> + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || >>>> + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || >>>> + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || >>>> + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) >>>> + return false; >>>> + >>>> + return true; >>>> +} >>>> + >>>> /* For every LSM hook that allows attachment of BPF programs, declare a nop >>>> * function where a BPF program can be attached. >>>> */ >>>> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ >>>> #include <linux/lsm_hook_defs.h> >>>> #undef LSM_HOOK >>>> >>>> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ >>>> +noinline RET bpf_lsm_##NAME##_ret(int ret) \ >>>> +{ \ >>>> + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ >>>> +} >>>> + >>>> +#include <linux/lsm_hook_defs.h> >>>> +#undef LSM_HOOK >>>> + >>> because lsm hooks is mess of undocumented return values your >>> "solution" is to add hundreds of noninline functions >>> and hack the call into them in JITs ?! >> I revisited the documentation and checked each LSM hook one by one. >> Hopefully, I completed it correctly, but I would review again (others >> are also welcome to do it). >> >> Not sure if there is a more efficient way. Do you have any idea? >> Maybe we find a way to use only one check function (by reusing the >> address of the attachment point?). >> >> Regarding the JIT approach, I didn't find a reliable solution for using >> just the verifier. As I wrote to you, there could be the case where the >> range can include positive values, despite the possible return values >> are zero and -EACCES. > Didn't you find that there are only 12 or so odd return cases. > Maybe refactor some of them to something that the verifier can enforce > and denylist the rest ? Changing security_mumble() often requires changes in either VFS, audit or networking code. Even simple changes can require extensive review and difficult to obtain Acked-by's. It may be the correct approach, but it won't be easy or quick. > Also denylist those that Casey mentioned like security_secid_to_secctx ? Identifying all the hooks that could be "dangerous" isn't an easy chore, and some of the "dangerous" hooks are key to implementing classes of policy and absolutely necessary for audit support.
On Wed, Nov 16, 2022 at 6:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 11/16/2022 7:47 AM, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > eBPF allows certain types of eBPF programs to modify the return value of > > the functions they attach to. This is used for example by BPF LSM to let > > security modules make their decision on LSM hooks. > > > > The JITed code looks like the following: > > > > ret = bpf_lsm_inode_permission_impl1(); // from a security module > > if (ret) > > goto out; > > > > .. > > > > ret = bpf_lsm_inode_permission_implN(); // from a security module > > if (ret) > > goto out; > > > > ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT > > out: > > > > If ret is not zero, the attachment points of BPF LSM are not executed. For > > this reason, the return value check cannot be done there. > > > > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check > > function. > > > > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier > > resolves the address of the check function (whose name is > > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the > > out label. If the return value is illegal, the check function changes it > > back to the default value defined by the LSM infrastructure: > > > > .. > > > > out: > > ret = bpf_lsm_inode_permission_ret(ret); > > As I've mentioned elsewhere, the return value is a small part of > the problem you have with eBPF programs and the BPF LSM. Because > the LSM infrastructure is inconsistent with regard to return codes, > values returned in pointers and use of secids there is no uniform > mechanism that I can see to address the "legitimate return" problem. > > Lets look at one of the ickyest interfaces we have, security_getprocattr(). > It returns the size of a string that it has allocated. It puts the > pointer to the allocated buffer into a char **value that was passed to it. > If bpf_getprocattr() returns a positive number and sets value to NULL Bad > Things can happen. If the return value is greater than the size allocated > ditto. If it returns an error but allocates a string you get a memory leak. I think we should not need this hook in BPF. We can create a list of hooks that we should not really expose via BPF. > > security_secid_to_secctx() has to work in concert with security_release_secctx() > to do memory lifecycle management. If secid_to_secctx() allocates memory > release_secctx() has to free it, while if secid_to_secctx() doesn't allocate > memory it better not. (SELinux allocates memory, Smack does not. It's a real > distinction) Your return checker would need to understand a lot more about > the behavior of your eBPF programs than what value they return. While this is possible to do using the BPF verifier (i.e. more detailed checks). I don't see the value of BPF programs attaching to these hooks and we should just not register the BPF LSM callbacks for these. > > > > > In this way, an eBPF program cannot cause illegal return values to be sent > > to BPF LSM, and to the callers of the LSM infrastructure. > > > > This is just a PoC, to validate the idea and get an early feedback. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > arch/arm64/net/bpf_jit_comp.c | 7 ++++--- > > arch/x86/net/bpf_jit_comp.c | 17 ++++++++++++++++- > > include/linux/bpf.h | 4 +++- > > kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++ > > kernel/bpf/bpf_struct_ops.c | 2 +- > > kernel/bpf/trampoline.c | 6 ++++-- > > kernel/bpf/verifier.c | 28 ++++++++++++++++++++++++++-- > > 7 files changed, 74 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c > > index 62f805f427b7..5412230c6935 100644 > > --- a/arch/arm64/net/bpf_jit_comp.c > > +++ b/arch/arm64/net/bpf_jit_comp.c > > @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs) > > */ > > static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, > > struct bpf_tramp_links *tlinks, void *orig_call, > > - int nargs, u32 flags) > > + void *ret_check_call, int nargs, u32 flags) > > { > > int i; > > int stack_size; > > @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, > > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, > > void *image_end, const struct btf_func_model *m, > > u32 flags, struct bpf_tramp_links *tlinks, > > - void *orig_call) > > + void *orig_call, void *ret_check_call) > > { > > int i, ret; > > int nargs = m->nr_args; > > @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, > > return -ENOTSUPP; > > } > > > > - ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags); > > + ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call, > > + nargs, flags); > > if (ret < 0) > > return ret; > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > index cec5195602bc..6cd727b4af0a 100644 > > --- a/arch/x86/net/bpf_jit_comp.c > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog, > > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, > > const struct btf_func_model *m, u32 flags, > > struct bpf_tramp_links *tlinks, > > - void *func_addr) > > + void *func_addr, void *func_ret_check_addr) > > { > > int ret, i, nr_args = m->nr_args, extra_nregs = 0; > > int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off; > > @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > for (i = 0; i < fmod_ret->nr_links; i++) > > emit_cond_near_jump(&branches[i], prog, branches[i], > > X86_JNE); > > + > > + if (func_ret_check_addr) { > > + emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8); > > + > > + /* call ret check function */ > > + if (emit_call(&prog, func_ret_check_addr, prog)) { > > + ret = -EINVAL; > > + goto cleanup; > > + } > > + > > + /* remember return value in a stack for bpf prog to access */ > > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8); > > + memcpy(prog, x86_nops[5], X86_PATCH_SIZE); > > + prog += X86_PATCH_SIZE; > > + } > > } > > > > if (fexit->nr_links) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 49f9d2bec401..f3551f7bdc28 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -919,7 +919,7 @@ struct bpf_tramp_image; > > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, > > const struct btf_func_model *m, u32 flags, > > struct bpf_tramp_links *tlinks, > > - void *orig_call); > > + void *orig_call, void *ret_call); > > u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog, > > struct bpf_tramp_run_ctx *run_ctx); > > void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start, > > @@ -974,6 +974,7 @@ struct bpf_trampoline { > > struct { > > struct btf_func_model model; > > void *addr; > > + void *ret_check_addr; > > bool ftrace_managed; > > } func; > > /* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF > > @@ -994,6 +995,7 @@ struct bpf_trampoline { > > struct bpf_attach_target_info { > > struct btf_func_model fmodel; > > long tgt_addr; > > + long tgt_ret_check_addr; > > const char *tgt_name; > > const struct btf_type *tgt_type; > > }; > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c > > index 37bcedf5a44e..f7f25d0064dd 100644 > > --- a/kernel/bpf/bpf_lsm.c > > +++ b/kernel/bpf/bpf_lsm.c > > @@ -18,6 +18,17 @@ > > #include <linux/ima.h> > > #include <linux/bpf-cgroup.h> > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > +{ > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > + return false; > > + > > + return true; > > +} > > + > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > * function where a BPF program can be attached. > > */ > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > #include <linux/lsm_hook_defs.h> > > #undef LSM_HOOK > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > +{ \ > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > +} > > + > > +#include <linux/lsm_hook_defs.h> > > +#undef LSM_HOOK > > + > > #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > BTF_ID(func, bpf_lsm_##NAME) > > BTF_SET_START(bpf_lsm_hooks) > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > > index 84b2d9dba79a..22485f0df534 100644 > > --- a/kernel/bpf/bpf_struct_ops.c > > +++ b/kernel/bpf/bpf_struct_ops.c > > @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks, > > */ > > flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; > > return arch_prepare_bpf_trampoline(NULL, image, image_end, > > - model, flags, tlinks, NULL); > > + model, flags, tlinks, NULL, NULL); > > } > > > > static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > index d6395215b849..3c6821b3c08c 100644 > > --- a/kernel/bpf/trampoline.c > > +++ b/kernel/bpf/trampoline.c > > @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > > > > err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE, > > &tr->func.model, tr->flags, tlinks, > > - tr->func.addr); > > + tr->func.addr, > > + tr->func.ret_check_addr); > > if (err < 0) > > goto out; > > > > @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key, > > > > memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel)); > > tr->func.addr = (void *)tgt_info->tgt_addr; > > + tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr; > > out: > > mutex_unlock(&tr->mutex); > > return tr; > > @@ -1055,7 +1057,7 @@ int __weak > > arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, > > const struct btf_func_model *m, u32 flags, > > struct bpf_tramp_links *tlinks, > > - void *orig_call) > > + void *orig_call, void *ret_check_call) > > { > > return -ENOTSUPP; > > } > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 5e74f460dfd0..1ad0fe5cefe9 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > { > > bool prog_extension = prog->type == BPF_PROG_TYPE_EXT; > > const char prefix[] = "btf_trace_"; > > - int ret = 0, subprog = -1, i; > > + int ret = 0, subprog = -1, i, tname_len; > > const struct btf_type *t; > > bool conservative = true; > > const char *tname; > > + char *tname_ret; > > struct btf *btf; > > - long addr = 0; > > + long addr = 0, ret_check_addr = 0; > > > > if (!btf_id) { > > bpf_log(log, "Tracing programs must provide btf_id\n"); > > @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > tname); > > return -ENOENT; > > } > > + > > + if (prog->expected_attach_type == BPF_LSM_MAC) { > > + tname_len = strlen(tname); > > + tname_ret = kmalloc(tname_len + 5, GFP_KERNEL); > > + if (!tname_ret) { > > + bpf_log(log, > > + "Cannot allocate memory for %s_ret string\n", > > + tname); > > + return -ENOMEM; > > + } > > + > > + snprintf(tname_ret, tname_len + 5, "%s_ret", tname); > > + ret_check_addr = kallsyms_lookup_name(tname_ret); > > + kfree(tname_ret); > > + > > + if (!ret_check_addr) { > > + bpf_log(log, > > + "Kernel symbol %s_ret not found\n", > > + tname); > > + return -ENOENT; > > + } > > + } > > } > > > > if (prog->aux->sleepable) { > > @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > break; > > } > > tgt_info->tgt_addr = addr; > > + tgt_info->tgt_ret_check_addr = ret_check_addr; > > tgt_info->tgt_name = tname; > > tgt_info->tgt_type = t; > > return 0;
On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: > > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu > > > <roberto.sassu@huaweicloud.com> wrote: > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > > > +{ > > > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > > > + return false; > > > > + > > > > + return true; > > > > +} > > > > + > > > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > > > * function where a BPF program can be attached. > > > > */ > > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > > > #include <linux/lsm_hook_defs.h> > > > > #undef LSM_HOOK > > > > > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > > > +{ \ > > > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > > > +} > > > > + > > > > +#include <linux/lsm_hook_defs.h> > > > > +#undef LSM_HOOK > > > > + > > > > > > because lsm hooks is mess of undocumented return values your > > > "solution" is to add hundreds of noninline functions > > > and hack the call into them in JITs ?! > > > > I revisited the documentation and checked each LSM hook one by one. > > Hopefully, I completed it correctly, but I would review again (others > > are also welcome to do it). > > > > Not sure if there is a more efficient way. Do you have any idea? > > Maybe we find a way to use only one check function (by reusing the > > address of the attachment point?). > > > > Regarding the JIT approach, I didn't find a reliable solution for using > > just the verifier. As I wrote to you, there could be the case where the > > range can include positive values, despite the possible return values > > are zero and -EACCES. > > Didn't you find that there are only 12 or so odd return cases. > Maybe refactor some of them to something that the verifier can enforce > and denylist the rest ? +1 > > Also denylist those that Casey mentioned like security_secid_to_secctx ? Just replied to Casey's comment and I agree, these hooks should be denylisted.
On Wed, Nov 16, 2022 at 2:04 PM KP Singh <kpsingh@kernel.org> wrote: > On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu > > <roberto.sassu@huaweicloud.com> wrote: > > > > > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: > > > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu > > > > <roberto.sassu@huaweicloud.com> wrote: > > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > > > > +{ > > > > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > > > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > > > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > > > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > > > > + return false; > > > > > + > > > > > + return true; > > > > > +} > > > > > + > > > > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > > > > * function where a BPF program can be attached. > > > > > */ > > > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > > > > #include <linux/lsm_hook_defs.h> > > > > > #undef LSM_HOOK > > > > > > > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > > > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > > > > +{ \ > > > > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > > > > +} > > > > > + > > > > > +#include <linux/lsm_hook_defs.h> > > > > > +#undef LSM_HOOK > > > > > + > > > > > > > > because lsm hooks is mess of undocumented return values your > > > > "solution" is to add hundreds of noninline functions > > > > and hack the call into them in JITs ?! > > > > > > I revisited the documentation and checked each LSM hook one by one. > > > Hopefully, I completed it correctly, but I would review again (others > > > are also welcome to do it). > > > > > > Not sure if there is a more efficient way. Do you have any idea? > > > Maybe we find a way to use only one check function (by reusing the > > > address of the attachment point?). > > > > > > Regarding the JIT approach, I didn't find a reliable solution for using > > > just the verifier. As I wrote to you, there could be the case where the > > > range can include positive values, despite the possible return values > > > are zero and -EACCES. > > > > Didn't you find that there are only 12 or so odd return cases. > > Maybe refactor some of them to something that the verifier can enforce > > and denylist the rest ? > > +1 I'm not sure we want to refactor the LSM hooks right now, we've got too much stuff in-progress which I consider higher value/priority. While I'm generally in favor of improving the sanity of interfaces, I'd much rather we resolve the IMA/EVM special cases and land the stacking changes before we start playing with refactoring the hooks. I know this is a bummer for the BPF folks, but the IMA/EVM and stacking patches affect everybody.
On 11/16/2022 6:12 PM, Casey Schaufler wrote: > On 11/16/2022 7:47 AM, Roberto Sassu wrote: >> From: Roberto Sassu <roberto.sassu@huawei.com> >> >> eBPF allows certain types of eBPF programs to modify the return value of >> the functions they attach to. This is used for example by BPF LSM to let >> security modules make their decision on LSM hooks. >> >> The JITed code looks like the following: >> >> ret = bpf_lsm_inode_permission_impl1(); // from a security module >> if (ret) >> goto out; >> >> .. >> >> ret = bpf_lsm_inode_permission_implN(); // from a security module >> if (ret) >> goto out; >> >> ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT >> out: >> >> If ret is not zero, the attachment points of BPF LSM are not executed. For >> this reason, the return value check cannot be done there. >> >> Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check >> function. >> >> Whenever an eBPF program attaches to an LSM hook, the eBPF verifier >> resolves the address of the check function (whose name is >> bpf_lsm_<hook name>_ret()) and adds a call to that function just after the >> out label. If the return value is illegal, the check function changes it >> back to the default value defined by the LSM infrastructure: >> >> .. >> >> out: >> ret = bpf_lsm_inode_permission_ret(ret); > > As I've mentioned elsewhere, the return value is a small part of > the problem you have with eBPF programs and the BPF LSM. Because > the LSM infrastructure is inconsistent with regard to return codes, > values returned in pointers and use of secids there is no uniform > mechanism that I can see to address the "legitimate return" problem. > > Lets look at one of the ickyest interfaces we have, security_getprocattr(). > It returns the size of a string that it has allocated. It puts the > pointer to the allocated buffer into a char **value that was passed to it. > If bpf_getprocattr() returns a positive number and sets value to NULL Bad > Things can happen. If the return value is greater than the size allocated > ditto. If it returns an error but allocates a string you get a memory leak. I hope I understood how it works correctly, but you cannot modify directly data accessible from a pointer provided as parameter by the LSM hook you attach to. The pointer is treated as scalar value and the eBPF verifier detects any attempt to dereference as an illegal access. The only way to modify such data is through helpers that need to be properly declared to be usable by eBPF programs. Also, if I'm not mistaken we have the limitation of five parameters per functions. Not sure what happens for hooks that have more than this. > security_secid_to_secctx() has to work in concert with security_release_secctx() > to do memory lifecycle management. If secid_to_secctx() allocates memory > release_secctx() has to free it, while if secid_to_secctx() doesn't allocate > memory it better not. (SELinux allocates memory, Smack does not. It's a real > distinction) Your return checker would need to understand a lot more about > the behavior of your eBPF programs than what value they return. I see. Within an eBPF program we are able to pair allocation and free together. I guess something similar could be done for pairs of LSM hooks. Roberto >> In this way, an eBPF program cannot cause illegal return values to be sent >> to BPF LSM, and to the callers of the LSM infrastructure. >> >> This is just a PoC, to validate the idea and get an early feedback. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> --- >> arch/arm64/net/bpf_jit_comp.c | 7 ++++--- >> arch/x86/net/bpf_jit_comp.c | 17 ++++++++++++++++- >> include/linux/bpf.h | 4 +++- >> kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++ >> kernel/bpf/bpf_struct_ops.c | 2 +- >> kernel/bpf/trampoline.c | 6 ++++-- >> kernel/bpf/verifier.c | 28 ++++++++++++++++++++++++++-- >> 7 files changed, 74 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c >> index 62f805f427b7..5412230c6935 100644 >> --- a/arch/arm64/net/bpf_jit_comp.c >> +++ b/arch/arm64/net/bpf_jit_comp.c >> @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs) >> */ >> static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, >> struct bpf_tramp_links *tlinks, void *orig_call, >> - int nargs, u32 flags) >> + void *ret_check_call, int nargs, u32 flags) >> { >> int i; >> int stack_size; >> @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, >> int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, >> void *image_end, const struct btf_func_model *m, >> u32 flags, struct bpf_tramp_links *tlinks, >> - void *orig_call) >> + void *orig_call, void *ret_check_call) >> { >> int i, ret; >> int nargs = m->nr_args; >> @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, >> return -ENOTSUPP; >> } >> >> - ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags); >> + ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call, >> + nargs, flags); >> if (ret < 0) >> return ret; >> >> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c >> index cec5195602bc..6cd727b4af0a 100644 >> --- a/arch/x86/net/bpf_jit_comp.c >> +++ b/arch/x86/net/bpf_jit_comp.c >> @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog, >> int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, >> const struct btf_func_model *m, u32 flags, >> struct bpf_tramp_links *tlinks, >> - void *func_addr) >> + void *func_addr, void *func_ret_check_addr) >> { >> int ret, i, nr_args = m->nr_args, extra_nregs = 0; >> int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off; >> @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i >> for (i = 0; i < fmod_ret->nr_links; i++) >> emit_cond_near_jump(&branches[i], prog, branches[i], >> X86_JNE); >> + >> + if (func_ret_check_addr) { >> + emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8); >> + >> + /* call ret check function */ >> + if (emit_call(&prog, func_ret_check_addr, prog)) { >> + ret = -EINVAL; >> + goto cleanup; >> + } >> + >> + /* remember return value in a stack for bpf prog to access */ >> + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8); >> + memcpy(prog, x86_nops[5], X86_PATCH_SIZE); >> + prog += X86_PATCH_SIZE; >> + } >> } >> >> if (fexit->nr_links) >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index 49f9d2bec401..f3551f7bdc28 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -919,7 +919,7 @@ struct bpf_tramp_image; >> int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, >> const struct btf_func_model *m, u32 flags, >> struct bpf_tramp_links *tlinks, >> - void *orig_call); >> + void *orig_call, void *ret_call); >> u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog, >> struct bpf_tramp_run_ctx *run_ctx); >> void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start, >> @@ -974,6 +974,7 @@ struct bpf_trampoline { >> struct { >> struct btf_func_model model; >> void *addr; >> + void *ret_check_addr; >> bool ftrace_managed; >> } func; >> /* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF >> @@ -994,6 +995,7 @@ struct bpf_trampoline { >> struct bpf_attach_target_info { >> struct btf_func_model fmodel; >> long tgt_addr; >> + long tgt_ret_check_addr; >> const char *tgt_name; >> const struct btf_type *tgt_type; >> }; >> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c >> index 37bcedf5a44e..f7f25d0064dd 100644 >> --- a/kernel/bpf/bpf_lsm.c >> +++ b/kernel/bpf/bpf_lsm.c >> @@ -18,6 +18,17 @@ >> #include <linux/ima.h> >> #include <linux/bpf-cgroup.h> >> >> +static bool is_ret_value_allowed(int ret, u32 ret_flags) >> +{ >> + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || >> + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || >> + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || >> + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) >> + return false; >> + >> + return true; >> +} >> + >> /* For every LSM hook that allows attachment of BPF programs, declare a nop >> * function where a BPF program can be attached. >> */ >> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ >> #include <linux/lsm_hook_defs.h> >> #undef LSM_HOOK >> >> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ >> +noinline RET bpf_lsm_##NAME##_ret(int ret) \ >> +{ \ >> + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ >> +} >> + >> +#include <linux/lsm_hook_defs.h> >> +#undef LSM_HOOK >> + >> #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ >> BTF_ID(func, bpf_lsm_##NAME) >> BTF_SET_START(bpf_lsm_hooks) >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >> index 84b2d9dba79a..22485f0df534 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks, >> */ >> flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; >> return arch_prepare_bpf_trampoline(NULL, image, image_end, >> - model, flags, tlinks, NULL); >> + model, flags, tlinks, NULL, NULL); >> } >> >> static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c >> index d6395215b849..3c6821b3c08c 100644 >> --- a/kernel/bpf/trampoline.c >> +++ b/kernel/bpf/trampoline.c >> @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut >> >> err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE, >> &tr->func.model, tr->flags, tlinks, >> - tr->func.addr); >> + tr->func.addr, >> + tr->func.ret_check_addr); >> if (err < 0) >> goto out; >> >> @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key, >> >> memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel)); >> tr->func.addr = (void *)tgt_info->tgt_addr; >> + tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr; >> out: >> mutex_unlock(&tr->mutex); >> return tr; >> @@ -1055,7 +1057,7 @@ int __weak >> arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, >> const struct btf_func_model *m, u32 flags, >> struct bpf_tramp_links *tlinks, >> - void *orig_call) >> + void *orig_call, void *ret_check_call) >> { >> return -ENOTSUPP; >> } >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 5e74f460dfd0..1ad0fe5cefe9 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, >> { >> bool prog_extension = prog->type == BPF_PROG_TYPE_EXT; >> const char prefix[] = "btf_trace_"; >> - int ret = 0, subprog = -1, i; >> + int ret = 0, subprog = -1, i, tname_len; >> const struct btf_type *t; >> bool conservative = true; >> const char *tname; >> + char *tname_ret; >> struct btf *btf; >> - long addr = 0; >> + long addr = 0, ret_check_addr = 0; >> >> if (!btf_id) { >> bpf_log(log, "Tracing programs must provide btf_id\n"); >> @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, >> tname); >> return -ENOENT; >> } >> + >> + if (prog->expected_attach_type == BPF_LSM_MAC) { >> + tname_len = strlen(tname); >> + tname_ret = kmalloc(tname_len + 5, GFP_KERNEL); >> + if (!tname_ret) { >> + bpf_log(log, >> + "Cannot allocate memory for %s_ret string\n", >> + tname); >> + return -ENOMEM; >> + } >> + >> + snprintf(tname_ret, tname_len + 5, "%s_ret", tname); >> + ret_check_addr = kallsyms_lookup_name(tname_ret); >> + kfree(tname_ret); >> + >> + if (!ret_check_addr) { >> + bpf_log(log, >> + "Kernel symbol %s_ret not found\n", >> + tname); >> + return -ENOENT; >> + } >> + } >> } >> >> if (prog->aux->sleepable) { >> @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, >> break; >> } >> tgt_info->tgt_addr = addr; >> + tgt_info->tgt_ret_check_addr = ret_check_addr; >> tgt_info->tgt_name = tname; >> tgt_info->tgt_type = t; >> return 0;
On Fri, 2022-11-18 at 09:44 +0100, Roberto Sassu wrote: > On 11/16/2022 6:12 PM, Casey Schaufler wrote: > > On 11/16/2022 7:47 AM, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > eBPF allows certain types of eBPF programs to modify the return value of > > > the functions they attach to. This is used for example by BPF LSM to let > > > security modules make their decision on LSM hooks. > > > > > > The JITed code looks like the following: > > > > > > ret = bpf_lsm_inode_permission_impl1(); // from a security module > > > if (ret) > > > goto out; > > > > > > .. > > > > > > ret = bpf_lsm_inode_permission_implN(); // from a security module > > > if (ret) > > > goto out; > > > > > > ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT > > > out: > > > > > > If ret is not zero, the attachment points of BPF LSM are not executed. For > > > this reason, the return value check cannot be done there. > > > > > > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check > > > function. > > > > > > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier > > > resolves the address of the check function (whose name is > > > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the > > > out label. If the return value is illegal, the check function changes it > > > back to the default value defined by the LSM infrastructure: > > > > > > .. > > > > > > out: > > > ret = bpf_lsm_inode_permission_ret(ret); > > > > As I've mentioned elsewhere, the return value is a small part of > > the problem you have with eBPF programs and the BPF LSM. Because > > the LSM infrastructure is inconsistent with regard to return codes, > > values returned in pointers and use of secids there is no uniform > > mechanism that I can see to address the "legitimate return" problem. > > > > Lets look at one of the ickyest interfaces we have, security_getprocattr(). > > It returns the size of a string that it has allocated. It puts the > > pointer to the allocated buffer into a char **value that was passed to it. > > If bpf_getprocattr() returns a positive number and sets value to NULL Bad > > Things can happen. If the return value is greater than the size allocated > > ditto. If it returns an error but allocates a string you get a memory leak. > > I hope I understood how it works correctly, but you cannot modify > directly data accessible from a pointer provided as parameter by the LSM > hook you attach to. The pointer is treated as scalar value and the eBPF > verifier detects any attempt to dereference as an illegal access. The > only way to modify such data is through helpers that need to be properly > declared to be usable by eBPF programs. I wanted to double check about accessing the LSM hook arguments from an eBPF program. I checked what it could prevent to access them. First, in kernel/bpf/btf.c: if (!btf_type_is_struct(t)) { bpf_log(log, "func '%s' arg%d type %s is not a struct\n", If the argument is not a struct, it is not accessible. Second, if a btf_struct_access method has not been defined for a structure, only read can be done (kernel/bpf/verifier.c): if (env->ops->btf_struct_access) { ret = env->ops->btf_struct_access(...); } else { if (atype != BPF_READ) { verbose(env, "only read is supported\n"); return -EACCES; } I found four: net/bpf/bpf_dummy_struct_ops.c: .btf_struct_access = bpf_dummy_ops_btf_struct_access, net/core/filter.c: .btf_struct_access = tc_cls_act_btf_struct_access, net/core/filter.c: .btf_struct_access = xdp_btf_struct_access, net/ipv4/bpf_tcp_ca.c: .btf_struct_access = bpf_tcp_ca_btf_struct_access, Anything else? Thanks Roberto
Hi Roberto,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on ]
url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057
base: the 4th patch of https://lore.kernel.org/r/20221115175652.3836811-5-roberto.sassu%40huaweicloud.com
patch link: https://lore.kernel.org/r/20221116154712.4115929-1-roberto.sassu%40huaweicloud.com
patch subject: [PoC][PATCH] bpf: Call return value check function in the JITed code
config: arm64-randconfig-r021-20221121
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057
git checkout 8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/net/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
arch/arm64/net/bpf_jit_comp.c: In function 'arch_prepare_bpf_trampoline':
>> arch/arm64/net/bpf_jit_comp.c:1998:63: warning: passing argument 5 of 'prepare_trampoline' makes pointer from integer without a cast [-Wint-conversion]
1998 | ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
| ^~~~~
| |
| int
arch/arm64/net/bpf_jit_comp.c:1767:37: note: expected 'void *' but argument is of type 'int'
1767 | void *ret_check_call, int nargs, u32 flags)
| ~~~~~~^~~~~~~~~~~~~~
arch/arm64/net/bpf_jit_comp.c:1998:15: error: too few arguments to function 'prepare_trampoline'
1998 | ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
| ^~~~~~~~~~~~~~~~~~
arch/arm64/net/bpf_jit_comp.c:1765:12: note: declared here
1765 | static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
| ^~~~~~~~~~~~~~~~~~
vim +/prepare_trampoline +1998 arch/arm64/net/bpf_jit_comp.c
efc9909fdce00a8 Xu Kuohai 2022-07-11 1962
efc9909fdce00a8 Xu Kuohai 2022-07-11 1963 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1964 void *image_end, const struct btf_func_model *m,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1965 u32 flags, struct bpf_tramp_links *tlinks,
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1966 void *orig_call, void *ret_check_call)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1967 {
eb707dde264af5e Yonghong Song 2022-08-31 1968 int i, ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1969 int nargs = m->nr_args;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1970 int max_insns = ((long)image_end - (long)image) / AARCH64_INSN_SIZE;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1971 struct jit_ctx ctx = {
efc9909fdce00a8 Xu Kuohai 2022-07-11 1972 .image = NULL,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1973 .idx = 0,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1974 };
efc9909fdce00a8 Xu Kuohai 2022-07-11 1975
efc9909fdce00a8 Xu Kuohai 2022-07-11 1976 /* the first 8 arguments are passed by registers */
efc9909fdce00a8 Xu Kuohai 2022-07-11 1977 if (nargs > 8)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1978 return -ENOTSUPP;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1979
eb707dde264af5e Yonghong Song 2022-08-31 1980 /* don't support struct argument */
eb707dde264af5e Yonghong Song 2022-08-31 1981 for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
eb707dde264af5e Yonghong Song 2022-08-31 1982 if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
eb707dde264af5e Yonghong Song 2022-08-31 1983 return -ENOTSUPP;
eb707dde264af5e Yonghong Song 2022-08-31 1984 }
eb707dde264af5e Yonghong Song 2022-08-31 1985
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1986 ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1987 nargs, flags);
efc9909fdce00a8 Xu Kuohai 2022-07-11 1988 if (ret < 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1989 return ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1990
efc9909fdce00a8 Xu Kuohai 2022-07-11 1991 if (ret > max_insns)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1992 return -EFBIG;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1993
efc9909fdce00a8 Xu Kuohai 2022-07-11 1994 ctx.image = image;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1995 ctx.idx = 0;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1996
efc9909fdce00a8 Xu Kuohai 2022-07-11 1997 jit_fill_hole(image, (unsigned int)(image_end - image));
efc9909fdce00a8 Xu Kuohai 2022-07-11 @1998 ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
efc9909fdce00a8 Xu Kuohai 2022-07-11 1999
efc9909fdce00a8 Xu Kuohai 2022-07-11 2000 if (ret > 0 && validate_code(&ctx) < 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 2001 ret = -EINVAL;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2002
efc9909fdce00a8 Xu Kuohai 2022-07-11 2003 if (ret > 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 2004 ret *= AARCH64_INSN_SIZE;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2005
efc9909fdce00a8 Xu Kuohai 2022-07-11 2006 return ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2007 }
efc9909fdce00a8 Xu Kuohai 2022-07-11 2008
Hi Roberto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on ]
url: https://github.com/intel-lab-lkp/linux/commits/UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057
base: the 4th patch of https://lore.kernel.org/r/20221115175652.3836811-5-roberto.sassu%40huaweicloud.com
patch link: https://lore.kernel.org/r/20221116154712.4115929-1-roberto.sassu%40huaweicloud.com
patch subject: [PoC][PATCH] bpf: Call return value check function in the JITed code
config: arm64-randconfig-r021-20221121
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review UPDATE-20221116-234919/Roberto-Sassu/security-Ensure-LSMs-return-expected-values/20221116-020057
git checkout 8d9a4cbc633baa0bb3f9945b999f4cd0ab5cc6bb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/arm64/net/bpf_jit_comp.c: In function 'arch_prepare_bpf_trampoline':
arch/arm64/net/bpf_jit_comp.c:1998:63: warning: passing argument 5 of 'prepare_trampoline' makes pointer from integer without a cast [-Wint-conversion]
1998 | ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
| ^~~~~
| |
| int
arch/arm64/net/bpf_jit_comp.c:1767:37: note: expected 'void *' but argument is of type 'int'
1767 | void *ret_check_call, int nargs, u32 flags)
| ~~~~~~^~~~~~~~~~~~~~
>> arch/arm64/net/bpf_jit_comp.c:1998:15: error: too few arguments to function 'prepare_trampoline'
1998 | ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
| ^~~~~~~~~~~~~~~~~~
arch/arm64/net/bpf_jit_comp.c:1765:12: note: declared here
1765 | static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
| ^~~~~~~~~~~~~~~~~~
vim +/prepare_trampoline +1998 arch/arm64/net/bpf_jit_comp.c
efc9909fdce00a8 Xu Kuohai 2022-07-11 1962
efc9909fdce00a8 Xu Kuohai 2022-07-11 1963 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1964 void *image_end, const struct btf_func_model *m,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1965 u32 flags, struct bpf_tramp_links *tlinks,
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1966 void *orig_call, void *ret_check_call)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1967 {
eb707dde264af5e Yonghong Song 2022-08-31 1968 int i, ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1969 int nargs = m->nr_args;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1970 int max_insns = ((long)image_end - (long)image) / AARCH64_INSN_SIZE;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1971 struct jit_ctx ctx = {
efc9909fdce00a8 Xu Kuohai 2022-07-11 1972 .image = NULL,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1973 .idx = 0,
efc9909fdce00a8 Xu Kuohai 2022-07-11 1974 };
efc9909fdce00a8 Xu Kuohai 2022-07-11 1975
efc9909fdce00a8 Xu Kuohai 2022-07-11 1976 /* the first 8 arguments are passed by registers */
efc9909fdce00a8 Xu Kuohai 2022-07-11 1977 if (nargs > 8)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1978 return -ENOTSUPP;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1979
eb707dde264af5e Yonghong Song 2022-08-31 1980 /* don't support struct argument */
eb707dde264af5e Yonghong Song 2022-08-31 1981 for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
eb707dde264af5e Yonghong Song 2022-08-31 1982 if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
eb707dde264af5e Yonghong Song 2022-08-31 1983 return -ENOTSUPP;
eb707dde264af5e Yonghong Song 2022-08-31 1984 }
eb707dde264af5e Yonghong Song 2022-08-31 1985
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1986 ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
8d9a4cbc633baa0 Roberto Sassu 2022-11-16 1987 nargs, flags);
efc9909fdce00a8 Xu Kuohai 2022-07-11 1988 if (ret < 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1989 return ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1990
efc9909fdce00a8 Xu Kuohai 2022-07-11 1991 if (ret > max_insns)
efc9909fdce00a8 Xu Kuohai 2022-07-11 1992 return -EFBIG;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1993
efc9909fdce00a8 Xu Kuohai 2022-07-11 1994 ctx.image = image;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1995 ctx.idx = 0;
efc9909fdce00a8 Xu Kuohai 2022-07-11 1996
efc9909fdce00a8 Xu Kuohai 2022-07-11 1997 jit_fill_hole(image, (unsigned int)(image_end - image));
efc9909fdce00a8 Xu Kuohai 2022-07-11 @1998 ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
efc9909fdce00a8 Xu Kuohai 2022-07-11 1999
efc9909fdce00a8 Xu Kuohai 2022-07-11 2000 if (ret > 0 && validate_code(&ctx) < 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 2001 ret = -EINVAL;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2002
efc9909fdce00a8 Xu Kuohai 2022-07-11 2003 if (ret > 0)
efc9909fdce00a8 Xu Kuohai 2022-07-11 2004 ret *= AARCH64_INSN_SIZE;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2005
efc9909fdce00a8 Xu Kuohai 2022-07-11 2006 return ret;
efc9909fdce00a8 Xu Kuohai 2022-07-11 2007 }
efc9909fdce00a8 Xu Kuohai 2022-07-11 2008
On Wed, 2022-11-16 at 09:55 -0800, Alexei Starovoitov wrote: > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote: > > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu > > > <roberto.sassu@huaweicloud.com> wrote: > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags) > > > > +{ > > > > + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || > > > > + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || > > > > + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || > > > > + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) > > > > + return false; > > > > + > > > > + return true; > > > > +} > > > > + > > > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > > > > * function where a BPF program can be attached. > > > > */ > > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ > > > > #include <linux/lsm_hook_defs.h> > > > > #undef LSM_HOOK > > > > > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ > > > > +noinline RET bpf_lsm_##NAME##_ret(int ret) \ > > > > +{ \ > > > > + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ > > > > +} > > > > + > > > > +#include <linux/lsm_hook_defs.h> > > > > +#undef LSM_HOOK > > > > + > > > > > > because lsm hooks is mess of undocumented return values your > > > "solution" is to add hundreds of noninline functions > > > and hack the call into them in JITs ?! > > > > I revisited the documentation and checked each LSM hook one by one. > > Hopefully, I completed it correctly, but I would review again (others > > are also welcome to do it). > > > > Not sure if there is a more efficient way. Do you have any idea? > > Maybe we find a way to use only one check function (by reusing the > > address of the attachment point?). > > > > Regarding the JIT approach, I didn't find a reliable solution for using > > just the verifier. As I wrote to you, there could be the case where the > > range can include positive values, despite the possible return values > > are zero and -EACCES. > > Didn't you find that there are only 12 or so odd return cases. > Maybe refactor some of them to something that the verifier can enforce > and denylist the rest ? Ok, went back to trying to enforce the return value on the verifier side, assuming that for now we consider hooks that return zero or a negative value. I wanted to see if at least we are able to enforce that. The biggest problem is which value of the register I should use, the 64 bit one or the 32 bit one? We can have a look at test_libbpf_get_fd_by_id_opts. The default flavor gives: 0000000000000000 <check_access>: 0: b4 00 00 00 00 00 00 00 w0 = 0 1: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0) 2: 18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r3 = 0 ll 4: 5d 32 05 00 00 00 00 00 if r2 != r3 goto +5 <LBB0_3> 5: 79 11 08 00 00 00 00 00 r1 = *(u64 *)(r1 + 8) 6: 57 01 00 00 02 00 00 00 r1 &= 2 7: b4 00 00 00 00 00 00 00 w0 = 0 8: 15 01 01 00 00 00 00 00 if r1 == 0 goto +1 <LBB0_3> 9: b4 00 00 00 f3 ff ff ff w0 = -13 smin_value = 0xfffffff3, smax_value = 0xfffffff3, s32_min_value = 0xfffffff3, s32_max_value = 0xfffffff3, I think it is because of this, in check_alu_op(): if (BPF_CLASS(insn->code) == BPF_ALU64) { __mark_reg_known(regs + insn->dst_reg, insn->imm); } else { __mark_reg_known(regs + insn->dst_reg, (u32)insn->imm); } } So, here you have to use the 32 bit values. But, if you use the no_alu32 flavor: 0000000000000000 <check_access>: 0: b7 00 00 00 00 00 00 00 r0 = 0 1: 79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0) 2: 18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r3 = 0 ll 4: 5d 32 04 00 00 00 00 00 if r2 != r3 goto +4 <LBB0_2> 5: 79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8) 6: 67 00 00 00 3e 00 00 00 r0 <<= 62 7: c7 00 00 00 3f 00 00 00 r0 s>>= 63 smin_value = 0xffffffffffffffff, smax_value = 0x0, s32_min_value = 0x80000000, s32_max_value = 0x7fffffff, 8: 57 00 00 00 f3 ff ff ff r0 &= -13 smin_value = 0xfffffffffffffff3, smax_value = 0x7fffffffffffffff, s32_min_value = 0x80000000, s32_max_value = 0x7ffffff3, I would have hoped to see: smin_value = 0xfffffffffffffff3, smax_value = 0x0, but it doesn't because of this, in scalar_min_max_and(): if (dst_reg->smin_value < 0 || smin_val < 0) { /* Lose signed bounds when ANDing negative numbers, * ain't nobody got time for that. */ dst_reg->smin_value = S64_MIN; dst_reg->smax_value = S64_MAX; Could we do an AND, if src_reg is known? And what would be the right register value to use? Thanks Roberto
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index 62f805f427b7..5412230c6935 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs) */ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, struct bpf_tramp_links *tlinks, void *orig_call, - int nargs, u32 flags) + void *ret_check_call, int nargs, u32 flags) { int i; int stack_size; @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im, int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, const struct btf_func_model *m, u32 flags, struct bpf_tramp_links *tlinks, - void *orig_call) + void *orig_call, void *ret_check_call) { int i, ret; int nargs = m->nr_args; @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, return -ENOTSUPP; } - ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags); + ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call, + nargs, flags); if (ret < 0) return ret; diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index cec5195602bc..6cd727b4af0a 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog, int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, const struct btf_func_model *m, u32 flags, struct bpf_tramp_links *tlinks, - void *func_addr) + void *func_addr, void *func_ret_check_addr) { int ret, i, nr_args = m->nr_args, extra_nregs = 0; int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off; @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i for (i = 0; i < fmod_ret->nr_links; i++) emit_cond_near_jump(&branches[i], prog, branches[i], X86_JNE); + + if (func_ret_check_addr) { + emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8); + + /* call ret check function */ + if (emit_call(&prog, func_ret_check_addr, prog)) { + ret = -EINVAL; + goto cleanup; + } + + /* remember return value in a stack for bpf prog to access */ + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8); + memcpy(prog, x86_nops[5], X86_PATCH_SIZE); + prog += X86_PATCH_SIZE; + } } if (fexit->nr_links) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 49f9d2bec401..f3551f7bdc28 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -919,7 +919,7 @@ struct bpf_tramp_image; int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, const struct btf_func_model *m, u32 flags, struct bpf_tramp_links *tlinks, - void *orig_call); + void *orig_call, void *ret_call); u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog, struct bpf_tramp_run_ctx *run_ctx); void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start, @@ -974,6 +974,7 @@ struct bpf_trampoline { struct { struct btf_func_model model; void *addr; + void *ret_check_addr; bool ftrace_managed; } func; /* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF @@ -994,6 +995,7 @@ struct bpf_trampoline { struct bpf_attach_target_info { struct btf_func_model fmodel; long tgt_addr; + long tgt_ret_check_addr; const char *tgt_name; const struct btf_type *tgt_type; }; diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index 37bcedf5a44e..f7f25d0064dd 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -18,6 +18,17 @@ #include <linux/ima.h> #include <linux/bpf-cgroup.h> +static bool is_ret_value_allowed(int ret, u32 ret_flags) +{ + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) + return false; + + return true; +} + /* For every LSM hook that allows attachment of BPF programs, declare a nop * function where a BPF program can be attached. */ @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ #include <linux/lsm_hook_defs.h> #undef LSM_HOOK +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ +noinline RET bpf_lsm_##NAME##_ret(int ret) \ +{ \ + return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \ +} + +#include <linux/lsm_hook_defs.h> +#undef LSM_HOOK + #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ BTF_ID(func, bpf_lsm_##NAME) BTF_SET_START(bpf_lsm_hooks) diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 84b2d9dba79a..22485f0df534 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks, */ flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0; return arch_prepare_bpf_trampoline(NULL, image, image_end, - model, flags, tlinks, NULL); + model, flags, tlinks, NULL, NULL); } static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index d6395215b849..3c6821b3c08c 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE, &tr->func.model, tr->flags, tlinks, - tr->func.addr); + tr->func.addr, + tr->func.ret_check_addr); if (err < 0) goto out; @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key, memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel)); tr->func.addr = (void *)tgt_info->tgt_addr; + tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr; out: mutex_unlock(&tr->mutex); return tr; @@ -1055,7 +1057,7 @@ int __weak arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end, const struct btf_func_model *m, u32 flags, struct bpf_tramp_links *tlinks, - void *orig_call) + void *orig_call, void *ret_check_call) { return -ENOTSUPP; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5e74f460dfd0..1ad0fe5cefe9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, { bool prog_extension = prog->type == BPF_PROG_TYPE_EXT; const char prefix[] = "btf_trace_"; - int ret = 0, subprog = -1, i; + int ret = 0, subprog = -1, i, tname_len; const struct btf_type *t; bool conservative = true; const char *tname; + char *tname_ret; struct btf *btf; - long addr = 0; + long addr = 0, ret_check_addr = 0; if (!btf_id) { bpf_log(log, "Tracing programs must provide btf_id\n"); @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, tname); return -ENOENT; } + + if (prog->expected_attach_type == BPF_LSM_MAC) { + tname_len = strlen(tname); + tname_ret = kmalloc(tname_len + 5, GFP_KERNEL); + if (!tname_ret) { + bpf_log(log, + "Cannot allocate memory for %s_ret string\n", + tname); + return -ENOMEM; + } + + snprintf(tname_ret, tname_len + 5, "%s_ret", tname); + ret_check_addr = kallsyms_lookup_name(tname_ret); + kfree(tname_ret); + + if (!ret_check_addr) { + bpf_log(log, + "Kernel symbol %s_ret not found\n", + tname); + return -ENOENT; + } + } } if (prog->aux->sleepable) { @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, break; } tgt_info->tgt_addr = addr; + tgt_info->tgt_ret_check_addr = ret_check_addr; tgt_info->tgt_name = tname; tgt_info->tgt_type = t; return 0;