Re: [PATCH v3 04/40] mm: consistently validate VMA state after mmap[_prepare] hooks

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Wed, Sep 23, 2026 at 09:47:20AM -0700, Suren Baghdasaryan wrote:
> On Thu, Sep 17, 2026 at 9:25 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> >
> > When the f_op->mmap_prepare or deprecated f_op->mmap hooks are invoked, the
> > driver might have done something crazy that is not permitted by the kernel.
> >
> > Currently we check for three such cases in __mmap_new_file_vma(), but only
> > if the legacy f_op->mmap hook is used:
> >
> > * Did sparc ADI result in invalid flags?
> >
> > * Did the driver alter vma->vm_start?
> >
> > * Did the driver make a file-backed mapping on a read-only file writable?
> >
> > Generalise these checks for both mmap_prepare and mmap and apply to all
> > invocations of mmap_file(), the f_op->mmap and f_op->mmap_prepare handling
> > in the core VMA code and the mmap_prepare compatibility layer.
> >
> > Also extend the vm_start check to vm_end also - drivers must not change the
> > VMA range at all.
> >
> > We also WARN_ON_ONCE() on these conditions as they are things that should
> > simply not occur in the kernel and it's important to call it out when it
> > does.
> >
> > We invoke mmap_prepare_validate() after mmap_action_prepare(), as mmap
> > actions often manipulate state in the descriptor thus providing the final
> > state the VMA will be derived from.
> >
> > Also call mmap_validate_vma_flags() in insert_vm_struct() to ensure that
> > special regions which are inserted (such as a VDSO or VVAR) also satisfy
> > the sanity checks.
> >
> > This way every VMA established through an mmap hook, whether via mmap() or
> > the compatibility layer, or inserted via insert_vm_struct(), has been
> > validated. brk() VMAs never pass through a driver hook and so need no such
> > check.
> >
> > While we're here, also fixup a couple disjoint blocks of #ifdef CONFIG_MMU.
> >
> > Finally, update the VMA userland tests to reflect the change.
> >
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Thanks!

>
> > ---
> >  mm/internal.h                   |  51 ++++++++++++--------
> >  mm/util.c                       |  19 ++++++--
> >  mm/vma.c                        | 100 ++++++++++++++++++++++++++++++++++------
> >  mm/vma.h                        |  25 ++++++++--
> >  tools/testing/vma/include/dup.h |  10 ++++
> >  5 files changed, 163 insertions(+), 42 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index fe576d468af4..970fb34898b2 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -213,6 +213,24 @@ static inline void *folio_raw_mapping(const struct folio *folio)
> >         return (void *)(mapping & ~FOLIO_MAPPING_FLAGS);
> >  }
> >
> > +/*
> > + * If the VMA has a close hook then close it, and since closing it might leave
> > + * it in an inconsistent state which makes the use of any hooks suspect, clear
> > + * them down by installing dummy empty hooks.
> > + */
> > +static inline void vma_close(struct vm_area_struct *vma)
> > +{
> > +       if (vma->vm_ops && vma->vm_ops->close) {
> > +               vma->vm_ops->close(vma);
> > +
> > +               /*
> > +                * The mapping is in an inconsistent state, and no further hooks
> > +                * may be invoked upon it.
> > +                */
> > +               vma->vm_ops = &vma_dummy_vm_ops;
> > +       }
> > +}
> > +
> >  /*
> >   * This is a file-backed mapping, and is about to be memory mapped - invoke its
> >   * mmap hook and safely handle error conditions. On error, VMA hooks will be
> > @@ -225,8 +243,12 @@ static inline void *folio_raw_mapping(const struct folio *folio)
> >   */
> >  static inline int mmap_file(struct file *file, struct vm_area_struct *vma)
> >  {
> > -       int err = vfs_mmap(file, vma);
> > +       const unsigned long prev_start = vma->vm_start;
> > +       const unsigned long prev_end = vma->vm_end;
> > +       const vma_flags_t prev_flags = vma->flags;
>
> nit: Might be just me but when I see prev_XXX in VMA-related code I
> picture previous VMA in the address space. Maybe call these orig_XXX?

Sure, will change.

>
> > +       int err;
> >
> > +       err = vfs_mmap(file, vma);
> >         /*
> >          * Either we tried to call the file hook for mmap() and an error arose
> >          * or a driver set vma->vm_ops = NULL intending there to be no VMA
> > @@ -239,26 +261,17 @@ 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;
> >
> > -       return err;
> > -}
> > -
> > -/*
> > - * If the VMA has a close hook then close it, and since closing it might leave
> > - * it in an inconsistent state which makes the use of any hooks suspect, clear
> > - * them down by installing dummy empty hooks.
> > - */
> > -static inline void vma_close(struct vm_area_struct *vma)
> > -{
> > -       if (vma->vm_ops && vma->vm_ops->close) {
> > -               vma->vm_ops->close(vma);
> > -
> > -               /*
> > -                * The mapping is in an inconsistent state, and no further hooks
> > -                * may be invoked upon it.
> > -                */
> > -               vma->vm_ops = &vma_dummy_vm_ops;
> > +       err = mmap_hook_validate(prev_start, prev_end, &prev_flags, vma);
> > +       if (unlikely(err)) {
> > +               vma->vm_start = prev_start;
> > +               vma->vm_end = prev_end;
> > +               vma_close(vma);
> >         }
> > +
> > +       return err;
> >  }
> >
> >  /* unmap_vmas is in mm/memory.c */
> > diff --git a/mm/util.c b/mm/util.c
> > index 016932780925..bdd5923eebc7 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -1224,19 +1224,28 @@ 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) {
> > -               if (desc->vm_file != vma->vm_file)
> > -                       fput(desc->vm_file);
> > -               return err;
> > -       }
> > +       if (err)
> > +               goto err_put;
> > +       /* Check the caller did nothing crazy. */
> > +       err = mmap_prepare_validate(&prev_desc, desc);
> > +       if (err)
> > +               goto err_put;
> >         /* Update the VMA from the descriptor. */
> >         compat_set_vma_from_desc(vma, desc);
> >         /* Complete any specified mmap actions. */
> >         return mmap_action_complete(vma, &desc->action, /*is_compat=*/true);
> > +
> > +err_put:
> > +       if (desc->vm_file != vma->vm_file)
> > +               fput(desc->vm_file);
> > +       return err;
> >  }
> >  EXPORT_SYMBOL(__compat_vma_mmap);
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 05d2c676672e..d6ed10cefc8f 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -2623,16 +2623,6 @@ static int __mmap_new_file_vma(struct mmap_state *map,
> >                 return error;
> >         }
> >
> > -       /* Drivers cannot alter the address of the VMA. */
> > -       WARN_ON_ONCE(map->addr != vma->vm_start);
> > -       /*
> > -        * Drivers should not permit writability when previously it was
> > -        * disallowed.
> > -        */
> > -       VM_WARN_ON_ONCE(!vma_flags_same_pair(&map->vma_flags, &vma->flags) &&
> > -                       !vma_flags_test(&map->vma_flags, VMA_MAYWRITE_BIT) &&
> > -                       vma_test(vma, VMA_MAYWRITE_BIT));
> > -
> >         map->vma_flags = vma->flags;
> >
> >         return 0;
> > @@ -2710,11 +2700,6 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap,
> >                 vma->flags = map->vma_flags;
> >         }
> >
> > -#ifdef CONFIG_SPARC64
> > -       /* TODO: Fix SPARC ADI! */
> > -       WARN_ON_ONCE(!arch_validate_flags(map->vm_flags));
> > -#endif
> > -
> >         /* Lock the VMA since it is modified after insertion into VMA tree */
> >         vma_start_write(vma);
> >         vma_iter_store_new(vmi, vma);
> > @@ -2777,6 +2762,80 @@ static void __mmap_complete(struct mmap_state *map, struct vm_area_struct *vma)
> >         vma_set_page_prot(vma);
> >  }
> >
> > +/* Check to ensure that the VMA flags of a newly mapped VMA are sane. */
> > +static int mmap_validate_vma_flags(const vma_flags_t *flags)
> > +{
> > +#ifdef CONFIG_SPARC64
> > +       const vm_flags_t legacy_flags = vma_flags_to_legacy(*flags);
> > +
> > +       /* TODO: Fix SPARC ADI! */
> > +       if (WARN_ON_ONCE(!arch_validate_flags(legacy_flags)))
> > +               return -EINVAL;
> > +#endif
> > +
> > +       return 0;
> > +}
> > +
> > +/* Check to ensure a driver hasn't done something crazy. */
> > +static int mmap_validate(unsigned long prev_start, unsigned long prev_end,
> > +                        unsigned long curr_start, unsigned long curr_end,
> > +                        const vma_flags_t *prev_flags,
> > +                        const vma_flags_t *curr_flags)
> > +{
> > +       bool was_maywrite, is_maywrite;
> > +
> > +       /* Drivers cannot alter the range of the VMA. */
> > +       if (WARN_ON_ONCE(prev_start != curr_start || prev_end != curr_end))
> > +               return -EINVAL;
> > +
> > +       was_maywrite = vma_flags_test(prev_flags, VMA_MAYWRITE_BIT);
> > +       is_maywrite = vma_flags_test(curr_flags, VMA_MAYWRITE_BIT);
> > +
> > +       /* A driver may not make a previously unwritable mapping writable. */
> > +       if (WARN_ON_ONCE(!was_maywrite && is_maywrite))
> > +               return -EINVAL;
> > +
> > +       return mmap_validate_vma_flags(curr_flags);
> > +}
> > +
> > +/**
> > + * mmap_prepare_validate() - Ensure the driver hasn't violated invariants in its
> > + * f_op->mmap_prepare hook.
> > + * @prev_desc: The VMA descriptor prior to the mmap_prepare hook being called.
> > + * @desc: The VMA descriptor after the mmap_prepare hook has been called.
> > + *
> > + * Returns: 0 on success, otherwise an error.
> > + */
> > +int mmap_prepare_validate(const struct vm_area_desc *prev_desc,
> > +                         const struct vm_area_desc *desc)
> > +{
> > +       return mmap_validate(prev_desc->start, prev_desc->end,
> > +                            desc->start, desc->end,
> > +                            &prev_desc->vma_flags, &desc->vma_flags);
> > +}
> > +
> > +/**
> > + * mmap_hook_validate() - Ensure the driver hasn't violated invariants in
> > + * its f_op->mmap hook.
> > + * @prev_start: The start of the mapping prior to the mmap hook.
> > + * @prev_end: The end of the mapping prior to the mmap hook.
> > + * @prev_flags: The VMA flags set for the VMA prior to the mmap hook.
> > + * @vma: The VMA after the hook has been applied.
> > + *
> > + * Returns: 0 on success, otherwise an error.
> > + */
> > +int mmap_hook_validate(unsigned long prev_start, unsigned long prev_end,
> > +                      const vma_flags_t *prev_flags,
> > +                      const struct vm_area_struct *vma)
> > +{
> > +       const unsigned long start = vma->vm_start;
> > +       const unsigned long end = vma->vm_end;
> > +       const vma_flags_t *flags = &vma->flags;
> > +
> > +       return mmap_validate(prev_start, prev_end, start, end, prev_flags,
> > +                            flags);
> > +}
> > +
> >  static int call_action_prepare(struct mmap_state *map,
> >                                struct vm_area_desc *desc)
> >  {
> > @@ -2803,6 +2862,7 @@ static int call_action_prepare(struct mmap_state *map,
> >  static int call_mmap_prepare(struct mmap_state *map,
> >                 struct vm_area_desc *desc)
> >  {
> > +       const struct vm_area_desc prev_desc = *desc;
> >         int err;
> >
> >         /* Invoke the hook. */
> > @@ -2822,6 +2882,11 @@ static int call_mmap_prepare(struct mmap_state *map,
> >         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;
> >         map->vma_flags = desc->vma_flags;
> > @@ -3457,10 +3522,15 @@ int __vm_munmap(unsigned long start, size_t len, bool unlock)
> >  int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
> >  {
> >         unsigned long charged = vma_pages(vma);
> > +       int err;
> >
> >         if (find_vma_intersection(mm, vma->vm_start, vma->vm_end))
> >                 return -ENOMEM;
> >
> > +       err = mmap_validate_vma_flags(&vma->flags);
> > +       if (err)
> > +               return err;
> > +
> >         if (vma_test(vma, VMA_ACCOUNT_BIT) &&
> >              security_vm_enough_memory_mm(mm, charged))
> >                 return -ENOMEM;
> > diff --git a/mm/vma.h b/mm/vma.h
> > index f15faa83f3d6..b2c3bc832a48 100644
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -782,14 +782,19 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *mm);
> >  struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig);
> >  void vm_area_free(struct vm_area_struct *vma);
> >
> > -/* vma_exec.c */
> >  #ifdef CONFIG_MMU
> > +int mmap_prepare_validate(const struct vm_area_desc *prev_desc,
> > +                         const struct vm_area_desc *desc);
> > +
> > +int mmap_hook_validate(unsigned long prev_start, unsigned long prev_end,
> > +                      const vma_flags_t *prev_flags,
> > +                      const struct vm_area_struct *vma);
> > +
> > +/* vma_exec.c */
> >  int create_init_stack_vma(struct mm_struct *mm, struct vm_area_struct **vmap,
> >                           unsigned long *top_mem_p);
> >  int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift);
> > -#endif
> >
> > -#ifdef CONFIG_MMU
> >  /*
> >   * Denies creating a writable executable mapping or gaining executable permissions.
> >   *
> > @@ -838,6 +843,20 @@ static inline bool map_deny_write_exec(const vma_flags_t *old,
> >
> >         return false;
> >  }
> > +#else
> > +static inline int mmap_prepare_validate(const struct vm_area_desc *prev_desc,
> > +                                       const struct vm_area_desc *desc)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int mmap_hook_validate(unsigned long prev_start,
> > +                                    unsigned long prev_end,
> > +                                    const vma_flags_t *prev_flags,
> > +                                    const struct vm_area_struct *vma)
> > +{
> > +       return 0;
> > +}
> >  #endif
> >
> >  struct vm_area_struct *__install_special_mapping(struct mm_struct *mm,
> > diff --git a/tools/testing/vma/include/dup.h b/tools/testing/vma/include/dup.h
> > index 2fd422789717..2986ae6ca1e5 100644
> > --- a/tools/testing/vma/include/dup.h
> > +++ b/tools/testing/vma/include/dup.h
> > @@ -1359,13 +1359,23 @@ static inline int vfs_mmap_prepare(struct file *file, struct vm_area_desc *desc)
> >         return file->f_op->mmap_prepare(desc);
> >  }
> >
> > +int mmap_prepare_validate(const struct vm_area_desc *prev_desc,
> > +                         const struct vm_area_desc *desc);
> > +
> >  static inline 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;
> >         /* Update the VMA from the descriptor. */
> >
> > --
> > 2.55.0
> >

--
Cheers, Lorenzo