Re: [PATCH 1/1] mm: Fix UAF when anon vma name is used after vma is freed

[Suren Baghdasaryan]

. .

On Wed, Feb 9, 2022 at 5:02 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> 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.

On second thought, we might have more places in the future we need to
stabilize anon_name, so put/get API can be useful. After prototyping
the refcounting approach it looks simple enough to use instead of
copying. Let me test it a bit and I'll post a replacement patch for
this one tomorrow.
Thanks,
Suren.

>
> >