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. 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) <[email protected]> --- mm/internal.h | 49 +++++++++++++-------- mm/util.c | 19 +++++--- mm/vma.c | 96 ++++++++++++++++++++++++++++++++++------- mm/vma.h | 24 +++++++++-- tools/testing/vma/include/dup.h | 10 +++++ 5 files changed, 156 insertions(+), 42 deletions(-) diff --git a/mm/internal.h b/mm/internal.h index fe576d468af4..1ed11eaf276a 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,11 @@ 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 vma_flags_t prev_flags = vma->flags; + 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 +260,16 @@ 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_flags, vma); + if (unlikely(err)) { + vma->vm_start = prev_start; + 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 dc74b3d721cc..626a18c08a26 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,76 @@ 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 curr_start, + const vma_flags_t *prev_flags, + const vma_flags_t *curr_flags) +{ + bool was_maywrite, is_maywrite; + + /* Drivers cannot alter the address of the VMA. */ + if (WARN_ON_ONCE(prev_start != curr_start)) + 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, desc->start, + &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_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, + const vma_flags_t *prev_flags, + const struct vm_area_struct *vma) +{ + const unsigned long start = vma->vm_start; + const vma_flags_t *flags = &vma->flags; + + return mmap_validate(prev_start, start, prev_flags, flags); +} + static int call_action_prepare(struct mmap_state *map, struct vm_area_desc *desc) { @@ -2803,6 +2858,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. */ @@ -2828,6 +2884,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; + /* * MAP_PRIVATE-/dev/zero mappings are an ancient way of getting * anonymous mappings. Rather than allowing these mappings to be odd @@ -3455,10 +3516,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..77d395b8b103 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, + 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,19 @@ 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, + 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
