On Wed, Feb 9, 2022 at 4:33 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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?
Yes, I considered that. It would require new get/put APIs for
anon_name and I thought I better keep it simple. This path is used
only by madvise() syscall, so the copy overhead should not be
critical. But if you think refcounting is more appropriate here I'll
happily rework it. It should still be quite simple. Please let me
know.
>
