On 10.08.23 17:53, Ryan Roberts wrote:
On 14/07/2023 19:29, Axel Rasmussen wrote:
This commit removed an extra check for zero-length ranges, and folded it
into the common validate_range() helper used by all UFFD ioctls.
It failed to notice though that UFFDIO_COPY *only* called validate_range
on the dst range, not the src range. So removing this check actually let
us proceed with zero-length source ranges, eventually hitting a BUG
further down in the call stack.
The correct fix seems clear: call validate_range() on the src range too.
Other ioctls are not affected by this, as they only have one range, not
two (src + dst).
Reported-by: syzbot+42309678e0bc7b32f8e9@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=42309678e0bc7b32f8e9
Signed-off-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
---
fs/userfaultfd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 53a7220c4679..36d233759233 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1759,6 +1759,9 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
sizeof(uffdio_copy)-sizeof(__s64)))
goto out;
+ ret = validate_range(ctx->mm, uffdio_copy.src, uffdio_copy.len);
+ if (ret)
+ goto out;
ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
if (ret)
goto out;
Hi Axel,
I've just noticed that this patch, now in mm-unstable, regresses the mkdirty mm
selftest:
# [INFO] detected THP size: 2048 KiB
TAP version 13
1..6
# [INFO] PTRACE write access
ok 1 SIGSEGV generated, page not modified
# [INFO] PTRACE write access to THP
ok 2 SIGSEGV generated, page not modified
# [INFO] Page migration
ok 3 SIGSEGV generated, page not modified
# [INFO] Page migration of THP
ok 4 SIGSEGV generated, page not modified
# [INFO] PTE-mapping a THP
ok 5 SIGSEGV generated, page not modified
# [INFO] UFFDIO_COPY
not ok 6 UFFDIO_COPY failed
Bail out! 1 out of 6 tests failed
# Totals: pass:5 fail:1 xfail:0 xpass:0 skip:0 error:0
Whereas all 6 tests pass against v6.5-rc4.
I'm afraid I don't know the test well and haven't looked at what the issue might
be, but noticed and thought I should point it out.
That test (written by me ;) ) essentially does
src = malloc(pagesize);
dst = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE|MAP_ANON, -1, 0)
...
uffdio_copy.dst = (unsigned long) dst;
uffdio_copy.src = (unsigned long) src;
uffdio_copy.len = pagesize;
uffdio_copy.mode = 0;
if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy)) {
...
So src might not be aligned to a full page.
According to the man page:
"EINVAL Either dst or len was not a multiple of the system page size, or
the range specified by src and len or dst and len was invalid."
So, AFAIKT, there is no requirement for src to be page-aligned.
Using validate_range() on the src is wrong.
--
Cheers,
David / dhildenb
