| Message ID | 20220210043215.42794-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] mm: fix use-after-free when anon vma name is used after vma is freed | expand |
On Wed 09-02-22 20:32:15, 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 > > Fix this by raising the name refcount and stabilizing it. Introduce > vma_anon_name_{get/put} API for this purpose. What is the reason that madvise_update_vma uses the naked name rather than the encapsulated anon_vma_name? This really just begs for problems.
On Wed, Feb 09, 2022 at 08:32:15PM -0800, Suren Baghdasaryan wrote: > +void vma_anon_name_put(struct anon_vma_name *anon_name) > +{ > + kref_put(&anon_name->kref, vma_anon_name_free); > +} To agree with Michal, make this: if (anon_name) kref_put(&anon_name->kref, vma_anon_name_free); > > - error = madvise_update_vma(vma, prev, start, end, new_flags, > - vma_anon_name(vma)); > + anon_name = vma_anon_name_get(vma); > + if (anon_name) { > + error = madvise_update_vma(vma, prev, start, end, new_flags, > + anon_name->name); > + vma_anon_name_put(anon_name); > + } else { > + error = madvise_update_vma(vma, prev, start, end, new_flags, > + NULL); > + } And then this becomes: anon_name = vma_anon_name_get(vma); error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name); vma_anon_name_put(anon_name);
On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team <kernel-team@android.com> wrote: > > On Wed 09-02-22 20:32:15, 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 > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > vma_anon_name_{get/put} API for this purpose. > > What is the reason that madvise_update_vma uses the naked name rather > than the encapsulated anon_vma_name? This really just begs for problems. The reason for that is the second place it's being used from the prctl syscall: prctl_set_vma madvise_set_anon_name madvise_vma_anon_name madvise_update_vma In that case the name parameter is not part of any anon_vma_name struct and therefore is stable. I can add a comment to madvise_update_vma indicating that the name parameter has to be stable if that helps. > -- > Michal Hocko > SUSE Labs > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Thu, Feb 10, 2022 at 5:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Feb 09, 2022 at 08:32:15PM -0800, Suren Baghdasaryan wrote: > > +void vma_anon_name_put(struct anon_vma_name *anon_name) > > +{ > > + kref_put(&anon_name->kref, vma_anon_name_free); > > +} > > To agree with Michal, make this: > > if (anon_name) > kref_put(&anon_name->kref, vma_anon_name_free); Ack. > > > > > - error = madvise_update_vma(vma, prev, start, end, new_flags, > > - vma_anon_name(vma)); > > + anon_name = vma_anon_name_get(vma); > > + if (anon_name) { > > + error = madvise_update_vma(vma, prev, start, end, new_flags, > > + anon_name->name); > > + vma_anon_name_put(anon_name); > > + } else { > > + error = madvise_update_vma(vma, prev, start, end, new_flags, > > + NULL); > > + } > > And then this becomes: > > anon_name = vma_anon_name_get(vma); > error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name); > vma_anon_name_put(anon_name); As I indicated in the other reply, there is another madvise_update_vma user which has only the name string, not the anon_vma_name struct. So this can become: anon_name = vma_anon_name_get(vma); error = madvise_update_vma(vma, prev, start, end, new_flags, anon_name ? anon_name->name : NULL); vma_anon_name_put(anon_name);
On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote: > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team > <kernel-team@android.com> wrote: > > > > On Wed 09-02-22 20:32:15, 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 > > > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > > vma_anon_name_{get/put} API for this purpose. > > > > What is the reason that madvise_update_vma uses the naked name rather > > than the encapsulated anon_vma_name? This really just begs for problems. > > The reason for that is the second place it's being used from the prctl syscall: > > prctl_set_vma > madvise_set_anon_name > madvise_vma_anon_name > madvise_update_vma > > In that case the name parameter is not part of any anon_vma_name > struct and therefore is stable. I can add a comment to > madvise_update_vma indicating that the name parameter has to be stable > if that helps. Seems to me it'd simplify things if replace_vma_anon_name() and madvise_vma_anon_name() took a struct anon_vma_name instead of a bare char *. You could construct it in madvise_set_anon_name().
On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote: > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team > > <kernel-team@android.com> wrote: > > > > > > On Wed 09-02-22 20:32:15, 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 > > > > > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > > > vma_anon_name_{get/put} API for this purpose. > > > > > > What is the reason that madvise_update_vma uses the naked name rather > > > than the encapsulated anon_vma_name? This really just begs for problems. > > > > The reason for that is the second place it's being used from the prctl syscall: > > > > prctl_set_vma > > madvise_set_anon_name > > madvise_vma_anon_name > > madvise_update_vma > > > > In that case the name parameter is not part of any anon_vma_name > > struct and therefore is stable. I can add a comment to > > madvise_update_vma indicating that the name parameter has to be stable > > if that helps. > > Seems to me it'd simplify things if replace_vma_anon_name() and > madvise_vma_anon_name() took a struct anon_vma_name instead of > a bare char *. You could construct it in madvise_set_anon_name(). Ok, this can be done. However I don't think changing replace_vma_anon_name() to accept a struct anon_vma_name would be a good idea. Reader might think that the object being passed will become the vma->anon_name of the vma, while in reality that's not the case. Keeping it a char* makes it obvious that the function will construct a new anon_vma_name struct. I'll post a v3 shortly implementing these suggestions. Thanks for the review folks!
On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote: > On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote: > > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team > > > <kernel-team@android.com> wrote: > > > > > > > > On Wed 09-02-22 20:32:15, 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 > > > > > > > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > > > > vma_anon_name_{get/put} API for this purpose. > > > > > > > > What is the reason that madvise_update_vma uses the naked name rather > > > > than the encapsulated anon_vma_name? This really just begs for problems. > > > > > > The reason for that is the second place it's being used from the prctl syscall: > > > > > > prctl_set_vma > > > madvise_set_anon_name > > > madvise_vma_anon_name > > > madvise_update_vma > > > > > > In that case the name parameter is not part of any anon_vma_name > > > struct and therefore is stable. I can add a comment to > > > madvise_update_vma indicating that the name parameter has to be stable > > > if that helps. > > > > Seems to me it'd simplify things if replace_vma_anon_name() and > > madvise_vma_anon_name() took a struct anon_vma_name instead of > > a bare char *. You could construct it in madvise_set_anon_name(). > > Ok, this can be done. However I don't think changing > replace_vma_anon_name() to accept a struct anon_vma_name would be a > good idea. Reader might think that the object being passed will become > the vma->anon_name of the vma, while in reality that's not the case. Why woud we not want that to be the case? It's a refcounted name. I don't see why it shouldn't be shared between multiple VMAs that have the same name?
On Thu, Feb 10, 2022 at 11:35 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote: > > On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote: > > > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team > > > > <kernel-team@android.com> wrote: > > > > > > > > > > On Wed 09-02-22 20:32:15, 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 > > > > > > > > > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > > > > > vma_anon_name_{get/put} API for this purpose. > > > > > > > > > > What is the reason that madvise_update_vma uses the naked name rather > > > > > than the encapsulated anon_vma_name? This really just begs for problems. > > > > > > > > The reason for that is the second place it's being used from the prctl syscall: > > > > > > > > prctl_set_vma > > > > madvise_set_anon_name > > > > madvise_vma_anon_name > > > > madvise_update_vma > > > > > > > > In that case the name parameter is not part of any anon_vma_name > > > > struct and therefore is stable. I can add a comment to > > > > madvise_update_vma indicating that the name parameter has to be stable > > > > if that helps. > > > > > > Seems to me it'd simplify things if replace_vma_anon_name() and > > > madvise_vma_anon_name() took a struct anon_vma_name instead of > > > a bare char *. You could construct it in madvise_set_anon_name(). > > > > Ok, this can be done. However I don't think changing > > replace_vma_anon_name() to accept a struct anon_vma_name would be a > > good idea. Reader might think that the object being passed will become > > the vma->anon_name of the vma, while in reality that's not the case. > > Why woud we not want that to be the case? It's a refcounted name. > I don't see why it shouldn't be shared between multiple VMAs that > have the same name? You are right. After I reworked the code it became apparent that replace_vma_anon_name() should use anon_vma_name. I have made that change and am testing it now. Hopefully no new surprises pop up. Thanks, Suren. >
On Thu, Feb 10, 2022 at 11:52 AM Suren Baghdasaryan <surenb@google.com> wrote: > > On Thu, Feb 10, 2022 at 11:35 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Thu, Feb 10, 2022 at 08:00:15AM -0800, Suren Baghdasaryan wrote: > > > On Thu, Feb 10, 2022 at 7:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Thu, Feb 10, 2022 at 07:18:24AM -0800, Suren Baghdasaryan wrote: > > > > > On Thu, Feb 10, 2022 at 4:40 AM 'Michal Hocko' via kernel-team > > > > > <kernel-team@android.com> wrote: > > > > > > > > > > > > On Wed 09-02-22 20:32:15, 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 > > > > > > > > > > > > > > Fix this by raising the name refcount and stabilizing it. Introduce > > > > > > > vma_anon_name_{get/put} API for this purpose. > > > > > > > > > > > > What is the reason that madvise_update_vma uses the naked name rather > > > > > > than the encapsulated anon_vma_name? This really just begs for problems. > > > > > > > > > > The reason for that is the second place it's being used from the prctl syscall: > > > > > > > > > > prctl_set_vma > > > > > madvise_set_anon_name > > > > > madvise_vma_anon_name > > > > > madvise_update_vma > > > > > > > > > > In that case the name parameter is not part of any anon_vma_name > > > > > struct and therefore is stable. I can add a comment to > > > > > madvise_update_vma indicating that the name parameter has to be stable > > > > > if that helps. > > > > > > > > Seems to me it'd simplify things if replace_vma_anon_name() and > > > > madvise_vma_anon_name() took a struct anon_vma_name instead of > > > > a bare char *. You could construct it in madvise_set_anon_name(). > > > > > > Ok, this can be done. However I don't think changing > > > replace_vma_anon_name() to accept a struct anon_vma_name would be a > > > good idea. Reader might think that the object being passed will become > > > the vma->anon_name of the vma, while in reality that's not the case. > > > > Why woud we not want that to be the case? It's a refcounted name. > > I don't see why it shouldn't be shared between multiple VMAs that > > have the same name? > > You are right. After I reworked the code it became apparent that > replace_vma_anon_name() should use anon_vma_name. I have made that > change and am testing it now. Hopefully no new surprises pop up. v3 is posted at https://lore.kernel.org/all/20220211013032.623763-1-surenb@google.com/ > Thanks, > Suren. > > >
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..9cf069e574c0 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -103,6 +103,22 @@ 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) +{ + 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) { @@ -976,6 +992,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 +1057,15 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, break; } - error = madvise_update_vma(vma, prev, start, end, new_flags, - vma_anon_name(vma)); + anon_name = vma_anon_name_get(vma); + if (anon_name) { + error = madvise_update_vma(vma, prev, start, end, new_flags, + anon_name->name); + vma_anon_name_put(anon_name); + } else { + error = madvise_update_vma(vma, prev, start, end, new_flags, + NULL); + } 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. 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@syzkaller.appspotmail.com Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- changes in v2: - Replaces name copying with refcounting and added vma_anon_name_{get/put} API, per Andrew Morton include/linux/mm_inline.h | 13 +++++++++++++ mm/madvise.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-)