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] [day] [month] [year] [list]

Message-ID: <CAJuCfpGJjyg-QDaMyptzXz-CRiGn4_+Y-HxLFkJsLAL3XNwP_A@mail.gmail.com>
Date:   Wed, 9 Feb 2022 20:34:04 -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 7:48 PM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> . .
>
> 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.

Had some time to test and the patch seems stable. The refcounting
version is posted at:
https://lore.kernel.org/all/20220210043215.42794-1-surenb@google.com/
Thanks!

> Thanks,
> Suren.
>
> >
> > >