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

[PATCH v3 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 v3:
- Change madvise_vma_anon_name and replace_vma_anon_name to accept struct
anon_vma_name* instead of char*, per Michal Hocko and Matthew Wilcox

 include/linux/mm_inline.h | 13 ++++++++
 mm/madvise.c              | 67 +++++++++++++++++++++++++++++----------
 2 files changed, 63 insertions(+), 17 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..1807778a5f70 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -70,6 +70,9 @@ static struct anon_vma_name *anon_vma_name_alloc(const char *name)
 	struct anon_vma_name *anon_name;
 	size_t count;
 
+	if (!name)
+		return NULL;
+
 	/* Add 1 for NUL terminator at the end of the anon_name->name */
 	count = strlen(name) + 1;
 	anon_name = kmalloc(struct_size(anon_name, name, count), GFP_KERNEL);
@@ -103,6 +106,23 @@ 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)
+{
+	if (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)
 {
@@ -126,33 +146,34 @@ void free_vma_anon_name(struct vm_area_struct *vma)
 }
 
 /* mmap_lock should be write-locked */
-static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
+static int replace_vma_anon_name(struct vm_area_struct *vma,
+				 struct anon_vma_name *anon_name)
 {
-	const char *anon_name;
+	const char *orig_name;
 
-	if (!name) {
+	if (!anon_name) {
 		free_vma_anon_name(vma);
 		return 0;
 	}
 
-	anon_name = vma_anon_name(vma);
-	if (anon_name) {
+	orig_name = vma_anon_name(vma);
+	if (orig_name) {
 		/* Same name, nothing to do here */
-		if (!strcmp(name, anon_name))
+		if (!strcmp(anon_name->name, orig_name))
 			return 0;
 
 		free_vma_anon_name(vma);
 	}
-	vma->anon_name = anon_vma_name_alloc(name);
-	if (!vma->anon_name)
-		return -ENOMEM;
+	kref_get(&anon_name->kref);
+	vma->anon_name = anon_name;
 
 	return 0;
 }
 #else /* CONFIG_ANON_VMA_NAME */
-static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
+static int replace_vma_anon_name(struct vm_area_struct *vma,
+				 struct anon_vma_name *anon_name)
 {
-	if (name)
+	if (anon_name)
 		return -EINVAL;
 
 	return 0;
@@ -161,12 +182,15 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
 /*
  * Update the vm_flags on region of a vma, splitting it or merging it as
  * necessary.  Must be called with mmap_sem held for writing;
+ * Caller should ensure anon_name stability by raising its refcount even when
+ * anon_name belongs to a valid vma because this function might free that vma.
  */
 static int madvise_update_vma(struct vm_area_struct *vma,
 			      struct vm_area_struct **prev, unsigned long start,
 			      unsigned long end, unsigned long new_flags,
-			      const char *name)
+			      struct anon_vma_name *anon_name)
 {
+	const char *name = anon_name ? anon_name->name : NULL;
 	struct mm_struct *mm = vma->vm_mm;
 	int error;
 	pgoff_t pgoff;
@@ -209,7 +233,7 @@ static int madvise_update_vma(struct vm_area_struct *vma,
 	 */
 	vma->vm_flags = new_flags;
 	if (!vma->vm_file) {
-		error = replace_vma_anon_name(vma, name);
+		error = replace_vma_anon_name(vma, anon_name);
 		if (error)
 			return error;
 	}
@@ -976,6 +1000,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 +1065,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 		break;
 	}
 
+	anon_name = vma_anon_name_get(vma);
 	error = madvise_update_vma(vma, prev, start, end, new_flags,
-				   vma_anon_name(vma));
+				   anon_name);
+	vma_anon_name_put(anon_name);
 
 out:
 	/*
@@ -1225,7 +1252,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
 static int madvise_vma_anon_name(struct vm_area_struct *vma,
 				 struct vm_area_struct **prev,
 				 unsigned long start, unsigned long end,
-				 unsigned long name)
+				 unsigned long anon_name)
 {
 	int error;
 
@@ -1234,7 +1261,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
 		return -EBADF;
 
 	error = madvise_update_vma(vma, prev, start, end, vma->vm_flags,
-				   (const char *)name);
+				   (struct anon_vma_name *)anon_name);
 
 	/*
 	 * madvise() returns EAGAIN if kernel resources, such as
@@ -1248,8 +1275,10 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
 int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
 			  unsigned long len_in, const char *name)
 {
+	struct anon_vma_name *anon_name;
 	unsigned long end;
 	unsigned long len;
+	int ret;
 
 	if (start & ~PAGE_MASK)
 		return -EINVAL;
@@ -1266,8 +1295,12 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
 	if (end == start)
 		return 0;
 
-	return madvise_walk_vmas(mm, start, end, (unsigned long)name,
+	anon_name = anon_vma_name_alloc(name);
+	ret = madvise_walk_vmas(mm, start, end, (unsigned long)anon_name,
 				 madvise_vma_anon_name);
+	vma_anon_name_put(anon_name);
+
+	return ret;
 }
 #endif /* CONFIG_ANON_VMA_NAME */
 /*
-- 
2.35.1.265.g69c8d7142f-goog

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

[Michal Hocko]

On Thu 10-02-22 17:30:32, Suren Baghdasaryan wrote:
[...]
> +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)
> +{
> +	if (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)
>  {
> @@ -126,33 +146,34 @@ void free_vma_anon_name(struct vm_area_struct *vma)
>  }
>  
>  /* mmap_lock should be write-locked */
> -static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
> +static int replace_vma_anon_name(struct vm_area_struct *vma,
> +				 struct anon_vma_name *anon_name)
>  {
> -	const char *anon_name;
> +	const char *orig_name;
>  
> -	if (!name) {
> +	if (!anon_name) {
>  		free_vma_anon_name(vma);
>  		return 0;
>  	}
>  
> -	anon_name = vma_anon_name(vma);
> -	if (anon_name) {
> +	orig_name = vma_anon_name(vma);
> +	if (orig_name) {
>  		/* Same name, nothing to do here */
> -		if (!strcmp(name, anon_name))
> +		if (!strcmp(anon_name->name, orig_name))
>  			return 0;
>  
>  		free_vma_anon_name(vma);
>  	}
> -	vma->anon_name = anon_vma_name_alloc(name);
> -	if (!vma->anon_name)
> -		return -ENOMEM;
> +	kref_get(&anon_name->kref);
> +	vma->anon_name = anon_name;

I really have to say that this is still confusing and potentially error
prone. I haven't really dived deeply in the early break out from strcmp
because I find your mixing strings and their referenced pointers rather
confusing in principle. I still do not see why you are avoiding a
relatively straighforward pattern. 

All you need is a shared pointer for your string and share
it as much as possible, no? Sharing means that you elevate the reference
counter whenever you assign to a vma and unshare when you are replacing
the shared pointer by a different one or when vma is freed. In other
words something like the following (pls note I have only compile tested
it and it very likely needs tweaks here and there I just wanted to point
the idea):

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 213cc569b192..689971696864 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2626,7 +2626,7 @@ static inline int vma_adjust(struct vm_area_struct *vma, unsigned long start,
 extern struct vm_area_struct *vma_merge(struct mm_struct *,
 	struct vm_area_struct *prev, unsigned long addr, unsigned long end,
 	unsigned long vm_flags, struct anon_vma *, struct file *, pgoff_t,
-	struct mempolicy *, struct vm_userfaultfd_ctx, const char *);
+	struct mempolicy *, struct vm_userfaultfd_ctx, struct anon_vma_name *);
 extern struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *);
 extern int __split_vma(struct mm_struct *, struct vm_area_struct *,
 	unsigned long addr, int new_below);
@@ -3372,11 +3372,11 @@ static inline int seal_check_future_write(int seals, struct vm_area_struct *vma)
 
 #ifdef CONFIG_ANON_VMA_NAME
 int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
-			  unsigned long len_in, const char *name);
+			  unsigned long len_in, struct anon_vma_name *name);
 #else
 static inline int
 madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
-		      unsigned long len_in, const char *name) {
+		      unsigned long len_in, struct anon_vma_name *name) {
 	return 0;
 }
 #endif
diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index b725839dfe71..cff619f762d0 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -145,45 +145,48 @@ 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 for orig_vma->vm_mm.
- * mmap_lock should be write-locked for new_vma->vm_mm or new_vma should be
- * isolated.
- */
-extern void dup_vma_anon_name(struct vm_area_struct *orig_vma,
-			      struct vm_area_struct *new_vma);
+static inline void anon_vma_name_get(struct anon_vma_name *name)
+{
+	if (!name)
+		return;
 
-/*
- * mmap_lock should be write-locked or vma should have been isolated under
- * write-locked mmap_lock protection.
- */
-extern void free_vma_anon_name(struct vm_area_struct *vma);
+	kref_get(&name->kref);
+}
 
-/* mmap_lock should be read-locked */
-static inline bool is_same_vma_anon_name(struct vm_area_struct *vma,
-					 const char *name)
+void vma_anon_name_free(struct kref *kref);
+static inline void anon_vma_name_put(struct anon_vma_name *name)
 {
-	const char *vma_name = vma_anon_name(vma);
+	if (!name)
+		return;
 
-	/* either both NULL, or pointers to same string */
-	if (vma_name == name)
+	kref_put(&name->kref, vma_anon_name_free);
+}
+
+static inline bool anon_vma_name_eq(struct anon_vma_name *name1, struct anon_vma_name *name2)
+{
+	if (name1 == name2)
 		return true;
 
-	return name && vma_name && !strcmp(name, vma_name);
+	return name1 && name2 && !strcmp(name1->name, name2->name);
+}
+
+static inline void free_vma_anon_name(struct vm_area_struct *vma)
+{
+	anon_vma_name_put(vma->anon_name);
 }
+
 #else /* CONFIG_ANON_VMA_NAME */
 static inline const char *vma_anon_name(struct vm_area_struct *vma)
 {
 	return NULL;
 }
-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) {}
-static inline bool is_same_vma_anon_name(struct vm_area_struct *vma,
-					 const char *name)
+static inline bool anon_vma_name_eq(struct anon_vma_name *name1, struct anon_vma_name *name2)
 {
 	return true;
 }
+static inline void free_vma_anon_name(struct vm_area_struct *vma)
+{
+}
 #endif  /* CONFIG_ANON_VMA_NAME */
 
 static inline void init_tlb_flush_pending(struct mm_struct *mm)
diff --git a/kernel/sys.c b/kernel/sys.c
index ecc4cf019242..662a0c8b4ceb 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2278,7 +2278,8 @@ static int prctl_set_vma(unsigned long opt, unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	const char __user *uname;
-	char *name, *pch;
+	char *uname, *pch;
+	struct anon_vma_name *vma_name = NULL;
 	int error;
 
 	switch (opt) {
@@ -2296,15 +2297,17 @@ static int prctl_set_vma(unsigned long opt, unsigned long addr,
 					return -EINVAL;
 				}
 			}
-		} else {
-			/* Reset the name */
-			name = NULL;
+
+			/* anon_vma has its own copy */
+			vma_name = anon_vma_name_alloc(name);
+			kfree(name);
+			if (!vma_name)
+				return PTR_ERR(vma_name);
 		}
 
 		mmap_write_lock(mm);
-		error = madvise_set_anon_name(mm, addr, size, name);
+		error = madvise_set_anon_name(mm, addr, size, vma_name);
 		mmap_write_unlock(mm);
-		kfree(name);
 		break;
 	default:
 		error = -EINVAL;
diff --git a/mm/madvise.c b/mm/madvise.c
index 5604064df464..f2f8065f67c1 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -65,7 +65,7 @@ static int madvise_need_mmap_write(int behavior)
 }
 
 #ifdef CONFIG_ANON_VMA_NAME
-static struct anon_vma_name *anon_vma_name_alloc(const char *name)
+struct anon_vma_name *anon_vma_name_alloc(const char *name)
 {
 	struct anon_vma_name *anon_name;
 	size_t count;
@@ -81,7 +81,7 @@ static struct anon_vma_name *anon_vma_name_alloc(const char *name)
 	return anon_name;
 }
 
-static void vma_anon_name_free(struct kref *kref)
+void vma_anon_name_free(struct kref *kref)
 {
 	struct anon_vma_name *anon_name =
 			container_of(kref, struct anon_vma_name, kref);
@@ -103,54 +103,25 @@ const char *vma_anon_name(struct vm_area_struct *vma)
 	return vma->anon_name->name;
 }
 
-void dup_vma_anon_name(struct vm_area_struct *orig_vma,
-		       struct vm_area_struct *new_vma)
-{
-	if (!has_vma_anon_name(orig_vma))
-		return;
-
-	kref_get(&orig_vma->anon_name->kref);
-	new_vma->anon_name = orig_vma->anon_name;
-}
-
-void free_vma_anon_name(struct vm_area_struct *vma)
-{
-	struct anon_vma_name *anon_name;
-
-	if (!has_vma_anon_name(vma))
-		return;
-
-	anon_name = vma->anon_name;
-	vma->anon_name = NULL;
-	kref_put(&anon_name->kref, vma_anon_name_free);
-}
-
 /* mmap_lock should be write-locked */
-static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
+static int replace_vma_anon_name(struct vm_area_struct *vma, struct anon_vma_name *name)
 {
-	const char *anon_name;
-
 	if (!name) {
-		free_vma_anon_name(vma);
+		anon_vma_name_put(vma->anon_name);
+		vma->anon_name = NULL;
 		return 0;
 	}
 
-	anon_name = vma_anon_name(vma);
-	if (anon_name) {
-		/* Same name, nothing to do here */
-		if (!strcmp(name, anon_name))
-			return 0;
-
-		free_vma_anon_name(vma);
-	}
-	vma->anon_name = anon_vma_name_alloc(name);
-	if (!vma->anon_name)
-		return -ENOMEM;
+	if (anon_vma_name_eq(vma->anon_name, name))
+		return 0;
 
+	anon_vma_name_put(vma->anon_name);
+	anon_vma_name_get(name);
+	vma->anon_name = name;
 	return 0;
 }
 #else /* CONFIG_ANON_VMA_NAME */
-static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
+static int replace_vma_anon_name(struct vm_area_struct *vma, struct anon_vma_name *name)
 {
 	if (name)
 		return -EINVAL;
@@ -165,13 +136,13 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
 static int madvise_update_vma(struct vm_area_struct *vma,
 			      struct vm_area_struct **prev, unsigned long start,
 			      unsigned long end, unsigned long new_flags,
-			      const char *name)
+			      struct anon_vma_name *name)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	int error;
 	pgoff_t pgoff;
 
-	if (new_flags == vma->vm_flags && is_same_vma_anon_name(vma, name)) {
+	if (new_flags == vma->vm_flags && anon_vma_name_eq(vma->anon_name, name)) {
 		*prev = vma;
 		return 0;
 	}
@@ -1041,7 +1012,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
 	}
 
 	error = madvise_update_vma(vma, prev, start, end, new_flags,
-				   vma_anon_name(vma));
+				   vma->anon_name);
 
 out:
 	/*
@@ -1234,7 +1205,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
 		return -EBADF;
 
 	error = madvise_update_vma(vma, prev, start, end, vma->vm_flags,
-				   (const char *)name);
+				   (struct anon_vma_name *)name);
 
 	/*
 	 * madvise() returns EAGAIN if kernel resources, such as
@@ -1246,7 +1217,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
 }
 
 int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
-			  unsigned long len_in, const char *name)
+			  unsigned long len_in, struct anon_vma_name *name)
 {
 	unsigned long end;
 	unsigned long len;
diff --git a/mm/mlock.c b/mm/mlock.c
index 8f584eddd305..e992755e8ffb 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -512,7 +512,7 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev,
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
 	*prev = vma_merge(mm, *prev, start, end, newflags, vma->anon_vma,
 			  vma->vm_file, pgoff, vma_policy(vma),
-			  vma->vm_userfaultfd_ctx, vma_anon_name(vma));
+			  vma->vm_userfaultfd_ctx, vma->anon_name);
 	if (*prev) {
 		vma = *prev;
 		goto success;
diff --git a/mm/mmap.c b/mm/mmap.c
index 1e8fdb0b51ed..c3b2e73d9c9a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1031,7 +1031,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 static inline int is_mergeable_vma(struct vm_area_struct *vma,
 				struct file *file, unsigned long vm_flags,
 				struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
-				const char *anon_name)
+				struct anon_vma_name *anon_name)
 {
 	/*
 	 * VM_SOFTDIRTY should not prevent from VMA merging, if we
@@ -1049,7 +1049,7 @@ static inline int is_mergeable_vma(struct vm_area_struct *vma,
 		return 0;
 	if (!is_mergeable_vm_userfaultfd_ctx(vma, vm_userfaultfd_ctx))
 		return 0;
-	if (!is_same_vma_anon_name(vma, anon_name))
+	if (!anon_vma_name_eq(vma->anon_name, anon_name))
 		return 0;
 	return 1;
 }
@@ -1084,7 +1084,7 @@ can_vma_merge_before(struct vm_area_struct *vma, unsigned long vm_flags,
 		     struct anon_vma *anon_vma, struct file *file,
 		     pgoff_t vm_pgoff,
 		     struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
-		     const char *anon_name)
+		     struct anon_vma_name *anon_name)
 {
 	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
 	    is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
@@ -1106,7 +1106,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
 		    struct anon_vma *anon_vma, struct file *file,
 		    pgoff_t vm_pgoff,
 		    struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
-		    const char *anon_name)
+		    struct anon_vma_name *anon_name)
 {
 	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
 	    is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
@@ -1167,7 +1167,7 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
 			struct anon_vma *anon_vma, struct file *file,
 			pgoff_t pgoff, struct mempolicy *policy,
 			struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
-			const char *anon_name)
+			struct anon_vma_name *anon_name)
 {
 	pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
 	struct vm_area_struct *area, *next;
@@ -3255,7 +3255,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 		return NULL;	/* should never get here */
 	new_vma = vma_merge(mm, prev, addr, addr + len, vma->vm_flags,
 			    vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
-			    vma->vm_userfaultfd_ctx, vma_anon_name(vma));
+			    vma->vm_userfaultfd_ctx, vma->anon_name);
 	if (new_vma) {
 		/*
 		 * Source vma may have been merged into new_vma
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 0138dfcdb1d8..dae529d24fa3 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -464,7 +464,7 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
 	pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
 	*pprev = vma_merge(mm, *pprev, start, end, newflags,
 			   vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
-			   vma->vm_userfaultfd_ctx, vma_anon_name(vma));
+			   vma->vm_userfaultfd_ctx, vma->anon_name);
 	if (*pprev) {
 		vma = *pprev;
 		VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
-- 
Michal Hocko
SUSE Labs

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

[Suren Baghdasaryan]

On Tue, Feb 15, 2022 at 8:00 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 10-02-22 17:30:32, Suren Baghdasaryan wrote:
> [...]
> > +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)
> > +{
> > +     if (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)
> >  {
> > @@ -126,33 +146,34 @@ void free_vma_anon_name(struct vm_area_struct *vma)
> >  }
> >
> >  /* mmap_lock should be write-locked */
> > -static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
> > +static int replace_vma_anon_name(struct vm_area_struct *vma,
> > +                              struct anon_vma_name *anon_name)
> >  {
> > -     const char *anon_name;
> > +     const char *orig_name;
> >
> > -     if (!name) {
> > +     if (!anon_name) {
> >               free_vma_anon_name(vma);
> >               return 0;
> >       }
> >
> > -     anon_name = vma_anon_name(vma);
> > -     if (anon_name) {
> > +     orig_name = vma_anon_name(vma);
> > +     if (orig_name) {
> >               /* Same name, nothing to do here */
> > -             if (!strcmp(name, anon_name))
> > +             if (!strcmp(anon_name->name, orig_name))
> >                       return 0;
> >
> >               free_vma_anon_name(vma);
> >       }
> > -     vma->anon_name = anon_vma_name_alloc(name);
> > -     if (!vma->anon_name)
> > -             return -ENOMEM;
> > +     kref_get(&anon_name->kref);
> > +     vma->anon_name = anon_name;
>
> I really have to say that this is still confusing and potentially error
> prone. I haven't really dived deeply in the early break out from strcmp
> because I find your mixing strings and their referenced pointers rather
> confusing in principle. I still do not see why you are avoiding a
> relatively straighforward pattern.
>
> All you need is a shared pointer for your string and share
> it as much as possible, no? Sharing means that you elevate the reference
> counter whenever you assign to a vma and unshare when you are replacing
> the shared pointer by a different one or when vma is freed. In other
> words something like the following (pls note I have only compile tested
> it and it very likely needs tweaks here and there I just wanted to point
> the idea):

Thanks for looking into this! Let me digest the proposal and will get
back with questions or a refactoring patch.

>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 213cc569b192..689971696864 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2626,7 +2626,7 @@ static inline int vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  extern struct vm_area_struct *vma_merge(struct mm_struct *,
>         struct vm_area_struct *prev, unsigned long addr, unsigned long end,
>         unsigned long vm_flags, struct anon_vma *, struct file *, pgoff_t,
> -       struct mempolicy *, struct vm_userfaultfd_ctx, const char *);
> +       struct mempolicy *, struct vm_userfaultfd_ctx, struct anon_vma_name *);
>  extern struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *);
>  extern int __split_vma(struct mm_struct *, struct vm_area_struct *,
>         unsigned long addr, int new_below);
> @@ -3372,11 +3372,11 @@ static inline int seal_check_future_write(int seals, struct vm_area_struct *vma)
>
>  #ifdef CONFIG_ANON_VMA_NAME
>  int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> -                         unsigned long len_in, const char *name);
> +                         unsigned long len_in, struct anon_vma_name *name);
>  #else
>  static inline int
>  madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> -                     unsigned long len_in, const char *name) {
> +                     unsigned long len_in, struct anon_vma_name *name) {
>         return 0;
>  }
>  #endif
> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index b725839dfe71..cff619f762d0 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -145,45 +145,48 @@ 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 for orig_vma->vm_mm.
> - * mmap_lock should be write-locked for new_vma->vm_mm or new_vma should be
> - * isolated.
> - */
> -extern void dup_vma_anon_name(struct vm_area_struct *orig_vma,
> -                             struct vm_area_struct *new_vma);
> +static inline void anon_vma_name_get(struct anon_vma_name *name)
> +{
> +       if (!name)
> +               return;
>
> -/*
> - * mmap_lock should be write-locked or vma should have been isolated under
> - * write-locked mmap_lock protection.
> - */
> -extern void free_vma_anon_name(struct vm_area_struct *vma);
> +       kref_get(&name->kref);
> +}
>
> -/* mmap_lock should be read-locked */
> -static inline bool is_same_vma_anon_name(struct vm_area_struct *vma,
> -                                        const char *name)
> +void vma_anon_name_free(struct kref *kref);
> +static inline void anon_vma_name_put(struct anon_vma_name *name)
>  {
> -       const char *vma_name = vma_anon_name(vma);
> +       if (!name)
> +               return;
>
> -       /* either both NULL, or pointers to same string */
> -       if (vma_name == name)
> +       kref_put(&name->kref, vma_anon_name_free);
> +}
> +
> +static inline bool anon_vma_name_eq(struct anon_vma_name *name1, struct anon_vma_name *name2)
> +{
> +       if (name1 == name2)
>                 return true;
>
> -       return name && vma_name && !strcmp(name, vma_name);
> +       return name1 && name2 && !strcmp(name1->name, name2->name);
> +}
> +
> +static inline void free_vma_anon_name(struct vm_area_struct *vma)
> +{
> +       anon_vma_name_put(vma->anon_name);
>  }
> +
>  #else /* CONFIG_ANON_VMA_NAME */
>  static inline const char *vma_anon_name(struct vm_area_struct *vma)
>  {
>         return NULL;
>  }
> -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) {}
> -static inline bool is_same_vma_anon_name(struct vm_area_struct *vma,
> -                                        const char *name)
> +static inline bool anon_vma_name_eq(struct anon_vma_name *name1, struct anon_vma_name *name2)
>  {
>         return true;
>  }
> +static inline void free_vma_anon_name(struct vm_area_struct *vma)
> +{
> +}
>  #endif  /* CONFIG_ANON_VMA_NAME */
>
>  static inline void init_tlb_flush_pending(struct mm_struct *mm)
> diff --git a/kernel/sys.c b/kernel/sys.c
> index ecc4cf019242..662a0c8b4ceb 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2278,7 +2278,8 @@ static int prctl_set_vma(unsigned long opt, unsigned long addr,
>  {
>         struct mm_struct *mm = current->mm;
>         const char __user *uname;
> -       char *name, *pch;
> +       char *uname, *pch;
> +       struct anon_vma_name *vma_name = NULL;
>         int error;
>
>         switch (opt) {
> @@ -2296,15 +2297,17 @@ static int prctl_set_vma(unsigned long opt, unsigned long addr,
>                                         return -EINVAL;
>                                 }
>                         }
> -               } else {
> -                       /* Reset the name */
> -                       name = NULL;
> +
> +                       /* anon_vma has its own copy */
> +                       vma_name = anon_vma_name_alloc(name);
> +                       kfree(name);
> +                       if (!vma_name)
> +                               return PTR_ERR(vma_name);
>                 }
>
>                 mmap_write_lock(mm);
> -               error = madvise_set_anon_name(mm, addr, size, name);
> +               error = madvise_set_anon_name(mm, addr, size, vma_name);
>                 mmap_write_unlock(mm);
> -               kfree(name);
>                 break;
>         default:
>                 error = -EINVAL;
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 5604064df464..f2f8065f67c1 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -65,7 +65,7 @@ static int madvise_need_mmap_write(int behavior)
>  }
>
>  #ifdef CONFIG_ANON_VMA_NAME
> -static struct anon_vma_name *anon_vma_name_alloc(const char *name)
> +struct anon_vma_name *anon_vma_name_alloc(const char *name)
>  {
>         struct anon_vma_name *anon_name;
>         size_t count;
> @@ -81,7 +81,7 @@ static struct anon_vma_name *anon_vma_name_alloc(const char *name)
>         return anon_name;
>  }
>
> -static void vma_anon_name_free(struct kref *kref)
> +void vma_anon_name_free(struct kref *kref)
>  {
>         struct anon_vma_name *anon_name =
>                         container_of(kref, struct anon_vma_name, kref);
> @@ -103,54 +103,25 @@ const char *vma_anon_name(struct vm_area_struct *vma)
>         return vma->anon_name->name;
>  }
>
> -void dup_vma_anon_name(struct vm_area_struct *orig_vma,
> -                      struct vm_area_struct *new_vma)
> -{
> -       if (!has_vma_anon_name(orig_vma))
> -               return;
> -
> -       kref_get(&orig_vma->anon_name->kref);
> -       new_vma->anon_name = orig_vma->anon_name;
> -}
> -
> -void free_vma_anon_name(struct vm_area_struct *vma)
> -{
> -       struct anon_vma_name *anon_name;
> -
> -       if (!has_vma_anon_name(vma))
> -               return;
> -
> -       anon_name = vma->anon_name;
> -       vma->anon_name = NULL;
> -       kref_put(&anon_name->kref, vma_anon_name_free);
> -}
> -
>  /* mmap_lock should be write-locked */
> -static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
> +static int replace_vma_anon_name(struct vm_area_struct *vma, struct anon_vma_name *name)
>  {
> -       const char *anon_name;
> -
>         if (!name) {
> -               free_vma_anon_name(vma);
> +               anon_vma_name_put(vma->anon_name);
> +               vma->anon_name = NULL;
>                 return 0;
>         }
>
> -       anon_name = vma_anon_name(vma);
> -       if (anon_name) {
> -               /* Same name, nothing to do here */
> -               if (!strcmp(name, anon_name))
> -                       return 0;
> -
> -               free_vma_anon_name(vma);
> -       }
> -       vma->anon_name = anon_vma_name_alloc(name);
> -       if (!vma->anon_name)
> -               return -ENOMEM;
> +       if (anon_vma_name_eq(vma->anon_name, name))
> +               return 0;
>
> +       anon_vma_name_put(vma->anon_name);
> +       anon_vma_name_get(name);
> +       vma->anon_name = name;
>         return 0;
>  }
>  #else /* CONFIG_ANON_VMA_NAME */
> -static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
> +static int replace_vma_anon_name(struct vm_area_struct *vma, struct anon_vma_name *name)
>  {
>         if (name)
>                 return -EINVAL;
> @@ -165,13 +136,13 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, const char *name)
>  static int madvise_update_vma(struct vm_area_struct *vma,
>                               struct vm_area_struct **prev, unsigned long start,
>                               unsigned long end, unsigned long new_flags,
> -                             const char *name)
> +                             struct anon_vma_name *name)
>  {
>         struct mm_struct *mm = vma->vm_mm;
>         int error;
>         pgoff_t pgoff;
>
> -       if (new_flags == vma->vm_flags && is_same_vma_anon_name(vma, name)) {
> +       if (new_flags == vma->vm_flags && anon_vma_name_eq(vma->anon_name, name)) {
>                 *prev = vma;
>                 return 0;
>         }
> @@ -1041,7 +1012,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
>         }
>
>         error = madvise_update_vma(vma, prev, start, end, new_flags,
> -                                  vma_anon_name(vma));
> +                                  vma->anon_name);
>
>  out:
>         /*
> @@ -1234,7 +1205,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
>                 return -EBADF;
>
>         error = madvise_update_vma(vma, prev, start, end, vma->vm_flags,
> -                                  (const char *)name);
> +                                  (struct anon_vma_name *)name);
>
>         /*
>          * madvise() returns EAGAIN if kernel resources, such as
> @@ -1246,7 +1217,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
>  }
>
>  int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> -                         unsigned long len_in, const char *name)
> +                         unsigned long len_in, struct anon_vma_name *name)
>  {
>         unsigned long end;
>         unsigned long len;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 8f584eddd305..e992755e8ffb 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -512,7 +512,7 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev,
>         pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
>         *prev = vma_merge(mm, *prev, start, end, newflags, vma->anon_vma,
>                           vma->vm_file, pgoff, vma_policy(vma),
> -                         vma->vm_userfaultfd_ctx, vma_anon_name(vma));
> +                         vma->vm_userfaultfd_ctx, vma->anon_name);
>         if (*prev) {
>                 vma = *prev;
>                 goto success;
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 1e8fdb0b51ed..c3b2e73d9c9a 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1031,7 +1031,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  static inline int is_mergeable_vma(struct vm_area_struct *vma,
>                                 struct file *file, unsigned long vm_flags,
>                                 struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -                               const char *anon_name)
> +                               struct anon_vma_name *anon_name)
>  {
>         /*
>          * VM_SOFTDIRTY should not prevent from VMA merging, if we
> @@ -1049,7 +1049,7 @@ static inline int is_mergeable_vma(struct vm_area_struct *vma,
>                 return 0;
>         if (!is_mergeable_vm_userfaultfd_ctx(vma, vm_userfaultfd_ctx))
>                 return 0;
> -       if (!is_same_vma_anon_name(vma, anon_name))
> +       if (!anon_vma_name_eq(vma->anon_name, anon_name))
>                 return 0;
>         return 1;
>  }
> @@ -1084,7 +1084,7 @@ can_vma_merge_before(struct vm_area_struct *vma, unsigned long vm_flags,
>                      struct anon_vma *anon_vma, struct file *file,
>                      pgoff_t vm_pgoff,
>                      struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -                    const char *anon_name)
> +                    struct anon_vma_name *anon_name)
>  {
>         if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
>             is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
> @@ -1106,7 +1106,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
>                     struct anon_vma *anon_vma, struct file *file,
>                     pgoff_t vm_pgoff,
>                     struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -                   const char *anon_name)
> +                   struct anon_vma_name *anon_name)
>  {
>         if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
>             is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
> @@ -1167,7 +1167,7 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
>                         struct anon_vma *anon_vma, struct file *file,
>                         pgoff_t pgoff, struct mempolicy *policy,
>                         struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -                       const char *anon_name)
> +                       struct anon_vma_name *anon_name)
>  {
>         pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
>         struct vm_area_struct *area, *next;
> @@ -3255,7 +3255,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>                 return NULL;    /* should never get here */
>         new_vma = vma_merge(mm, prev, addr, addr + len, vma->vm_flags,
>                             vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> -                           vma->vm_userfaultfd_ctx, vma_anon_name(vma));
> +                           vma->vm_userfaultfd_ctx, vma->anon_name);
>         if (new_vma) {
>                 /*
>                  * Source vma may have been merged into new_vma
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 0138dfcdb1d8..dae529d24fa3 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -464,7 +464,7 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
>         pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT);
>         *pprev = vma_merge(mm, *pprev, start, end, newflags,
>                            vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> -                          vma->vm_userfaultfd_ctx, vma_anon_name(vma));
> +                          vma->vm_userfaultfd_ctx, vma->anon_name);
>         if (*pprev) {
>                 vma = *pprev;
>                 VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
> --
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

Forgot to include one hunk:

diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
index cff619f762d0..17c20597e244 100644
--- a/include/linux/mm_inline.h
+++ b/include/linux/mm_inline.h
@@ -175,6 +175,11 @@ static inline void free_vma_anon_name(struct vm_area_struct *vma)
 	anon_vma_name_put(vma->anon_name);
 }
 
+static inline void dup_vma_anon_name(struct vm_area_struct *vma)
+{
+	anon_vma_name_get(vma->anon_name);
+}
+
 #else /* CONFIG_ANON_VMA_NAME */
 static inline const char *vma_anon_name(struct vm_area_struct *vma)
 {
@@ -187,6 +192,9 @@ static inline bool anon_vma_name_eq(struct anon_vma_name *name1, struct anon_vma
 static inline void free_vma_anon_name(struct vm_area_struct *vma)
 {
 }
+static inline void dup_vma_anon_name(struct vm_area_struct *vma)
+{
+}
 #endif  /* CONFIG_ANON_VMA_NAME */
 
 static inline void init_tlb_flush_pending(struct mm_struct *mm)
diff --git a/kernel/fork.c b/kernel/fork.c
index d75a528f7b21..dee514488003 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -366,7 +366,7 @@ struct vm_area_struct *vm_area_dup(struct vm_area_struct *orig)
 		*new = data_race(*orig);
 		INIT_LIST_HEAD(&new->anon_vma_chain);
 		new->vm_next = new->vm_prev = NULL;
-		dup_vma_anon_name(orig, new);
+		dup_vma_anon_name(new);
 	}
 	return new;
 }
-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

One thing I was considering is to check agains ref counte overflo (a
deep process chain with many vmas could grow really high. ref_count
interface doesn't provide any easy way to check for overflows as far as
I could see from a quick glance so I gave up there but the logic would
be really straightforward. We just create a new anon_vma_name with the same
content and use it when duplicating if the usage grow really
(arbitrarily) high.
-- 
Michal Hocko
SUSE Labs

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

[Suren Baghdasaryan]

On Tue, Feb 15, 2022 at 12:05 PM Michal Hocko <mhocko@suse.com> wrote:
>
> One thing I was considering is to check agains ref counte overflo (a
> deep process chain with many vmas could grow really high. ref_count
> interface doesn't provide any easy way to check for overflows as far as
> I could see from a quick glance so I gave up there but the logic would
> be really straightforward. We just create a new anon_vma_name with the same
> content and use it when duplicating if the usage grow really
> (arbitrarily) high.

I went over proposed changes. I see a couple small required fixes
(resetting the name to NULL seems to be missing and I think
dup_vma_anon_name needs some tweaking) but overall quite
straight-forward. I'll post a separate patch to do this refactoring.
The original patch is fixing the UAF issue, so I don't want to mix it
with refactoring. Please let me know if you see an issue with
separating it that way.
Thanks,
Suren.


> --
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Tue 15-02-22 15:02:54, Suren Baghdasaryan wrote:
> On Tue, Feb 15, 2022 at 12:05 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > One thing I was considering is to check agains ref counte overflo (a
> > deep process chain with many vmas could grow really high. ref_count
> > interface doesn't provide any easy way to check for overflows as far as
> > I could see from a quick glance so I gave up there but the logic would
> > be really straightforward. We just create a new anon_vma_name with the same
> > content and use it when duplicating if the usage grow really
> > (arbitrarily) high.
> 
> I went over proposed changes. I see a couple small required fixes
> (resetting the name to NULL seems to be missing and I think
> dup_vma_anon_name needs some tweaking) but overall quite
> straight-forward.

OK, great that this makes sense to you. As I've said I didn't really go
into details, not even dared to boot that to test. So it will very
likely need some more work but I do not expect this to grow much.

> I'll post a separate patch to do this refactoring.
> The original patch is fixing the UAF issue, so I don't want to mix it
> with refactoring. Please let me know if you see an issue with
> separating it that way.

Well, I am not sure TBH. Look at diffstats. Your fix
2 files changed, 63 insertions(+), 17 deletions(-)
the refactoring which should fix this and potentially others that might
be still lurking there (because mixing shared pointers and their internal
objects just begs for problems) is
7 files changed, 63 insertions(+), 86 deletions(-)

more files touched for sure but the net result is much more clear and a
much more code removed.
The overflow logic would make it bigger but I guess the existing scheme
needs it as well.

I would also claim that both approaches are really painful to review
because the existing model spreads into several areas and it is not
really clear you caught them all just by staring into the diff so both
will be rather painful to backport to older kernels. Fortunately this
would be only 5.17.
-- 
Michal Hocko
SUSE Labs

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

[Suren Baghdasaryan]

On Wed, Feb 16, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Tue 15-02-22 15:02:54, Suren Baghdasaryan wrote:
> > On Tue, Feb 15, 2022 at 12:05 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > One thing I was considering is to check agains ref counte overflo (a
> > > deep process chain with many vmas could grow really high. ref_count
> > > interface doesn't provide any easy way to check for overflows as far as
> > > I could see from a quick glance so I gave up there but the logic would
> > > be really straightforward. We just create a new anon_vma_name with the same
> > > content and use it when duplicating if the usage grow really
> > > (arbitrarily) high.
> >
> > I went over proposed changes. I see a couple small required fixes
> > (resetting the name to NULL seems to be missing and I think
> > dup_vma_anon_name needs some tweaking) but overall quite
> > straight-forward.
>
> OK, great that this makes sense to you. As I've said I didn't really go
> into details, not even dared to boot that to test. So it will very
> likely need some more work but I do not expect this to grow much.
>
> > I'll post a separate patch to do this refactoring.
> > The original patch is fixing the UAF issue, so I don't want to mix it
> > with refactoring. Please let me know if you see an issue with
> > separating it that way.
>
> Well, I am not sure TBH. Look at diffstats. Your fix
> 2 files changed, 63 insertions(+), 17 deletions(-)
> the refactoring which should fix this and potentially others that might
> be still lurking there (because mixing shared pointers and their internal
> objects just begs for problems) is
> 7 files changed, 63 insertions(+), 86 deletions(-)
>
> more files touched for sure but the net result is much more clear and a
> much more code removed.
> The overflow logic would make it bigger but I guess the existing scheme
> needs it as well.

Ok, I'll see how to slice it after it's complete and tested.
Thanks for the input!

>
> I would also claim that both approaches are really painful to review
> because the existing model spreads into several areas and it is not
> really clear you caught them all just by staring into the diff so both
> will be rather painful to backport to older kernels. Fortunately this
> would be only 5.17.
> --
> Michal Hocko
> SUSE Labs

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

[Suren Baghdasaryan]

On Thu, Feb 17, 2022 at 11:54 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Feb 16, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Tue 15-02-22 15:02:54, Suren Baghdasaryan wrote:
> > > On Tue, Feb 15, 2022 at 12:05 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > One thing I was considering is to check agains ref counte overflo (a
> > > > deep process chain with many vmas could grow really high. ref_count
> > > > interface doesn't provide any easy way to check for overflows as far as
> > > > I could see from a quick glance so I gave up there but the logic would
> > > > be really straightforward. We just create a new anon_vma_name with the same
> > > > content and use it when duplicating if the usage grow really
> > > > (arbitrarily) high.
> > >
> > > I went over proposed changes. I see a couple small required fixes
> > > (resetting the name to NULL seems to be missing and I think
> > > dup_vma_anon_name needs some tweaking) but overall quite
> > > straight-forward.
> >
> > OK, great that this makes sense to you. As I've said I didn't really go
> > into details, not even dared to boot that to test. So it will very
> > likely need some more work but I do not expect this to grow much.
> >
> > > I'll post a separate patch to do this refactoring.
> > > The original patch is fixing the UAF issue, so I don't want to mix it
> > > with refactoring. Please let me know if you see an issue with
> > > separating it that way.
> >
> > Well, I am not sure TBH. Look at diffstats. Your fix
> > 2 files changed, 63 insertions(+), 17 deletions(-)
> > the refactoring which should fix this and potentially others that might
> > be still lurking there (because mixing shared pointers and their internal
> > objects just begs for problems) is
> > 7 files changed, 63 insertions(+), 86 deletions(-)
> >
> > more files touched for sure but the net result is much more clear and a
> > much more code removed.
> > The overflow logic would make it bigger but I guess the existing scheme
> > needs it as well.
>
> Ok, I'll see how to slice it after it's complete and tested.
> Thanks for the input!

I posted the new patchset that includes:
1. refactoring of the code suggested by Michal:
https://lore.kernel.org/all/20220222054025.3412898-1-surenb@google.com
2. refcount overflow protection suggested by Michal:
https://lore.kernel.org/all/20220222054025.3412898-2-surenb@google.com
3. UAF fix (originally implemented by this patch) reimplemented after
the first two changes:
https://lore.kernel.org/all/20220222054025.3412898-3-surenb@google.com
Hopefully this sequence makes sense.
Thanks,
Suren.

>
> >
> > I would also claim that both approaches are really painful to review
> > because the existing model spreads into several areas and it is not
> > really clear you caught them all just by staring into the diff so both
> > will be rather painful to backport to older kernels. Fortunately this
> > would be only 5.17.
> > --
> > Michal Hocko
> > SUSE Labs