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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpHLPLRYnhomHwPNk8X83dYOnRyE1aQjt6tf6BTGoEqTxw@mail.gmail.com>
Date:   Wed, 9 Feb 2022 19:48:00 -0800
From:   Suren Baghdasaryan <surenb@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Colin Cross <ccross@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Michal Hocko <mhocko@...e.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Matthew Wilcox <willy@...radead.org>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Johannes Weiner <hannes@...xchg.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>, brauner@...nel.org,
        legion@...nel.org, ran.xiaokai@....com.cn, sashal@...nel.org,
        Chris Hyser <chris.hyser@...cle.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Peter Collingbourne <pcc@...gle.com>, caoxiaofeng@...ong.com,
        David Hildenbrand <david@...hat.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        linux-mm <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-team <kernel-team@...roid.com>,
        syzbot+aa7b3d4b35f9dc46a366@...kaller.appspotmail.com
Subject: Re: [PATCH 1/1] mm: Fix UAF when anon vma name is used after vma is freed

. .

On Wed, Feb 9, 2022 at 5:02 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Wed, Feb 9, 2022 at 4:33 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> >
> > On Wed,  9 Feb 2022 16:18:01 -0800 Suren Baghdasaryan <surenb@...gle.com> 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.

>
> >