[PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

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 raising the name refcount and stabilizing it. Introduce
vma_anon_name_{get/put} API for this purpose.

Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory")
Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
changes in v2:
- Replaces name copying with refcounting and added vma_anon_name_{get/put} API,
per Andrew Morton

 include/linux/mm_inline.h | 13 +++++++++++++
 mm/madvise.c              | 28 ++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index b725839dfe71..2ad9b28499b1 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -145,6 +145,11 @@ static __always_inline void del_page_from_lru_list(struct page *page,
  */
 extern const char *vma_anon_name(struct vm_area_struct *vma);
 
+/* mmap_lock should be read-locked */
+extern struct anon_vma_name *vma_anon_name_get(struct vm_area_struct *vma);
+
+extern void vma_anon_name_put(struct anon_vma_name *anon_name);
+
 /*
  * mmap_lock should be read-locked for orig_vma->vm_mm.
  * mmap_lock should be write-locked for new_vma->vm_mm or new_vma should be
@@ -176,6 +181,14 @@ static inline const char *vma_anon_name(struct vm_area_struct *vma)
 {
 	return NULL;
 }
+
+static inline
+struct anon_vma_name *vma_anon_name_get(struct vm_area_struct *vma)
+{
+	return NULL;
+}
+
+static inline void vma_anon_name_put(struct anon_vma_name *anon_name) {}
 static inline void dup_vma_anon_name(struct vm_area_struct *orig_vma,
 			      struct vm_area_struct *new_vma) {}
 static inline void free_vma_anon_name(struct vm_area_struct *vma) {}
diff --git a/mm/madvise.c b/mm/madvise.c
index 5604064df464..9cf069e574c0 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -103,6 +103,22 @@ const char *vma_anon_name(struct vm_area_struct *vma)
 	return vma->anon_name->name;
 }
 
+struct anon_vma_name *vma_anon_name_get(struct vm_area_struct *vma)
+{
+	if (!has_vma_anon_name(vma))
+		return NULL;
+
+	mmap_assert_locked(vma->vm_mm);
+
+	kref_get(&vma->anon_name->kref);
+	return vma->anon_name;
+}
+
+void vma_anon_name_put(struct anon_vma_name *anon_name)
+{
+	kref_put(&anon_name->kref, vma_anon_name_free);
+}
+
 void dup_vma_anon_name(struct vm_area_struct *orig_vma,
 		       struct vm_area_struct *new_vma)
 {
@@ -976,6 +992,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 {
 	int error;
 	unsigned long new_flags = vma->vm_flags;
+	struct anon_vma_name *anon_name;
 
 	switch (behavior) {
 	case MADV_REMOVE:
@@ -1040,8 +1057,15 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 		break;
 	}
 
-	error = madvise_update_vma(vma, prev, start, end, new_flags,
-				   vma_anon_name(vma));
+	anon_name = vma_anon_name_get(vma);
+	if (anon_name) {
+		error = madvise_update_vma(vma, prev, start, end, new_flags,
+					   anon_name->name);
+		vma_anon_name_put(anon_name);
+	} else {
+		error = madvise_update_vma(vma, prev, start, end, new_flags,
+					   NULL);
+	}
 
 out:
 	/*
-- 
2.35.1.265.g69c8d7142f-goog

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Michal Hocko]

On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> vma_anon_name_{get/put} API for this purpose.

What is the reason that madvise_update_vma uses the naked name rather
than the encapsulated anon_vma_name? This really just begs for problems.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Matthew Wilcox]

On Wed, Feb 09, 2022 at 08:32:15PM -0800, Suren Baghdasaryan wrote:
> +void vma_anon_name_put(struct anon_vma_name *anon_name)
> +{
> +	kref_put(&anon_name->kref, vma_anon_name_free);
> +}

To agree with Michal, make this:

	if (anon_name)
		kref_put(&anon_name->kref, vma_anon_name_free);

>  
> -	error = madvise_update_vma(vma, prev, start, end, new_flags,
> -				   vma_anon_name(vma));
> +	anon_name = vma_anon_name_get(vma);
> +	if (anon_name) {
> +		error = madvise_update_vma(vma, prev, start, end, new_flags,
> +					   anon_name->name);
> +		vma_anon_name_put(anon_name);
> +	} else {
> +		error = madvise_update_vma(vma, prev, start, end, new_flags,
> +					   NULL);
> +	}

And then this becomes:

	anon_name = vma_anon_name_get(vma);
	error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name);
	vma_anon_name_put(anon_name);

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
<kernel-team@android.com> wrote:
>
> On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > vma_anon_name_{get/put} API for this purpose.
>
> What is the reason that madvise_update_vma uses the naked name rather
> than the encapsulated anon_vma_name? This really just begs for problems.

The reason for that is the second place it's being used from the prctl syscall:

prctl_set_vma
  madvise_set_anon_name
    madvise_vma_anon_name
      madvise_update_vma

In that case the name parameter is not part of any anon_vma_name
struct and therefore is stable. I can add a comment to
madvise_update_vma indicating that the name parameter has to be stable
if that helps.

> --
> Michal Hocko
> SUSE Labs
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

On Thu, Feb 10, 2022 at 5:27 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Feb 09, 2022 at 08:32:15PM -0800, Suren Baghdasaryan wrote:
> > +void vma_anon_name_put(struct anon_vma_name *anon_name)
> > +{
> > +     kref_put(&anon_name->kref, vma_anon_name_free);
> > +}
>
> To agree with Michal, make this:
>
>         if (anon_name)
>                 kref_put(&anon_name->kref, vma_anon_name_free);

Ack.

>
> >
> > -     error = madvise_update_vma(vma, prev, start, end, new_flags,
> > -                                vma_anon_name(vma));
> > +     anon_name = vma_anon_name_get(vma);
> > +     if (anon_name) {
> > +             error = madvise_update_vma(vma, prev, start, end, new_flags,
> > +                                        anon_name->name);
> > +             vma_anon_name_put(anon_name);
> > +     } else {
> > +             error = madvise_update_vma(vma, prev, start, end, new_flags,
> > +                                        NULL);
> > +     }
>
> And then this becomes:
>
>         anon_name = vma_anon_name_get(vma);
>         error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name);
>         vma_anon_name_put(anon_name);

As I indicated in the other reply, there is another madvise_update_vma
user which has only the name string, not the anon_vma_name struct. So
this can become:

   anon_name = vma_anon_name_get(vma);
   error = madvise_update_vma(vma, prev, start, end, new_flags,
                                                   anon_name ?
anon_name->name : NULL);
   vma_anon_name_put(anon_name);

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Matthew Wilcox]

On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote:
> On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > > vma_anon_name_{get/put} API for this purpose.
> >
> > What is the reason that madvise_update_vma uses the naked name rather
> > than the encapsulated anon_vma_name? This really just begs for problems.
> 
> The reason for that is the second place it's being used from the prctl syscall:
> 
> prctl_set_vma
>   madvise_set_anon_name
>     madvise_vma_anon_name
>       madvise_update_vma
> 
> In that case the name parameter is not part of any anon_vma_name
> struct and therefore is stable. I can add a comment to
> madvise_update_vma indicating that the name parameter has to be stable
> if that helps.

Seems to me it'd simplify things if replace_vma_anon_name() and
madvise_vma_anon_name() took a struct anon_vma_name instead of
a bare char *.  You could construct it in madvise_set_anon_name().

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote:
> > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
> > <kernel-team@android.com> wrote:
> > >
> > > On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > > > vma_anon_name_{get/put} API for this purpose.
> > >
> > > What is the reason that madvise_update_vma uses the naked name rather
> > > than the encapsulated anon_vma_name? This really just begs for problems.
> >
> > The reason for that is the second place it's being used from the prctl syscall:
> >
> > prctl_set_vma
> >   madvise_set_anon_name
> >     madvise_vma_anon_name
> >       madvise_update_vma
> >
> > In that case the name parameter is not part of any anon_vma_name
> > struct and therefore is stable. I can add a comment to
> > madvise_update_vma indicating that the name parameter has to be stable
> > if that helps.
>
> Seems to me it'd simplify things if replace_vma_anon_name() and
> madvise_vma_anon_name() took a struct anon_vma_name instead of
> a bare char *.  You could construct it in madvise_set_anon_name().

Ok, this can be done. However I don't think changing
replace_vma_anon_name() to accept a struct anon_vma_name would be a
good idea. Reader might think that the object being passed will become
the vma->anon_name of the vma, while in reality that's not the case.
Keeping it a char* makes it obvious that the function will construct a
new anon_vma_name struct.
I'll post a v3 shortly implementing these suggestions.
Thanks for the review folks!

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Matthew Wilcox]

On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote:
> On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote:
> > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
> > > <kernel-team@android.com> wrote:
> > > >
> > > > On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > > > > vma_anon_name_{get/put} API for this purpose.
> > > >
> > > > What is the reason that madvise_update_vma uses the naked name rather
> > > > than the encapsulated anon_vma_name? This really just begs for problems.
> > >
> > > The reason for that is the second place it's being used from the prctl syscall:
> > >
> > > prctl_set_vma
> > >   madvise_set_anon_name
> > >     madvise_vma_anon_name
> > >       madvise_update_vma
> > >
> > > In that case the name parameter is not part of any anon_vma_name
> > > struct and therefore is stable. I can add a comment to
> > > madvise_update_vma indicating that the name parameter has to be stable
> > > if that helps.
> >
> > Seems to me it'd simplify things if replace_vma_anon_name() and
> > madvise_vma_anon_name() took a struct anon_vma_name instead of
> > a bare char *.  You could construct it in madvise_set_anon_name().
> 
> Ok, this can be done. However I don't think changing
> replace_vma_anon_name() to accept a struct anon_vma_name would be a
> good idea. Reader might think that the object being passed will become
> the vma->anon_name of the vma, while in reality that's not the case.

Why woud we not want that to be the case?  It's a refcounted name.
I don't see why it shouldn't be shared between multiple VMAs that
have the same name?

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

On Thu, Feb 10, 2022 at 11:35 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote:
> > On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote:
> > > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
> > > > <kernel-team@android.com> wrote:
> > > > >
> > > > > On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > > > > > vma_anon_name_{get/put} API for this purpose.
> > > > >
> > > > > What is the reason that madvise_update_vma uses the naked name rather
> > > > > than the encapsulated anon_vma_name? This really just begs for problems.
> > > >
> > > > The reason for that is the second place it's being used from the prctl syscall:
> > > >
> > > > prctl_set_vma
> > > >   madvise_set_anon_name
> > > >     madvise_vma_anon_name
> > > >       madvise_update_vma
> > > >
> > > > In that case the name parameter is not part of any anon_vma_name
> > > > struct and therefore is stable. I can add a comment to
> > > > madvise_update_vma indicating that the name parameter has to be stable
> > > > if that helps.
> > >
> > > Seems to me it'd simplify things if replace_vma_anon_name() and
> > > madvise_vma_anon_name() took a struct anon_vma_name instead of
> > > a bare char *.  You could construct it in madvise_set_anon_name().
> >
> > Ok, this can be done. However I don't think changing
> > replace_vma_anon_name() to accept a struct anon_vma_name would be a
> > good idea. Reader might think that the object being passed will become
> > the vma->anon_name of the vma, while in reality that's not the case.
>
> Why woud we not want that to be the case?  It's a refcounted name.
> I don't see why it shouldn't be shared between multiple VMAs that
> have the same name?

You are right. After I reworked the code it became apparent that
replace_vma_anon_name() should use anon_vma_name. I have made that
change and am testing it now. Hopefully no new surprises pop up.
Thanks,
Suren.

>

Re: [PATCH v2 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

[Suren Baghdasaryan]

On Thu, Feb 10, 2022 at 11:52 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Thu, Feb 10, 2022 at 11:35 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote:
> > > On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote:
> > > > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team
> > > > > <kernel-team@android.com> wrote:
> > > > > >
> > > > > > On Wed 09-02-22 20:32:15, 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->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 raising the name refcount and stabilizing it. Introduce
> > > > > > > vma_anon_name_{get/put} API for this purpose.
> > > > > >
> > > > > > What is the reason that madvise_update_vma uses the naked name rather
> > > > > > than the encapsulated anon_vma_name? This really just begs for problems.
> > > > >
> > > > > The reason for that is the second place it's being used from the prctl syscall:
> > > > >
> > > > > prctl_set_vma
> > > > >   madvise_set_anon_name
> > > > >     madvise_vma_anon_name
> > > > >       madvise_update_vma
> > > > >
> > > > > In that case the name parameter is not part of any anon_vma_name
> > > > > struct and therefore is stable. I can add a comment to
> > > > > madvise_update_vma indicating that the name parameter has to be stable
> > > > > if that helps.
> > > >
> > > > Seems to me it'd simplify things if replace_vma_anon_name() and
> > > > madvise_vma_anon_name() took a struct anon_vma_name instead of
> > > > a bare char *.  You could construct it in madvise_set_anon_name().
> > >
> > > Ok, this can be done. However I don't think changing
> > > replace_vma_anon_name() to accept a struct anon_vma_name would be a
> > > good idea. Reader might think that the object being passed will become
> > > the vma->anon_name of the vma, while in reality that's not the case.
> >
> > Why woud we not want that to be the case?  It's a refcounted name.
> > I don't see why it shouldn't be shared between multiple VMAs that
> > have the same name?
>
> You are right. After I reworked the code it became apparent that
> replace_vma_anon_name() should use anon_vma_name. I have made that
> change and am testing it now. Hopefully no new surprises pop up.

v3 is posted at
https://lore.kernel.org/all/20220211013032.623763-1-surenb@google.com/

> Thanks,
> Suren.
>
> >