From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: KASAN: use-after-free Read in vhost_chr_write_iter Date: Tue, 22 May 2018 06:53:27 +0300 Message-ID: <20180522065147-mutt-send-email-mst@kernel.org> References: <20180517134544.GA20646@dragonet.kaist.ac.kr> <58419d62-3074-2e5a-8504-da1cdeb08280@redhat.com> <20180521173645-mutt-send-email-mst@kernel.org> <51cf8274-162f-384b-0ff7-47fbf15412f1@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: DaeRyong Jeong , kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, byoungyoung@purdue.edu, kt0755@gmail.com, bammanag@purdue.edu To: Jason Wang Return-path: Content-Disposition: inline In-Reply-To: <51cf8274-162f-384b-0ff7-47fbf15412f1@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Tue, May 22, 2018 at 11:50:29AM +0800, Jason Wang wrote: > > > On 2018年05月21日 22:42, Michael S. Tsirkin wrote: > > On Mon, May 21, 2018 at 10:38:10AM +0800, Jason Wang wrote: > > > On 2018年05月18日 17:24, Jason Wang wrote: > > > > On 2018年05月17日 21:45, DaeRyong Jeong wrote: > > > > > We report the crash: KASAN: use-after-free Read in vhost_chr_write_iter > > > > > > > > > > This crash has been found in v4.17-rc1 using RaceFuzzer (a modified > > > > > version of Syzkaller), which we describe more at the end of this > > > > > report. Our analysis shows that the race occurs when invoking two > > > > > syscalls concurrently, write$vnet and ioctl$VHOST_RESET_OWNER. > > > > > > > > > > > > > > > Analysis: > > > > > We think the concurrent execution of vhost_process_iotlb_msg() and > > > > > vhost_dev_cleanup() causes the crash. > > > > > Both of functions can run concurrently (please see call sequence below), > > > > > and possibly, there is a race on dev->iotlb. > > > > > If the switch occurs right after vhost_dev_cleanup() frees > > > > > dev->iotlb, vhost_process_iotlb_msg() still sees the non-null value > > > > > and it > > > > > keep executing without returning -EFAULT. Consequently, use-after-free > > > > > occures > > > > > > > > > > > > > > > Thread interleaving: > > > > > CPU0 (vhost_process_iotlb_msg)                CPU1 (vhost_dev_cleanup) > > > > > (In the case of both VHOST_IOTLB_UPDATE and > > > > > VHOST_IOTLB_INVALIDATE) > > > > > =====                            ===== > > > > >                             vhost_umem_clean(dev->iotlb); > > > > > if (!dev->iotlb) { > > > > >             ret = -EFAULT; > > > > >                 break; > > > > > } > > > > >                             dev->iotlb = NULL; > > > > > > > > > > > > > > > Call Sequence: > > > > > CPU0 > > > > > ===== > > > > > vhost_net_chr_write_iter > > > > >     vhost_chr_write_iter > > > > >         vhost_process_iotlb_msg > > > > > > > > > > CPU1 > > > > > ===== > > > > > vhost_net_ioctl > > > > >     vhost_net_reset_owner > > > > >         vhost_dev_reset_owner > > > > >             vhost_dev_cleanup > > > > Thanks a lot for the analysis. > > > > > > > > This could be addressed by simply protect it with dev mutex. > > > > > > > > Will post a patch. > > > > > > > Could you please help to test the attached patch? I've done some smoking > > > test. > > > > > > Thanks > > > >From 88328386f3f652e684ee33dc4cf63dcaed871aea Mon Sep 17 00:00:00 2001 > > > From: Jason Wang > > > Date: Fri, 18 May 2018 17:33:27 +0800 > > > Subject: [PATCH] vhost: synchronize IOTLB message with dev cleanup > > > > > > DaeRyong Jeong reports a race between vhost_dev_cleanup() and > > > vhost_process_iotlb_msg(): > > > > > > Thread interleaving: > > > CPU0 (vhost_process_iotlb_msg) CPU1 (vhost_dev_cleanup) > > > (In the case of both VHOST_IOTLB_UPDATE and > > > VHOST_IOTLB_INVALIDATE) > > > ===== ===== > > > vhost_umem_clean(dev->iotlb); > > > if (!dev->iotlb) { > > > ret = -EFAULT; > > > break; > > > } > > > dev->iotlb = NULL; > > > > > > The reason is we don't synchronize between them, fixing by protecting > > > vhost_process_iotlb_msg() with dev mutex. > > > > > > Reported-by: DaeRyong Jeong > > > Fixes: 6b1e6cc7855b0 ("vhost: new device IOTLB API") > > > Reported-by: DaeRyong Jeong > > Long terms we might want to move iotlb into vqs > > so that messages can be processed in parallel. > > Not sure how to do it yet. > > > > Then we probably need to extend IOTLB msg to have a queue idx. But I thinkit > was probably only help if we split tx/rx into separate processes. > > Thanks 3 mutex locks on each access isn't pretty even if done by a single process, but yes - might be more important for scsi. -- MST