. .
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.
Thanks,
Suren.
>
> >
