linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	Jiri Kosina <jikos@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>, Shuah Khan <shuah@kernel.org>,
	Dave Marchevsky <davemarchevsky@fb.com>,
	Joe Stringer <joe@cilium.io>, Jonathan Corbet <corbet@lwn.net>
Cc: Tero Kristo <tero.kristo@linux.intel.com>,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-doc@vger.kernel.org,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: [PATCH bpf-next v10 02/23] bpf: split btf_check_subprog_arg_match in two
Date: Fri,  2 Sep 2022 15:29:17 +0200	[thread overview]
Message-ID: <20220902132938.2409206-3-benjamin.tissoires@redhat.com> (raw)
In-Reply-To: <20220902132938.2409206-1-benjamin.tissoires@redhat.com>

btf_check_subprog_arg_match() was used twice in verifier.c:
- when checking for the type mismatches between a (sub)prog declaration
  and BTF
- when checking the call of a subprog to see if the provided arguments
  are correct and valid

This is problematic when we check if the first argument of a program
(pointer to ctx) is correctly accessed:
To be able to ensure we access a valid memory in the ctx, the verifier
assumes the pointer to context is not null.
This has the side effect of marking the program accessing the entire
context, even if the context is never dereferenced.

For example, by checking the context access with the current code, the
following eBPF program would fail with -EINVAL if the ctx is set to null
from the userspace:

```
SEC("syscall")
int prog(struct my_ctx *args) {
  return 0;
}
```

In that particular case, we do not want to actually check that the memory
is correct while checking for the BTF validity, but we just want to
ensure that the (sub)prog definition matches the BTF we have.

So split btf_check_subprog_arg_match() in two so we can actually check
for the memory used when in a call, and ignore that part when not.

Note that a further patch is in preparation to disentangled
btf_check_func_arg_match() from these two purposes, and so right now we
just add a new hack around that by adding a boolean to this function.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

new in v10
---
 include/linux/bpf.h   |  2 ++
 kernel/bpf/btf.c      | 54 +++++++++++++++++++++++++++++++++++++++----
 kernel/bpf/verifier.c |  2 +-
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9c1674973e03..c9c72a089579 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1943,6 +1943,8 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 struct bpf_reg_state;
 int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 				struct bpf_reg_state *regs);
+int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
+			   struct bpf_reg_state *regs);
 int btf_check_kfunc_arg_match(struct bpf_verifier_env *env,
 			      const struct btf *btf, u32 func_id,
 			      struct bpf_reg_state *regs,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 903719b89238..eca9ea78ee5f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6170,7 +6170,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
 				    bool ptr_to_mem_ok,
-				    u32 kfunc_flags)
+				    u32 kfunc_flags,
+				    bool processing_call)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	bool rel = false, kptr_get = false, trusted_arg = false;
@@ -6356,7 +6357,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 					reg_ref_tname);
 				return -EINVAL;
 			}
-		} else if (ptr_to_mem_ok) {
+		} else if (ptr_to_mem_ok && processing_call) {
 			const struct btf_type *resolve_ret;
 			u32 type_size;
 
@@ -6431,7 +6432,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	return rel ? ref_regno : 0;
 }
 
-/* Compare BTF of a function with given bpf_reg_state.
+/* Compare BTF of a function declaration with given bpf_reg_state.
  * Returns:
  * EFAULT - there is a verifier bug. Abort verification.
  * EINVAL - there is a type mismatch or BTF is not available.
@@ -6458,7 +6459,50 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 		return -EINVAL;
 
 	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0);
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, false);
+
+	/* Compiler optimizations can remove arguments from static functions
+	 * or mismatched type can be passed into a global function.
+	 * In such cases mark the function as unreliable from BTF point of view.
+	 */
+	if (err)
+		prog->aux->func_info_aux[subprog].unreliable = true;
+	return err;
+}
+
+/* Compare BTF of a function call with given bpf_reg_state.
+ * Returns:
+ * EFAULT - there is a verifier bug. Abort verification.
+ * EINVAL - there is a type mismatch or BTF is not available.
+ * 0 - BTF matches with what bpf_reg_state expects.
+ * Only PTR_TO_CTX and SCALAR_VALUE states are recognized.
+ *
+ * NOTE: the code is duplicated from btf_check_subprog_arg_match()
+ * because btf_check_func_arg_match() is still doing both. Once that
+ * function is split in 2, we can call from here btf_check_subprog_arg_match()
+ * first, and then treat the calling part in a new code path.
+ */
+int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
+			   struct bpf_reg_state *regs)
+{
+	struct bpf_prog *prog = env->prog;
+	struct btf *btf = prog->aux->btf;
+	bool is_global;
+	u32 btf_id;
+	int err;
+
+	if (!prog->aux->func_info)
+		return -EINVAL;
+
+	btf_id = prog->aux->func_info[subprog].type_id;
+	if (!btf_id)
+		return -EFAULT;
+
+	if (prog->aux->func_info_aux[subprog].unreliable)
+		return -EINVAL;
+
+	is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, true);
 
 	/* Compiler optimizations can remove arguments from static functions
 	 * or mismatched type can be passed into a global function.
@@ -6474,7 +6518,7 @@ int btf_check_kfunc_arg_match(struct bpf_verifier_env *env,
 			      struct bpf_reg_state *regs,
 			      u32 kfunc_flags)
 {
-	return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags);
+	return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags, true);
 }
 
 /* Convert BTF of a function into bpf_reg_state if possible
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0194a36d0b36..d27fae3ce949 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6626,7 +6626,7 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 	func_info_aux = env->prog->aux->func_info_aux;
 	if (func_info_aux)
 		is_global = func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL;
-	err = btf_check_subprog_arg_match(env, subprog, caller->regs);
+	err = btf_check_subprog_call(env, subprog, caller->regs);
 	if (err == -EFAULT)
 		return err;
 	if (is_global) {
-- 
2.36.1


  parent reply	other threads:[~2022-09-02 13:57 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-02 13:29 [PATCH bpf-next v10 00/23] Introduce eBPF support for HID devices Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 01/23] selftests/bpf: regroup and declare similar kfuncs selftests in an array Benjamin Tissoires
2022-09-06  3:25   ` Kumar Kartikeya Dwivedi
2022-09-06  3:27     ` Kumar Kartikeya Dwivedi
2022-09-06 13:50       ` Benjamin Tissoires
2022-09-06 16:12         ` Benjamin Tissoires
2022-09-02 13:29 ` Benjamin Tissoires [this message]
2022-09-02 13:29 ` [PATCH bpf-next v10 03/23] bpf/verifier: allow all functions to read user provided context Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 04/23] selftests/bpf: add test for accessing ctx from syscall program type Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 05/23] bpf/btf: bump BTF_KFUNC_SET_MAX_CNT Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 06/23] bpf/verifier: allow kfunc to return an allocated mem Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 07/23] selftests/bpf: Add tests for kfunc returning a memory pointer Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 08/23] HID: core: store the unique system identifier in hid_device Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 09/23] HID: export hid_report_type to uapi Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 10/23] HID: convert defines of HID class requests into a proper enum Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 11/23] HID: Kconfig: split HID support and hid-core compilation Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 12/23] HID: initial BPF implementation Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 13/23] selftests/bpf: add tests for the HID-bpf initial implementation Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 14/23] HID: bpf: allocate data memory for device_event BPF programs Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 15/23] selftests/bpf/hid: add test to change the report size Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 16/23] HID: bpf: introduce hid_hw_request() Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 17/23] selftests/bpf: add tests for bpf_hid_hw_request Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 18/23] HID: bpf: allow to change the report descriptor Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 19/23] selftests/bpf: add report descriptor fixup tests Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 20/23] selftests/bpf: Add a test for BPF_F_INSERT_HEAD Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 21/23] samples/bpf: HID: add new hid_mouse example Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 22/23] samples/bpf: HID: add Surface Dial example Benjamin Tissoires
2022-09-02 13:29 ` [PATCH bpf-next v10 23/23] Documentation: add HID-BPF docs Benjamin Tissoires
2022-09-20 13:43 ` [PATCH bpf-next v10 00/23] Introduce eBPF support for HID devices Benjamin Tissoires

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220902132938.2409206-3-benjamin.tissoires@redhat.com \
    --to=benjamin.tissoires@redhat.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=davemarchevsky@fb.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jikos@kernel.org \
    --cc=joe@cilium.io \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=memxor@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=songliubraving@fb.com \
    --cc=tero.kristo@linux.intel.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).