On Mon, Feb 21, 2022 at 9:40 PM 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 raising the name refcount and stabilizing it.
>
> Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory")
> Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Reported-by: syzbot+aa7b3d4b35f9dc46a366@xxxxxxxxxxxxxxxxxxxxxxxxx
> ---
> changes in v3:
> - Reapplied the fix after code refactoring, per Michal Hocko
Hi Andrew,
Since I needed to make some refactoring before adding this fix, in
order to apply this new version to mmotm you would need to revert the
previous version of this patch from your tree:
0cc16837d264 ("mm: fix use-after-free when anon vma name is used after
vma is freed")
and then apply the whole patchset (3 patches) after it is reviewed.
Sorry for the inconvenience but I think this way the refactoring and
the fix would be in the right order and with no overlap.
The patchset applies cleanly to Linus' ToT and to mmotm after
0cc16837d264 is reverted.
Thanks,
Suren.
>
> mm/madvise.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index a395884aeecb..00e8105430e9 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -140,6 +140,8 @@ static int replace_vma_anon_name(struct vm_area_struct *vma,
> /*
> * Update the vm_flags on region of a vma, splitting it or merging it as
> * necessary. Must be called with mmap_sem held for writing;
> + * Caller should ensure anon_name stability by raising its refcount even when
> + * anon_name belongs to a valid vma because this function might free that vma.
> */
> static int madvise_update_vma(struct vm_area_struct *vma,
> struct vm_area_struct **prev, unsigned long start,
> @@ -1021,8 +1023,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> }
>
> anon_name = vma_anon_name(vma);
> + anon_vma_name_get(anon_name);
> error = madvise_update_vma(vma, prev, start, end, new_flags,
> anon_name);
> + anon_vma_name_put(anon_name);
>
> out:
> /*
> --
> 2.35.1.473.g83b2b277ed-goog
>
