Received-SPF: pass (google.com: domain of linux-kernel+bounces-187800-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 23 May 2024 19:52:22 +0300
From: Marcel <nitan.marcel@gmail.com>
To: linux-kernel@vger.kernel.org
Subject: =?US-ASCII?Q?How_to_properly_fix_reading_user_po?=
 =?US-ASCII?Q?inters_in_bpf_in_android_kernel_4=2E9=3F?=
User-Agent: K-9 Mail for Android
Message-ID: <42DD54A2-D0C2-4A70-B461-7C16D3ECB8D2@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

This seems that it was a long standing problem with the Linux kernel in gen=
eral=2E bpf_probe_read should have worked for both kernel and user pointers=
 but it fails with access error when reading an user one instead=2E=20

I know there's a patch upstream that fixes this by introducing new helpers=
 for reading kernel and userspace pointers and I tried to back port them ba=
ck to my kernel but with no success=2E Tools like bcc fail to use them and =
instead they report that the arguments sent to the helpers are invalid=2E I=
 assume this is due to the arguments ARG_CONST_STACK_SIZE and ARG_PTR_TO_RA=
W_STACK handle data different in the 4=2E9 android version and the upstream=
 version but I'm not sure that this is the cause=2E I left the patch I did =
below and with a link to the kernel I'm working on and maybe someone can ta=
ke a look and give me an hand (the patch isn't applied yet)

<https://github=2Ecom/nitanmarcel/android_kernel_oneplus_sdm845-bpf>

diff --git a/include/uapi/linux/bpf=2Eh b/include/uapi/linux/bpf=2Eh
index 744b4763b80e=2E=2Ede94c13b7193 100644
--- a/include/uapi/linux/bpf=2Eh
+++ b/include/uapi/linux/bpf=2Eh
@@ -559,6 +559,43 @@ enum bpf_func_id {
    */
    BPF_FUNC_probe_read_user,
=20
+   /**
+   * int bpf_probe_read_kernel(void *dst, int size, void *src)
+   *     Read a kernel pointer safely=2E
+   *     Return: 0 on success or negative error
+   */
+   BPF_FUNC_probe_read_kernel,
+
+	/**
+	 * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)
+	 *     Copy a NUL terminated string from user unsafe address=2E In case =
the string
+	 *     length is smaller than size, the target is not padded with furthe=
r NUL
+	 *     bytes=2E In case the string length is larger than size, just coun=
t-1
+	 *     bytes are copied and the last byte is set to NUL=2E
+	 *     @dst: destination address
+	 *     @size: maximum number of bytes to copy, including the trailing NU=
L
+	 *     @unsafe_ptr: unsafe address
+	 *     Return:
+	 *       > 0 length of the string including the trailing NUL on success
+	 *       < 0 error
+	 */
+	BPF_FUNC_probe_read_user_str,
+
+	/**
+	 * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)
+	 *     Copy a NUL terminated string from unsafe address=2E In case the s=
tring
+	 *     length is smaller than size, the target is not padded with furthe=
r NUL
+	 *     bytes=2E In case the string length is larger than size, just coun=
t-1
+	 *     bytes are copied and the last byte is set to NUL=2E
+	 *     @dst: destination address
+	 *     @size: maximum number of bytes to copy, including the trailing NU=
L
+	 *     @unsafe_ptr: unsafe address
+	 *     Return:
+	 *       > 0 length of the string including the trailing NUL on success
+	 *       < 0 error
+	 */
+	BPF_FUNC_probe_read_kernel_str,
+
 	__BPF_FUNC_MAX_ID,
 };
=20
diff --git a/kernel/trace/bpf_trace=2Ec b/kernel/trace/bpf_trace=2Ec
index a1e37a5d8c88=2E=2E3478ca744a45 100644
--- a/kernel/trace/bpf_trace=2Ec
+++ b/kernel/trace/bpf_trace=2Ec
@@ -94,7 +94,7 @@ static const struct bpf_func_proto bpf_probe_read_proto =
=3D {
 	=2Earg3_type	=3D ARG_ANYTHING,
 };
=20
-BPF_CALL_3(bpf_probe_read_user, void *, dst, u32, size, const void *, uns=
afe_ptr)
+BPF_CALL_3(bpf_probe_read_user, void *, dst, u32, size, const void  __use=
r *, unsafe_ptr)
 {
 	int ret;
=20
@@ -115,6 +115,27 @@ static const struct bpf_func_proto bpf_probe_read_use=
r_proto =3D {
 };
=20
=20
+BPF_CALL_3(bpf_probe_read_kernel, void *, dst, u32, size, const void *, u=
nsafe_ptr)
+{
+	int ret;
+
+	ret =3D probe_kernel_read(dst, unsafe_ptr, size);
+	if (unlikely(ret < 0))
+		memset(dst, 0, size);
+
+	return ret;
+}
+
+static const struct bpf_func_proto bpf_probe_read_kernel_proto =3D {
+	=2Efunc		=3D bpf_probe_read_kernel,
+	=2Egpl_only	=3D true,
+	=2Eret_type	=3D RET_INTEGER,
+	=2Earg1_type	=3D ARG_PTR_TO_RAW_STACK,
+	=2Earg2_type	=3D ARG_CONST_STACK_SIZE,
+	=2Earg3_type	=3D ARG_ANYTHING,
+};
+
+
 BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
 	   u32, size)
 {
@@ -487,6 +508,69 @@ static const struct bpf_func_proto bpf_probe_read_str=
_proto =3D {
 	=2Earg3_type	=3D ARG_ANYTHING,
 };
=20
+
+
+BPF_CALL_3(bpf_probe_read_user_str, void *, dst, u32, size,
+	   const void __user *, unsafe_ptr)
+{
+	int ret;
+
+	/*
+	 * The strncpy_from_unsafe() call will likely not fill the entire
+	 * buffer, but that's okay in this circumstance as we're probing
+	 * arbitrary memory anyway similar to bpf_probe_read() and might
+	 * as well probe the stack=2E Thus, memory is explicitly cleared
+	 * only in error case, so that improper users ignoring return
+	 * code altogether don't copy garbage; otherwise length of string
+	 * is returned that can be used for bpf_perf_event_output() et al=2E
+	 */
+	ret =3D strncpy_from_unsafe_user(dst, unsafe_ptr, size);
+	if (unlikely(ret < 0))
+		memset(dst, 0, size);
+
+	return ret;
+}
+
+static const struct bpf_func_proto bpf_probe_read_user_str_proto =3D {
+	=2Efunc		=3D bpf_probe_read_user_str,
+	=2Egpl_only	=3D true,
+	=2Eret_type	=3D RET_INTEGER,
+	=2Earg1_type	=3D ARG_PTR_TO_RAW_STACK,
+	=2Earg2_type	=3D ARG_CONST_STACK_SIZE,
+	=2Earg3_type	=3D ARG_ANYTHING,
+};
+
+
+BPF_CALL_3(bpf_probe_read_kernel_str, void *, dst, u32, size,
+	   const void *, unsafe_ptr)
+{
+	int ret;
+
+	/*
+	 * The strncpy_from_unsafe() call will likely not fill the entire
+	 * buffer, but that's okay in this circumstance as we're probing
+	 * arbitrary memory anyway similar to bpf_probe_read() and might
+	 * as well probe the stack=2E Thus, memory is explicitly cleared
+	 * only in error case, so that improper users ignoring return
+	 * code altogether don't copy garbage; otherwise length of string
+	 * is returned that can be used for bpf_perf_event_output() et al=2E
+	 */
+	ret =3D strncpy_from_unsafe(dst, unsafe_ptr, size);
+	if (unlikely(ret < 0))
+		memset(dst, 0, size);
+
+	return ret;
+}
+
+static const struct bpf_func_proto bpf_probe_read_kernel_str_proto =3D {
+	=2Efunc		=3D bpf_probe_read_kernel_str,
+	=2Egpl_only	=3D true,
+	=2Eret_type	=3D RET_INTEGER,
+	=2Earg1_type	=3D ARG_PTR_TO_RAW_STACK,
+	=2Earg2_type	=3D ARG_CONST_STACK_SIZE,
+	=2Earg3_type	=3D ARG_ANYTHING,
+};
+
 static const struct bpf_func_proto *tracing_func_proto(enum bpf_func_id f=
unc_id)
 {
 	switch (func_id) {
@@ -500,8 +584,14 @@ static const struct bpf_func_proto *tracing_func_prot=
o(enum bpf_func_id func_id)
 		return &bpf_probe_read_proto;
 	case BPF_FUNC_probe_read_user:
 		return &bpf_probe_read_user_proto;
+	case BPF_FUNC_probe_read_kernel:
+		return &bpf_probe_read_kernel_proto;
 	case BPF_FUNC_probe_read_str:
 		return &bpf_probe_read_str_proto;
+	case BPF_FUNC_probe_read_user_str:
+		return &bpf_probe_read_user_str_proto;
+	case BPF_FUNC_probe_read_kernel_str:
+		return &bpf_probe_read_kernel_proto;
 	case BPF_FUNC_ktime_get_ns:
 		return &bpf_ktime_get_ns_proto;
 	case BPF_FUNC_tail_call:
diff --git a/tools/include/uapi/linux/bpf=2Eh b/tools/include/uapi/linux/b=
pf=2Eh
index 155ce25c069d=2E=2E91d5691288a7 100644
--- a/tools/include/uapi/linux/bpf=2Eh
+++ b/tools/include/uapi/linux/bpf=2Eh
@@ -522,7 +522,44 @@ enum bpf_func_id {
    *     Return: 0 on success or negative error
    */
    BPF_FUNC_probe_read_user,
+
+   /**
+   * int bpf_probe_read_kernel(void *dst, int size, void *src)
+   *     Read a kernel pointer safely=2E
+   *     Return: 0 on success or negative error
+   */
+   BPF_FUNC_probe_read_kernel,
 =09
+	/**
+	 * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)
+	 *     Copy a NUL terminated string from user unsafe address=2E In case =
the string
+	 *     length is smaller than size, the target is not padded with furthe=
r NUL
+	 *     bytes=2E In case the string length is larger than size, just coun=
t-1
+	 *     bytes are copied and the last byte is set to NUL=2E
+	 *     @dst: destination address
+	 *     @size: maximum number of bytes to copy, including the trailing NU=
L
+	 *     @unsafe_ptr: unsafe address
+	 *     Return:
+	 *       > 0 length of the string including the trailing NUL on success
+	 *       < 0 error
+	 */
+	BPF_FUNC_probe_read_user_str,
+
+	/**
+	 * int bpf_probe_read_str(void *dst, int size, const void *unsafe_ptr)
+	 *     Copy a NUL terminated string from unsafe address=2E In case the s=
tring
+	 *     length is smaller than size, the target is not padded with furthe=
r NUL
+	 *     bytes=2E In case the string length is larger than size, just coun=
t-1
+	 *     bytes are copied and the last byte is set to NUL=2E
+	 *     @dst: destination address
+	 *     @size: maximum number of bytes to copy, including the trailing NU=
L
+	 *     @unsafe_ptr: unsafe address
+	 *     Return:
+	 *       > 0 length of the string including the trailing NUL on success
+	 *       < 0 error
+	 */
+	BPF_FUNC_probe_read_kernel_str,
+ =20
   __BPF_FUNC_MAX_ID,
 };