Hi.
First of all, I hope you are fine and the same for your relatives.
This contribution fixes a bug where the byte before the destination address can
be reset when a page fault occurs in strncpy_from_kernel_nofault() while copying
the first byte from the source address.
This bug leaded to kernel panic if a pointer containing the modified address is
dereferenced as the pointer does not contain a correct addresss.
To fix this bug, we simply reset the current destination byte in a case of a
page fault.
The proposed fix was tested and validated inside a VM:
root@vm-amd64:~# ./share/linux/tools/testing/selftests/bpf/test_progs --name varlen
...
#222 varlen:OK
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
Without the patch, the test will fail:
root@vm-amd64:~# ./share/linux/tools/testing/selftests/bpf/test_progs --name varlen
...
#222 varlen:FAIL
Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
If you see any way to improve this contribution, feel free to share.
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(-)
Best regards and thank you in advance.
--
2.25.1
From: Alban Crequy <[email protected]>
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.
Signed-off-by: Alban Crequy <[email protected]>
Tested-by: Francis Laniel <[email protected]>
---
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.25.1
On 11/8/22 11:52 AM, Francis Laniel wrote:
> From: Alban Crequy <[email protected]>
>
> 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.
>
> Signed-off-by: Alban Crequy <[email protected]>
> Tested-by: Francis Laniel <[email protected]>
> ---
> 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';
What if the fault is due to dst, so the above won't work, right?
The original code should work fine if the first byte copy is successful.
For the first byte copy fault, maybe just to leave it as is?
> return -EFAULT;
> }
>
> --
> 2.25.1
>
On Tue, 8 Nov 2022 20:52:06 +0100 Francis Laniel <[email protected]> wrote:
> From: Alban Crequy <[email protected]>
>
> 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.
>
> Signed-off-by: Alban Crequy <[email protected]>
> Tested-by: Francis Laniel <[email protected]>
Reviewed-by: Andrew Morton <[email protected]>
Please merge via the bpf tree.
This looks potentially nasty. Fortunately only tracing code uses it,
but I'm thinking it should have cc:stable and a Fixes:?
Hi.
Le mardi 8 novembre 2022, 22:05:51 CET Andrew Morton a ?crit :
> On Tue, 8 Nov 2022 20:52:06 +0100 Francis Laniel
<[email protected]> wrote:
> > From: Alban Crequy <[email protected]>
> >
> > 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.
> >
> > Signed-off-by: Alban Crequy <[email protected]>
> > Tested-by: Francis Laniel <[email protected]>
>
> Reviewed-by: Andrew Morton <[email protected]>
>
> Please merge via the bpf tree.
>
> This looks potentially nasty. Fortunately only tracing code uses it,
> but I'm thinking it should have cc:stable and a Fixes:?
Thank you for the review!
Sorry, I thought to add stable list but forgot to add it when sending the
series...
I will sent a v2 with your review and without rfc tag to, among others,
stable.