| Message ID | 20220223153613.835563-3-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] mm: refactor vm_area_struct::anon_vma_name usage code | expand |
On Wed 23-02-22 07:36:13, Suren Baghdasaryan 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 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: Not that I want to nit pick on the wording here because destruction of the original vma will not result in UAF automatically. For that it would need to hold anon_vma_name with the last reference. So I would reformulate: "In cases when vma_merge merges the original vma and destroys it, it might result in UAF. For that the original vma would have to hold the anon_vma_name with the last reference. The following vma would need to contain a different anon_vma_name object with the same string. > madvise_vma_behavior(vma) > madvise_update_vma(vma, ..., anon_name == vma->anon_name) > vma_merge(vma) > __vma_adjust(vma) <-- merges vma with adjacent one > vm_area_free(vma) <-- frees the original vma > replace_vma_anon_name(anon_name) <-- UAF of vma->anon_name > > 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@google.com> > Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > changes in v5: > - Updated description, per Michal Hocko > > mm/madvise.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 1f2693dccf7b..38d0f515d548 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -131,6 +131,8 @@ static int replace_anon_vma_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, > @@ -945,6 +947,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > unsigned long behavior) > { > int error; > + struct anon_vma_name *anon_name; > unsigned long new_flags = vma->vm_flags; > > switch (behavior) { > @@ -1010,8 +1013,11 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > break; > } > > + anon_name = anon_vma_name(vma); > + anon_vma_name_get(anon_name); > error = madvise_update_vma(vma, prev, start, end, new_flags, > - anon_vma_name(vma)); > + anon_name); > + anon_vma_name_put(anon_name); > > out: > /* > -- > 2.35.1.473.g83b2b277ed-goog
diff --git a/mm/madvise.c b/mm/madvise.c index 1f2693dccf7b..38d0f515d548 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -131,6 +131,8 @@ static int replace_anon_vma_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, @@ -945,6 +947,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, unsigned long behavior) { int error; + struct anon_vma_name *anon_name; unsigned long new_flags = vma->vm_flags; switch (behavior) { @@ -1010,8 +1013,11 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, break; } + anon_name = anon_vma_name(vma); + anon_vma_name_get(anon_name); error = madvise_update_vma(vma, prev, start, end, new_flags, - anon_vma_name(vma)); + anon_name); + anon_vma_name_put(anon_name); out: /*
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 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(vma) madvise_update_vma(vma, ..., anon_name == vma->anon_name) vma_merge(vma) __vma_adjust(vma) <-- merges vma with adjacent one vm_area_free(vma) <-- frees the original vma replace_vma_anon_name(anon_name) <-- UAF of vma->anon_name 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@google.com> Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com --- changes in v5: - Updated description, per Michal Hocko mm/madvise.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)