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@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
---
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
