| Message ID | 20220222054025.3412898-3-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] mm: refactor vm_area_struct::anon_vma_name usage code | expand |
On Mon, Feb 21, 2022 at 9:40 PM 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 raising the name refcount and stabilizing it. > > Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory") > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com > --- > changes in v3: > - Reapplied the fix after code refactoring, per Michal Hocko Hi Andrew, Since I needed to make some refactoring before adding this fix, in order to apply this new version to mmotm you would need to revert the previous version of this patch from your tree: 0cc16837d264 ("mm: fix use-after-free when anon vma name is used after vma is freed") and then apply the whole patchset (3 patches) after it is reviewed. Sorry for the inconvenience but I think this way the refactoring and the fix would be in the right order and with no overlap. The patchset applies cleanly to Linus' ToT and to mmotm after 0cc16837d264 is reverted. Thanks, Suren. > > mm/madvise.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/madvise.c b/mm/madvise.c > index a395884aeecb..00e8105430e9 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -140,6 +140,8 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, > /* > * 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, > @@ -1021,8 +1023,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > } > > anon_name = vma_anon_name(vma); > + anon_vma_name_get(anon_name); > error = madvise_update_vma(vma, prev, start, end, new_flags, > anon_name); > + anon_vma_name_put(anon_name); > > out: > /* > -- > 2.35.1.473.g83b2b277ed-goog >
On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 This seems to be stale because bare const char pointer is not passed in the call chain. In fact I am not even sure there is any actual UAF here after the rework. Could you be more specific in describing the scenario? > Fix this by raising the name refcount and stabilizing it. > > Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory") > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com > --- > changes in v3: > - Reapplied the fix after code refactoring, per Michal Hocko > > mm/madvise.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/madvise.c b/mm/madvise.c > index a395884aeecb..00e8105430e9 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -140,6 +140,8 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, > /* > * 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, > @@ -1021,8 +1023,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > } > > anon_name = vma_anon_name(vma); > + anon_vma_name_get(anon_name); > error = madvise_update_vma(vma, prev, start, end, new_flags, > anon_name); > + anon_vma_name_put(anon_name); > > out: > /* > -- > 2.35.1.473.g83b2b277ed-goog
On Tue, Feb 22, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote: > > On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 > > This seems to be stale because bare const char pointer is not passed in > the call chain. In fact I am not even sure there is any actual UAF here > after the rework. > Could you be more specific in describing the scenario? Yes, sorry, I need to update the part of the description talking about passing vma->anon_name->name directly. I think UAF is still there, it's just harder to reproduce (admittedly I could not reproduce it with the previous reproducer). The scenario would be when a vma with vma->anon_name->kref == 1 is being merged with another one and freed in the process: madvise_vma_behavior anon_name = vma_anon_name(vma) <-- does not increase refcount madvise_update_vma(anon_name) *prev = vma_merge <-- returns another vma __vma_adjust vm_area_free(vma) free_vma_anon_name anon_vma_name_put vma_anon_name_free <-- frees the vma->anon_name vma = *prev <-- original vma was freed replace_vma_anon_name(vma, >>anon_name<<) <-- UAF Does this make sense or did I miss something? Thanks, Suren. > > > Fix this by raising the name refcount and stabilizing it. > > > > Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory") > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com > > --- > > changes in v3: > > - Reapplied the fix after code refactoring, per Michal Hocko > > > > mm/madvise.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > index a395884aeecb..00e8105430e9 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -140,6 +140,8 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, > > /* > > * 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, > > @@ -1021,8 +1023,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > > } > > > > anon_name = vma_anon_name(vma); > > + anon_vma_name_get(anon_name); > > error = madvise_update_vma(vma, prev, start, end, new_flags, > > anon_name); > > + anon_vma_name_put(anon_name); > > > > out: > > /* > > -- > > 2.35.1.473.g83b2b277ed-goog > > -- > Michal Hocko > SUSE Labs
On Tue 22-02-22 07:43:40, Suren Baghdasaryan wrote: > On Tue, Feb 22, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote: > > > > On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 > > > > This seems to be stale because bare const char pointer is not passed in > > the call chain. In fact I am not even sure there is any actual UAF here > > after the rework. > > Could you be more specific in describing the scenario? > > Yes, sorry, I need to update the part of the description talking about > passing vma->anon_name->name directly. > I think UAF is still there, it's just harder to reproduce (admittedly > I could not reproduce it with the previous reproducer). The scenario > would be when a vma with vma->anon_name->kref == 1 is being merged > with another one and freed in the process: > > madvise_vma_behavior > anon_name = vma_anon_name(vma) <-- does not increase refcount > madvise_update_vma(anon_name) > *prev = vma_merge <-- returns another vma > __vma_adjust > vm_area_free(vma) > free_vma_anon_name > anon_vma_name_put > vma_anon_name_free <-- frees the vma->anon_name > vma = *prev <-- original vma was freed How come this is not a UAF in the first place? > replace_vma_anon_name(vma, >>anon_name<<) <-- UAF > > Does this make sense or did I miss something? Sorry for being dense but I still do not see it. If *prev has been freed then we already have a different UAF. Admittedly, I am not really fluent at vma_merge code path so I am not really sure your chain above is really possible. I will try to double check later.
On Wed, Feb 23, 2022 at 12:55 AM Michal Hocko <mhocko@suse.com> wrote: > > On Tue 22-02-22 07:43:40, Suren Baghdasaryan wrote: > > On Tue, Feb 22, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 > > > > > > This seems to be stale because bare const char pointer is not passed in > > > the call chain. In fact I am not even sure there is any actual UAF here > > > after the rework. > > > Could you be more specific in describing the scenario? > > > > Yes, sorry, I need to update the part of the description talking about > > passing vma->anon_name->name directly. > > I think UAF is still there, it's just harder to reproduce (admittedly > > I could not reproduce it with the previous reproducer). The scenario > > would be when a vma with vma->anon_name->kref == 1 is being merged > > with another one and freed in the process: > > > > madvise_vma_behavior > > anon_name = vma_anon_name(vma) <-- does not increase refcount > > madvise_update_vma(anon_name) > > *prev = vma_merge <-- returns another vma > > __vma_adjust > > vm_area_free(vma) > > free_vma_anon_name > > anon_vma_name_put > > vma_anon_name_free <-- frees the vma->anon_name > > vma = *prev <-- original vma was freed > > How come this is not a UAF in the first place? Sorry, I got you confused. The original vma that was passed as a parameter to vma_merge(vma) was freed and vma_merge() returns the area it was merged with: *prev = vma_merge(vma_a) <-- vma_a is merged with adjacent vma_b, vma_a is freed and vma_b is returned vma = *prev <-- "vma" now points to vma_b > > > replace_vma_anon_name(vma, >>anon_name<<) <-- UAF > > > > Does this make sense or did I miss something? > > Sorry for being dense but I still do not see it. If *prev has been freed > then we already have a different UAF. Admittedly, I am not really fluent > at vma_merge code path so I am not really sure your chain above is > really possible. I will try to double check later. > -- > Michal Hocko > SUSE Labs
On Wed 23-02-22 06:10:54, Suren Baghdasaryan wrote: > On Wed, Feb 23, 2022 at 12:55 AM Michal Hocko <mhocko@suse.com> wrote: > > > > On Tue 22-02-22 07:43:40, Suren Baghdasaryan wrote: > > > On Tue, Feb 22, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > > > On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 > > > > > > > > This seems to be stale because bare const char pointer is not passed in > > > > the call chain. In fact I am not even sure there is any actual UAF here > > > > after the rework. > > > > Could you be more specific in describing the scenario? > > > > > > Yes, sorry, I need to update the part of the description talking about > > > passing vma->anon_name->name directly. > > > I think UAF is still there, it's just harder to reproduce (admittedly > > > I could not reproduce it with the previous reproducer). The scenario > > > would be when a vma with vma->anon_name->kref == 1 is being merged > > > with another one and freed in the process: > > > > > > madvise_vma_behavior > > > anon_name = vma_anon_name(vma) <-- does not increase refcount > > > madvise_update_vma(anon_name) > > > *prev = vma_merge <-- returns another vma > > > __vma_adjust > > > vm_area_free(vma) > > > free_vma_anon_name > > > anon_vma_name_put > > > vma_anon_name_free <-- frees the vma->anon_name > > > vma = *prev <-- original vma was freed > > > > How come this is not a UAF in the first place? > > Sorry, I got you confused. The original vma that was passed as a > parameter to vma_merge(vma) was freed and vma_merge() returns the area > it was merged with: > > *prev = vma_merge(vma_a) <-- vma_a is merged with adjacent vma_b, > vma_a is freed and vma_b is returned > vma = *prev <-- "vma" now points to vma_b OK, I see the problem now. Essentially if we have two adjacent vmas which have the same name but wrapped by two anon_vma_name instances and we merge them together then the first one can be freed along with its anon_vma_name which is still used by replace_anon_vma_name. Thanks for the patience.
On Wed, Feb 23, 2022 at 7:29 AM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 23-02-22 06:10:54, Suren Baghdasaryan wrote: > > On Wed, Feb 23, 2022 at 12:55 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > On Tue 22-02-22 07:43:40, Suren Baghdasaryan wrote: > > > > On Tue, Feb 22, 2022 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > > > > > On Mon 21-02-22 21:40:25, Suren Baghdasaryan 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 > > > > > > > > > > This seems to be stale because bare const char pointer is not passed in > > > > > the call chain. In fact I am not even sure there is any actual UAF here > > > > > after the rework. > > > > > Could you be more specific in describing the scenario? > > > > > > > > Yes, sorry, I need to update the part of the description talking about > > > > passing vma->anon_name->name directly. > > > > I think UAF is still there, it's just harder to reproduce (admittedly > > > > I could not reproduce it with the previous reproducer). The scenario > > > > would be when a vma with vma->anon_name->kref == 1 is being merged > > > > with another one and freed in the process: > > > > > > > > madvise_vma_behavior > > > > anon_name = vma_anon_name(vma) <-- does not increase refcount > > > > madvise_update_vma(anon_name) > > > > *prev = vma_merge <-- returns another vma > > > > __vma_adjust > > > > vm_area_free(vma) > > > > free_vma_anon_name > > > > anon_vma_name_put > > > > vma_anon_name_free <-- frees the vma->anon_name > > > > vma = *prev <-- original vma was freed > > > > > > How come this is not a UAF in the first place? > > > > Sorry, I got you confused. The original vma that was passed as a > > parameter to vma_merge(vma) was freed and vma_merge() returns the area > > it was merged with: > > > > *prev = vma_merge(vma_a) <-- vma_a is merged with adjacent vma_b, > > vma_a is freed and vma_b is returned > > vma = *prev <-- "vma" now points to vma_b > > OK, I see the problem now. Essentially if we have two adjacent vmas > which have the same name but wrapped by two anon_vma_name instances > and we merge them together then the first one can be freed along with > its anon_vma_name which is still used by replace_anon_vma_name. > > Thanks for the patience. Thanks for reviewing! I posted the next version of the patchset at https://lore.kernel.org/linux-mm/20220223153613.835563-1-surenb@google.com/ > -- > Michal Hocko > SUSE Labs
diff --git a/mm/madvise.c b/mm/madvise.c index a395884aeecb..00e8105430e9 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -140,6 +140,8 @@ static int replace_vma_anon_name(struct vm_area_struct *vma, /* * 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, @@ -1021,8 +1023,10 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, } anon_name = vma_anon_name(vma); + anon_vma_name_get(anon_name); error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name); + anon_vma_name_put(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 raising the name refcount and stabilizing it. Fixes: 9a10064f5625 ("mm: add a field to store names for private anonymous memory") Signed-off-by: Suren Baghdasaryan <surenb@google.com> Reported-by: syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com --- changes in v3: - Reapplied the fix after code refactoring, per Michal Hocko mm/madvise.c | 4 ++++ 1 file changed, 4 insertions(+)