| Message ID | 20220210001801.15413-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mm: Fix UAF when anon vma name is used after vma is freed | expand |
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?
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 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. > > >
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. > > > > > >
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: /*
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(-)