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

[PATCH 1/1] mm: Fix UAF 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 passing madvise_update_vma a copy of the name.

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>
---
 include/linux/mm.h |  2 ++
 kernel/sys.c       |  1 -
 mm/madvise.c       | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 213cc569b192..e3490be76f35 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3370,6 +3370,8 @@ static inline int seal_check_future_write(int seals, struct vm_area_struct *vma)
 	return 0;
 }
 
+#define ANON_VMA_NAME_MAX_LEN	80
+
 #ifdef CONFIG_ANON_VMA_NAME
 int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
 			  unsigned long len_in, const char *name);
diff --git a/kernel/sys.c b/kernel/sys.c
index ecc4cf019242..4cb6dc4bc09c 100644
--- 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);
 
 out:
 	/*
-- 
2.35.0.263.gb82422642f-goog

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

[Andrew Morton]

On Wed,  9 Feb 2022 16:18:01 -0800 Suren Baghdasaryan <surenb@google.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?

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

>

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

>
> >

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