On Wed, 9 Feb 2022 16:18:01 -0800 Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> When adjacent vmas are being merged it can result in the vma that was
> originally passed to madvise_update_vma being destroyed. In the current
> implementation, the name parameter passed to madvise_update_vma points
> directly to vma->anon_name->name and it is used after the call to
> vma_merge. In the cases when vma_merge merges the original vma and
> destroys it, this will result in use-after-free bug as shown below:
>
> madvise_vma_behavior << passes vma->anon_name->name as name param
> madvise_update_vma(name)
> vma_merge
> __vma_adjust
> vm_area_free <-- frees the vma
> replace_vma_anon_name(name) <-- UAF
>
> Fix this by passing madvise_update_vma a copy of the name.
>
> ...
>
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2263,7 +2263,6 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which,
>
> #ifdef CONFIG_ANON_VMA_NAME
>
> -#define ANON_VMA_NAME_MAX_LEN 80
> #define ANON_VMA_NAME_INVALID_CHARS "\\`$[]"
>
> static inline bool is_valid_name_char(char ch)
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 5604064df464..f36a5a9942d8 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -976,6 +976,8 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> {
> int error;
> unsigned long new_flags = vma->vm_flags;
> + char name_buf[ANON_VMA_NAME_MAX_LEN];
> + const char *anon_name;
>
> switch (behavior) {
> case MADV_REMOVE:
> @@ -1040,8 +1042,18 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> break;
> }
>
> + anon_name = vma_anon_name(vma);
> + if (anon_name) {
> + /*
> + * Make a copy of the name because vma might be destroyed when
> + * merged with another one and the name parameter might be used
> + * after that.
> + */
> + strcpy(name_buf, anon_name);
> + anon_name = name_buf;
> + }
> error = madvise_update_vma(vma, prev, start, end, new_flags,
> - vma_anon_name(vma));
> + anon_name);
anon_name is refcounted. Why not use kref_get()/kref_put() instead of
taking a copy?
