From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-f200.google.com (mail-oi1-f200.google.com [209.85.167.200]) by kanga.kvack.org (Postfix) with ESMTP id 0B9878E0001 for ; Tue, 18 Dec 2018 00:28:12 -0500 (EST) Received: by mail-oi1-f200.google.com with SMTP id w80so748366oiw.19 for ; Mon, 17 Dec 2018 21:28:12 -0800 (PST) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id s49sor3025309otb.113.2018.12.17.21.28.10 for (Google Transport Security); Mon, 17 Dec 2018 21:28:11 -0800 (PST) MIME-Version: 1.0 References: <1545104531-30658-1-git-send-email-gchen.guomin@gmail.com> <20181217233821-mutt-send-email-mst@kernel.org> In-Reply-To: <20181217233821-mutt-send-email-mst@kernel.org> From: gchen chen Date: Tue, 18 Dec 2018 13:27:59 +0800 Message-ID: Subject: Re: [PATCH] Export mm_update_next_owner function for unuse_mm. Content-Type: multipart/alternative; boundary="000000000000c86fde057d452939" Sender: owner-linux-mm@kvack.org List-ID: To: "Michael S. Tsirkin" Cc: Jason Wang , Christoph Hellwig , Andrew Morton , "Luis R. Rodriguez" , gchen , "Eric W. Biederman" , Dominik Brodowski , Arnd Bergmann , linux-kernel@vger.kernel.org, linux-mm@kvack.org --000000000000c86fde057d452939 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sorry,It does not need to be exported. I have modified this patch and re-commit it. Thank you very much for reminding. thanks Michael S. Tsirkin =E4=BA=8E2018=E5=B9=B412=E6=9C=8818=E6= =97=A5=E5=91=A8=E4=BA=8C =E4=B8=8B=E5=8D=8812:38=E5=86=99=E9=81=93=EF=BC=9A On Tue, Dec 18, 2018 at 11:42:11AM +0800, gchen.guomin@gmail.com wrote: > From: guomin chen > > When mm->owner is modified by exit_mm, if the new owner directly calls > unuse_mm to exit, it will cause Use-After-Free. Due to the unuse_mm() > directly sets tsk->mm=3DNULL. > > Under normal circumstances,When do_exit exits, mm->owner will > be updated on exit_mm(). but when the kernel process calls > unuse_mm() and then exits,mm->owner cannot be updated. And it > will point to a task that has been released. > > The current issue flow is as follows: > Process C Process A Process B > qemu-system-x86_64: kernel:vhost_net kernel: vhost_net > open /dev/vhost-net > VHOST_SET_OWNER create kthread vhost-%d create kthread vhost-%d > network init use_mm() use_mm() > ... ... > Abnormal exited > ... > do_exit > exit_mm() > update mm->owner to A > exit_files() > close_files() > kthread_should_stop() unuse_mm() > Stop Process A tsk->mm=3DNULL > do_exit() > can't update owner > A exit completed vhost-%d rcv first package > vhost-%d build rcv buffer for vq > page fault > access mm & mm->owner > NOW,mm->owner still pointer A > kernel UAF > stop Process B > > Although I am having this issue on vhost_net,But it affects all users of > unuse_mm. > > Cc: "Eric W. Biederman" > Cc: Andrew Morton > Cc: "Luis R. Rodriguez" > Cc: Dominik Brodowski > Cc: Arnd Bergmann > Cc: linux-kernel@vger.kernel.org > Cc: linux-mm@kvack.org > Cc: "Michael S. Tsirkin" > Cc: Jason Wang > Cc: Christoph Hellwig > Signed-off-by: guomin chen > --- > kernel/exit.c | 1 + > mm/mmu_context.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/kernel/exit.c b/kernel/exit.c > index 0e21e6d..9e046dd 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -486,6 +486,7 @@ void mm_update_next_owner(struct mm_struct *mm) > task_unlock(c); > put_task_struct(c); > } > +EXPORT_SYMBOL(mm_update_next_owner); > #endif /* CONFIG_MEMCG */ > > /* So why export it? Is that still needed? > diff --git a/mm/mmu_context.c b/mm/mmu_context.c > index 3e612ae..9eb81aa 100644 > --- a/mm/mmu_context.c > +++ b/mm/mmu_context.c > @@ -60,5 +60,6 @@ void unuse_mm(struct mm_struct *mm) > /* active_mm is still 'mm' */ > enter_lazy_tlb(mm, tsk); > task_unlock(tsk); > + mm_update_next_owner(mm); > } > EXPORT_SYMBOL_GPL(unuse_mm); > -- > 1.8.3.1 --000000000000c86fde057d452939 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Sorry,It does not need to be exported.
I have modified this patch and re-commit it.
Thank you very much for reminding.

thanks

Michael S. Tsirkin = <mst@redhat.com> =E4=BA=8E2018= =E5=B9=B412=E6=9C=8818=E6=97=A5=E5=91=A8=E4=BA=8C =E4=B8=8B=E5=8D=8812:38= =E5=86=99=E9=81=93=EF=BC=9A
On Tue, De= c 18, 2018 at 11:42:11AM +0800, gchen.guomin@gmail.com wrote:
> From: guomin chen <gchen.guomin@gmail.com>
>
> When mm->owner is modified by exit_mm, if the new owner directly ca= lls
> unuse_mm to exit, it will cause Use-After-Free. Due to the unuse_mm()<= br> > directly sets tsk->mm=3DNULL.
>
>=C2=A0 Under normal circumstances,When do_exit exits, mm->owner will=
>=C2=A0 be updated on exit_mm(). but when the kernel process calls
>=C2=A0 unuse_mm() and then exits,mm->owner cannot be updated. And it=
>=C2=A0 will point to a task that has been released.
>
> The current issue flow is as follows:
> Process C=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Process A=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Process B
> qemu-system-x86_64:=C2=A0 =C2=A0 =C2=A0kernel:vhost_net=C2=A0 kernel: = vhost_net
> open /dev/vhost-net
>=C2=A0 =C2=A0VHOST_SET_OWNER=C2=A0 =C2=A0create kthread vhost-%d=C2=A0 = create kthread vhost-%d
>=C2=A0 =C2=A0network init=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0use_m= m()=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 use_mm()
>=C2=A0 =C2=A0 ...=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0...
>=C2=A0 =C2=A0 Abnormal exited
>=C2=A0 =C2=A0 ...
>=C2=A0 =C2=A0do_exit
>=C2=A0 =C2=A0exit_mm()
>=C2=A0 =C2=A0update mm->owner to A
>=C2=A0 =C2=A0exit_files()
>=C2=A0 =C2=A0 close_files()
>=C2=A0 =C2=A0 kthread_should_stop() unuse_mm()
>=C2=A0 =C2=A0 =C2=A0Stop Process A=C2=A0 =C2=A0 =C2=A0 =C2=A0tsk->mm= =3DNULL
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 do_exit()
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 can't update owner
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 A exit completed=C2=A0 vhost-%d=C2=A0 rcv first packag= e
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 vhost-%d build rcv buffer for vq
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 page fault
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 access mm & mm->owner
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 NOW,mm->owner still pointer A
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 kernel UAF
>=C2=A0 =C2=A0 =C2=A0stop Process B
>
> Although I am having this issue on vhost_net,But it affects all users = of
> unuse_mm.
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: = linux-kernel@vger.kernel.org
> Cc: linux-mm@k= vack.org
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: guomin chen <gchen.guomin@gmail.com>
> ---
>=C2=A0 kernel/exit.c=C2=A0 =C2=A0 | 1 +
>=C2=A0 mm/mmu_context.c | 1 +
>=C2=A0 2 files changed, 2 insertions(+)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 0e21e6d..9e046dd 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -486,6 +486,7 @@ void mm_update_next_owner(struct mm_struct *mm) >=C2=A0 =C2=A0 =C2=A0 =C2=A0task_unlock(c);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0put_task_struct(c);
>=C2=A0 }
> +EXPORT_SYMBOL(mm_update_next_owner);
>=C2=A0 #endif /* CONFIG_MEMCG */
>=C2=A0
>=C2=A0 /*

So why export it? Is that still needed?

> diff --git a/mm/mmu_context.c b/mm/mmu_context.c
> index 3e612ae..9eb81aa 100644
> --- a/mm/mmu_context.c
> +++ b/mm/mmu_context.c
> @@ -60,5 +60,6 @@ void unuse_mm(struct mm_struct *mm)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* active_mm is still 'mm' */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0enter_lazy_tlb(mm, tsk);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0task_unlock(tsk);
> +=C2=A0 =C2=A0 =C2=A0mm_update_next_owner(mm);
>=C2=A0 }
>=C2=A0 EXPORT_SYMBOL_GPL(unuse_mm);
> --
> 1.8.3.1
--000000000000c86fde057d452939--