[PATCH bpf v2 0/2] Fix offset when fault occurs in strncpy_from_kernel_nofault()

[PATCH bpf v2 0/2] Fix offset when fault occurs in strncpy_from_kernel_nofault()

[Alban Crequy]

Hi,

This is v2 of the fix & selftest previously sent at:
https://lore.kernel.org/linux-mm/20221108195211.214025-1-flaniel@linux.microsoft.com/

Changes v1 to v2:
- add 'cc:stable', 'Fixes:' and review/ack tags
- update commitmsg and fix my email
- rebase on bpf tree and tag for bpf tree

Thanks!


Alban Crequy (2):
  maccess: fix writing offset in case of fault in
    strncpy_from_kernel_nofault()
  selftests: bpf: add a test when bpf_probe_read_kernel_str() returns
    EFAULT

 mm/maccess.c                                    | 2 +-
 tools/testing/selftests/bpf/prog_tests/varlen.c | 7 +++++++
 tools/testing/selftests/bpf/progs/test_varlen.c | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH bpf v2 1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel_nofault()

[Alban Crequy]

If a page fault occurs while copying the first byte, this function resets one
byte before dst.
As a consequence, an address could be modified and leaded to kernel crashes if
case the modified address was accessed later.

Fixes: b58294ead14c ("maccess: allow architectures to provide kernel probing directly")
Cc: <stable@vger.kernel.org> [5.8]
Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>
Tested-by: Francis Laniel <flaniel@linux.microsoft.com>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>

---

Changes v1 to v2:
- add 'cc:stable', 'Fixes:' and review tag
- fix my email
- rebase on bpf tree and tag for bpf tree
---
 mm/maccess.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index 5f4d240f67ec..074f6b086671 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -97,7 +97,7 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
 	return src - unsafe_addr;
 Efault:
 	pagefault_enable();
-	dst[-1] = '\0';
+	dst[0] = '\0';
 	return -EFAULT;
 }
 
-- 
2.36.1

[PATCH bpf v2 2/2] selftests: bpf: add a test when bpf_probe_read_kernel_str() returns EFAULT

[Alban Crequy]

This commit tests previous fix of bpf_probe_read_kernel_str().

The BPF helper bpf_probe_read_kernel_str should return -EFAULT when
given a bad source pointer and the target buffer should only be modified
to make the string NULL terminated.

bpf_probe_read_kernel_str() was previously inserting a NULL before the
beginning of the dst buffer. This test should ensure that the
implementation stays correct for now on.

Without the fix, this test will fail as follows:
  $ cd tools/testing/selftests/bpf
  $ make
  $ sudo ./test_progs --name=varlen
  ...
  test_varlen:FAIL:check got 0 != exp 66

Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Acked-by: Yonghong Song <yhs@fb.com>

Changes v1 to v2:
- add ack tag
- fix my email
- rebase on bpf tree and tag for bpf tree
---
 tools/testing/selftests/bpf/prog_tests/varlen.c | 7 +++++++
 tools/testing/selftests/bpf/progs/test_varlen.c | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/varlen.c b/tools/testing/selftests/bpf/prog_tests/varlen.c
index dd324b4933db..4d7056f8f177 100644
--- a/tools/testing/selftests/bpf/prog_tests/varlen.c
+++ b/tools/testing/selftests/bpf/prog_tests/varlen.c
@@ -63,6 +63,13 @@ void test_varlen(void)
 	CHECK_VAL(data->total4, size1 + size2);
 	CHECK(memcmp(data->payload4, exp_str, size1 + size2), "content_check",
 	      "doesn't match!\n");
+
+	CHECK_VAL(bss->ret_bad_read, -EFAULT);
+	CHECK_VAL(data->payload_bad[0], 0x42);
+	CHECK_VAL(data->payload_bad[1], 0x42);
+	CHECK_VAL(data->payload_bad[2], 0);
+	CHECK_VAL(data->payload_bad[3], 0x42);
+	CHECK_VAL(data->payload_bad[4], 0x42);
 cleanup:
 	test_varlen__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_varlen.c b/tools/testing/selftests/bpf/progs/test_varlen.c
index 3987ff174f1f..20eb7d422c41 100644
--- a/tools/testing/selftests/bpf/progs/test_varlen.c
+++ b/tools/testing/selftests/bpf/progs/test_varlen.c
@@ -19,6 +19,7 @@ __u64 payload1_len1 = 0;
 __u64 payload1_len2 = 0;
 __u64 total1 = 0;
 char payload1[MAX_LEN + MAX_LEN] = {};
+__u64 ret_bad_read = 0;
 
 /* .data */
 int payload2_len1 = -1;
@@ -36,6 +37,8 @@ int payload4_len2 = -1;
 int total4= -1;
 char payload4[MAX_LEN + MAX_LEN] = { 1 };
 
+char payload_bad[5] = { 0x42, 0x42, 0x42, 0x42, 0x42 };
+
 SEC("raw_tp/sys_enter")
 int handler64_unsigned(void *regs)
 {
@@ -61,6 +64,8 @@ int handler64_unsigned(void *regs)
 
 	total1 = payload - (void *)payload1;
 
+	ret_bad_read = bpf_probe_read_kernel_str(payload_bad + 2, 1, (void *) -1);
+
 	return 0;
 }
 
-- 
2.36.1

Re: [PATCH bpf v2 0/2] Fix offset when fault occurs in strncpy_from_kernel_nofault()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu, 10 Nov 2022 09:56:12 +0100 you wrote:
> Hi,
> 
> This is v2 of the fix & selftest previously sent at:
> https://lore.kernel.org/linux-mm/20221108195211.214025-1-flaniel@linux.microsoft.com/
> 
> Changes v1 to v2:
> - add 'cc:stable', 'Fixes:' and review/ack tags
> - update commitmsg and fix my email
> - rebase on bpf tree and tag for bpf tree
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel_nofault()
    https://git.kernel.org/bpf/bpf/c/8678ea06852c
  - [bpf,v2,2/2] selftests: bpf: add a test when bpf_probe_read_kernel_str() returns EFAULT
    https://git.kernel.org/bpf/bpf/c/9cd094829dae

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html