On Wed, Feb 9, 2022 at 7:48 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> . .
>
> On Wed, Feb 9, 2022 at 5:02 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
> >
> > On Wed, Feb 9, 2022 at 4:33 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 9 Feb 2022 16:18:01 -0800 Suren Baghdasaryan <surenb@xxxxxxxxxx> 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@xxxxxxxxxx/
Thanks!
> Thanks,
> Suren.
>
> >
> > >
