Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Error cleanup path unmaps arbitrary memory if a driver illegally alters vma->vm_start. - [High] Driver resources and file reference are leaked when mmap_prepare_validate() fails after a successful hook. --
commit 7871b5bece83247c9fe3efaac4b928710f478390 Author: Lorenzo Stoakes (ARM) <[email protected]> mm: consistently validate VMA state after mmap[_prepare] hooks This commit introduces mmap_prepare_validate() and mmap_hook_validate() to verify that mmap hooks do not perform unauthorized operations, such as modifying the VMA start address. It applies these checks uniformly across VMA creation paths. > diff --git a/mm/internal.h b/mm/internal.h > --- a/mm/internal.h > +++ b/mm/internal.h [ ... ] > @@ -238,26 +259,14 @@ static inline int mmap_file(struct file *file, struct > vm_area_struct *vma) > */ > if (unlikely(err || !vma->vm_ops)) > vma->vm_ops = &vma_dummy_vm_ops; > + if (unlikely(err)) > + return err; > > + err = mmap_hook_validate(prev_start, &prev_flags, vma); > + if (unlikely(err)) > + vma_close(vma); > > + return err; > } [Severity: High] If a driver illegally alters vma->vm_start, mmap_hook_validate() will correctly catch the violation and return -EINVAL. However, does this leave the corrupted vma->vm_start in place when returning to the caller? Looking at __mmap_new_file_vma(), the error handling path relies on vma->vm_start: error = mmap_file(vma->vm_file, vma); if (error) { UNMAP_STATE(unmap, vmi, vma, vma->vm_start, vma->vm_end, map->prev, map->next); ... unmap_region(&unmap); return error; } If mmap_file() fails because the driver altered vma->vm_start, could unmap_region() accidentally unmap arbitrary user memory using the corrupted start address? > diff --git a/mm/util.c b/mm/util.c > --- a/mm/util.c > +++ b/mm/util.c [ ... ] > @@ -1224,10 +1224,17 @@ EXPORT_SYMBOL(compat_set_desc_from_vma); > int __compat_vma_mmap(struct vm_area_desc *desc, > struct vm_area_struct *vma) > { > + struct vm_area_desc prev_desc; > int err; > > + /* Derive state prior to mmap_prepare hook. */ > + compat_set_desc_from_vma(&prev_desc, desc->file, vma); > /* Perform any preparatory tasks for mmap action. */ > err = mmap_action_prepare(desc); > + if (err) > + return err; > + /* Check the caller did nothing crazy. */ > + err = mmap_prepare_validate(&prev_desc, desc); > if (err) > return err; [Severity: High] If mmap_prepare_validate() detects an invariant violation and returns an error here, does this early return leak driver resources and the file reference? If the driver's mmap_prepare() hook succeeded, it may have allocated resources (expecting them to be freed later via vm_ops->close) or placed a new file reference in desc->vm_file. Since vma_close() is never called and the local desc structure is discarded, it appears these resources might be leaked. > diff --git a/mm/vma.c b/mm/vma.c > --- a/mm/vma.c > +++ b/mm/vma.c [ ... ] > @@ -2800,10 +2855,16 @@ static int call_mmap_prepare(struct mmap_state *map, > if (!desc->vm_ops) > return -EINVAL; > > + /* Perform any preparatory tasks for mmap action. */ > err = call_action_prepare(map, desc); > if (err) > return err; > > + /* Check the caller did nothing crazy. */ > + err = mmap_prepare_validate(&prev_desc, desc); > + if (err) > + return err; > + > /* Update fields permitted to be changed. */ > map->pgoff = desc->pgoff; [Severity: High] This early return appears to have the same resource leak behavior as __compat_vma_mmap() above. Would a validation failure here also skip releasing driver resources or a newly assigned desc->vm_file before the VMA is fully established? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260908-b4-mmap-prepare-vma-flag-sanify-v1-0-dacf19cce...@kernel.org?part=3
