Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] Denial of Service
Pre-existing issues:
- [High] Memory Leak
- [High] Use-After-Free / Double Free
--
commit 12d56e2f89cf46c2631f51c17242ae01f2acd00b
Author: Lorenzo Stoakes (ARM) [off-list ref]
mm/vma: enforce that only kernel-owned mappings may set VMA_IO_BIT
This patch updates mmap_validate_vma_flags to warn and fail if a mapping sets
VMA_IO_BIT without being marked as kernel-owned.
quoted hunk ↗ jump to hunk
diff --git a/mm/vma.c b/mm/vma.c
index cb0c4c6257567..5996757d5aaf4 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -2772,6 +2772,12 @@ static int mmap_validate_vma_flags(const vma_flags_t *flags)
return -EINVAL;
#endif
+ if (!vma_flags_test_any(flags, VMA_PFNMAP_BIT, VMA_MIXEDMAP_BIT)) {
+ /* Only kernel-owned mappings may set VMA_IO_BIT. */
+ if (WARN_ON_ONCE(vma_flags_test(flags, VMA_IO_BIT)))
[Severity: Medium]
Can this warning be triggered by an unprivileged user intentionally mapping
a device exposed by a buggy or out-of-tree driver?
If a user interacts with a buggy driver that sets VMA_IO_BIT improperly,
triggering this warning will crash the system if panic_on_warn=1 is
configured. Is there a way to validate this without the warning to
prevent a local denial of service?
+ return -EINVAL;
[Severity: High]
This is a pre-existing issue, but does returning an error here expose a
resource leak in call_mmap_prepare()?
If __mmap_region() handles an mmap call for a driver that implements the
.mmap_prepare hook, the driver may replace the mapping file:
mm/vma.c:call_mmap_prepare() {
...
err = mmap_prepare_validate(&prev_desc, desc);
if (err)
return err;
/* Update fields permitted to be changed. */
map->pgoff = desc->pgoff;
if (desc->vm_file != map->file) {
...
map->file = desc->vm_file;
map->file_doesnt_need_get = true;
}
...
}
If mmap_prepare_validate() fails because of this new VMA_IO_BIT check returning
an error, call_mmap_prepare() returns early. The state updates to map->file and
map->file_doesnt_need_get are bypassed.
When __mmap_region() aborts, it evaluates map.file_doesnt_need_get to decide
whether to clean up the new file:
mm/vma.c:__mmap_region() {
...
if (map.file_doesnt_need_get)
fput(map.file);
...
}
Since map.file_doesnt_need_get evaluates to false, is the reference to the
newly allocated file permanently lost?
[Severity: High]
This is also a pre-existing issue, but can this failure path also lead to a
double-free when mmap_file() fails?
If a driver's .mmap_prepare hook successfully replaces the mapping file,
call_mmap_prepare() sets map->file_doesnt_need_get = true. In
__mmap_new_file_vma(), the get_file() call is skipped.
If mmap_file() then fails (either because the driver's .mmap hook returns an
error, or this validation fails), the error path unconditionally drops the
reference:
mm/vma.c:__mmap_new_file_vma() {
...
error = mmap_file(map->file, vma, vmg);
if (error) {
...
fput(vma->vm_file);
vma->vm_file = NULL;
return error;
}
...
}
The error propagates back to __mmap_region(), which executes the abort_munmap
block:
mm/vma.c:__mmap_region() {
...
abort_munmap:
...
if (map.file_doesnt_need_get)
fput(map.file);
...
}
Because map.file_doesnt_need_get is still true, it calls fput() on the exact
same file pointer, leading to a double-free. Does this new validation step
make this sequence more reachable by unprivileged users interacting with
out-of-tree drivers?
quoted hunk ↗ jump to hunk
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-b4-mmap-prepare-vma-flag-sanify-v1-0-dacf19cce22b@kernel.org?part=24