From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 437661EEE0 for ; Wed, 2 Oct 2024 15:36:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727883408; cv=none; b=D9Q9ZWKUvPkkw/qvvUYvSQfQmR5xvPwP86D0WVDAVqlHzEdI+WouF3mr56iE6d2HHy0VBj4WG/XLCAQ7jVoruXMCo8MSdCHxZ63/yfIcqkxm3vsjM16KMo7entQoOFuwIoBK6MvztSm0DpdtOHhCYtQUSWxyNWjvfUgz3vqDjjY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727883408; c=relaxed/simple; bh=WNDVuJVYRzoATG9Ou+1RPn3y5hF6zGTSgTzDLHMArws=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=hdwlAr0hEFGfK7foh47SHQKiqFPFuU4hndzr3jNR8b9MV+0i3rcjE6lqFjIHb/8Km1FPK9t7qkLBsGdFVfM+WMOr/u9hZIVFkkWMGtIIuWicr0OKqoqm2MnKleVzf3+z3rz/vLhrAOabWPpyTW+7n/XoX5kzxEU7Bfm3v154+CM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=CSMMpw4m; arc=none smtp.client-ip=209.85.210.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="CSMMpw4m" Received: by mail-pf1-f177.google.com with SMTP id d2e1a72fcca58-71971d2099cso5609805b3a.2 for ; Wed, 02 Oct 2024 08:36:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1727883406; x=1728488206; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=OTB+PF2DdHacG+b6eao+ymQdh7xOwx1kJuRK+tH/lb0=; b=CSMMpw4mbbBFYit+nO0NdbsABQNOpO6BsUlB4lJE3MXOL9dg4x5jfTwtfZDIGyOWhD 03ZWIan4ktPKhXZAqpBeSLrCwH/qMMGHl1+A8ksixCohV08bbhvKc+bwsPjsdHcPrDv6 79ssRFcD2pLssU1YIu9fvYtewf/7fK0yFRVERr0iouskQNCSI7PtUoKlPv14vySEmWHV 3OP/wyTo1xv+EeFbHpJEK6L0O3xNHnWziAju9B5fkrXw8/+VXpDj+cYTQsmJQ3qTtYS9 57G5iGpr22BJCb4aq6W/CznRiwmYjegUZrRdhW6TvLM/VM+qaDA4nEgJcArhM0jgZzIT 5rGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727883406; x=1728488206; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OTB+PF2DdHacG+b6eao+ymQdh7xOwx1kJuRK+tH/lb0=; b=gQR0ILse7rEyDbhpVRakDeE4mYFbUhNH2ktB1tHsUMbyqH+4L0WXn1CVtKPjaLuchg ea6DXOilliJlxGnRmIpc36rv7/eKCNDiVNCLzQ8v0qScZVn1i5DKI6lj4iET2ZFym3qz HIJ+H0AIQSCX/rISxiDlPcwI7VIt8/gjO5jCA9jXufMIcwj7CqXF3szgIMogQ9JPveT7 5oHGAJe17n2U4K9W9q1zb72Ya8QG29MLBzGaXEfUA2reGHoRF/17nJ4AKmC13DR4eiY0 ZRItSHlyUde34e94i45aKWwpQA5OUsiStQ/c7D9NKJ77yN78dmmc5JhgJjLe1nFTtX19 tYRg== X-Forwarded-Encrypted: i=1; AJvYcCXqUrNU5HbvT/soYGtEVCNBQvwpGSOQrUUcarkW/KP2FZZz0/XDF5ofE90EzVN9/hb81n7arRV4VgqXacs=@vger.kernel.org X-Gm-Message-State: AOJu0YwPy0s+LTwBDNZzsHp8PiucRrAk3F0VPOinUW+UYu82Qmb3/NEm rmYkTxoOlyaJVvNNWdi18L1rluCq3XA1a9evlajD/gCo62GKqLRNRaauUGDcmIU= X-Google-Smtp-Source: AGHT+IH/7GBO+FC7LZe14Gvhx3upKoEFUNAKdET9bmKw2fLWvE3OojbtfIet/HJEf1PECAomvcGvtQ== X-Received: by 2002:aa7:9990:0:b0:717:94d2:43c3 with SMTP id d2e1a72fcca58-71dc5d5bf6bmr4257868b3a.18.1727883406513; Wed, 02 Oct 2024 08:36:46 -0700 (PDT) Received: from p14s ([2604:3d09:148c:c800:80f9:2130:90df:197]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-71b26518787sm9919544b3a.124.2024.10.02.08.36.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Oct 2024 08:36:45 -0700 (PDT) Date: Wed, 2 Oct 2024 09:36:43 -0600 From: Mathieu Poirier To: Mark-PK Tsai =?utf-8?B?KOiUoeaym+WJmyk=?= Cc: "matthias.bgg@gmail.com" , "andersson@kernel.org" , AngeloGioacchino Del Regno , "linux-remoteproc@vger.kernel.org" , JY Ho =?utf-8?B?KOS9lemnv+W9pSk=?= , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mediatek@lists.infradead.org" , YJ Chiang =?utf-8?B?KOaxn+iLseadsCk=?= Subject: Re: [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio Message-ID: References: <20240920091659.3933-1-mark-pk.tsai@mediatek.com> <109aa022a017648be7b1c583ec21af74d8a23dc7.camel@mediatek.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <109aa022a017648be7b1c583ec21af74d8a23dc7.camel@mediatek.com> On Fri, Sep 27, 2024 at 02:51:38AM +0000, Mark-PK Tsai (蔡沛剛) wrote: > Hi, > > Could someone help to review it or provide suggestions? > Any comments are welcome. > > This patch is intended to allow the rproc driver to handle the > following use-after-free issue: > > > ### UAF log What does "UAF" means? > [ 337.540275][ T470] virtqueue_add.llvm.10153462284424984632+0xb48/0 > xcc4 > [ 337.546969][ T470] virtqueue_add_inbuf+0x44/0x6c > <--- vqs are freed in vring_del_virtqueue > [ 337.551755][ T470] rpmsg_recv_done+0x1fc/0x2c4 [virtio_rpmsg_bus] > [ 337.558023][ T470] vring_interrupt+0xa0/0xe0 > [ 337.562462][ T470] rproc_vq_interrupt+0x34/0x48 > [ 337.567160][ T470] handle_event+0x28/0x48 [mtk_pqu_rproc] > [ 337.572742][ T470] irq_thread_fn+0x4c/0xcc > [ 337.577009][ T470] irq_thread+0x1d0/0x360 > [ 337.581189][ T470] kthread+0x168/0x1cc > [ 337.585107][ T470] ret_from_fork+0x10/0x20 > > ### stack trace of obj free > [ 339.253063][ T470] die_helper: <- > vring_del_virtqueue+0x16c/0x198 > [ 339.259757][ T470] die_helper: <- kfree+0x274/0x35c > [ 339.265236][ T470] die_helper: <- > vring_del_virtqueue+0x16c/0x198 > [ 339.271929][ T470] die_helper: <- > rproc_virtio_del_vqs+0x3c/0x58 > [ 339.278535][ T470] die_helper: <- rpmsg_remove+0x8c/0x12c > [virtio_rpmsg_bus] > [ 339.286189][ T470] die_helper: <- > virtio_dev_remove+0x64/0x174 > [ 339.292622][ T470] die_helper: <- > device_release_driver_internal+0x1a8/0x32c > [ 339.300270][ T470] die_helper: <- > bus_remove_device+0x130/0x160 > [ 339.306789][ T470] die_helper: <- device_del+0x224/0x518 > [ 339.312702][ T470] die_helper: <- device_unregister+0x20/0x3c > [ 339.319047][ T470] die_helper: <- > rproc_remove_virtio_dev+0x20/0x44 > [ 339.325913][ T470] die_helper: <- > device_for_each_child+0x84/0x100 > [ 339.332696][ T470] die_helper: <- > rproc_vdev_do_stop+0x30/0x5c > [ 339.339130][ T470] die_helper: <- rproc_stop+0xcc/0x200 > None of the above is showing me where this use-after-free happens. > On Fri, 2024-09-20 at 17:16 +0800, Mark-PK Tsai wrote: > > Add synchornize_cbs to rproc_virtio_config_ops and a > > synchronize_vqs callback to the rproc_ops to ensure vqs' > > state changes are visible in vring_interrupts when the vq is > > broken or removed. > > > > And when VIRTIO_HARDEN_NOTIFICATION is not set, call > > rproc_virtio_synchronize_cbs directly in __rproc_virtio_del_vqs > > before virtqueue is free to ensure that rproc_vq_interrupt is > > aware of the virtqueue removal. > > > > The synchronized_vqs is expected to be implemented in rproc > > driver to ensure that all previous vring_interrupts are handled > > before the vqs are marked as broken or removed. > > > > Signed-off-by: Mark-PK Tsai > > --- > > drivers/remoteproc/remoteproc_virtio.c | 12 ++++++++++++ > > include/linux/remoteproc.h | 4 ++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/drivers/remoteproc/remoteproc_virtio.c > > b/drivers/remoteproc/remoteproc_virtio.c > > index d3f39009b28e..e18258b69851 100644 > > --- a/drivers/remoteproc/remoteproc_virtio.c > > +++ b/drivers/remoteproc/remoteproc_virtio.c > > @@ -74,6 +74,14 @@ static bool rproc_virtio_notify(struct virtqueue > > *vq) > > return true; > > } > > > > +static void rproc_virtio_synchronize_cbs(struct virtio_device *vdev) > > +{ > > + struct rproc *rproc = vdev_to_rproc(vdev); > > + > > + if (rproc->ops->synchronize_vqs) > > + rproc->ops->synchronize_vqs(rproc); > > +} > > + > > /** > > * rproc_vq_interrupt() - tell remoteproc that a virtqueue is > > interrupted > > * @rproc: handle to the remote processor > > @@ -171,6 +179,9 @@ static void __rproc_virtio_del_vqs(struct > > virtio_device *vdev) > > list_for_each_entry_safe(vq, n, &vdev->vqs, list) { > > rvring = vq->priv; > > rvring->vq = NULL; > > +#ifndef CONFIG_VIRTIO_HARDEN_NOTIFICATION > > + rproc_virtio_synchronize_cbs(vdev); > > +#endif > > vring_del_virtqueue(vq); > > } > > } > > @@ -334,6 +345,7 @@ static const struct virtio_config_ops > > rproc_virtio_config_ops = { > > .get_status = rproc_virtio_get_status, > > .get = rproc_virtio_get, > > .set = rproc_virtio_set, > > + .synchronize_cbs = rproc_virtio_synchronize_cbs, Where and when is this called? This patch is very confusing and doesn't have a user. As such it is impossible for me to understand what is going on. Thanks, Mathieu > > }; > > > > /* > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > index b4795698d8c2..73901678ac7d 100644 > > --- a/include/linux/remoteproc.h > > +++ b/include/linux/remoteproc.h > > @@ -381,6 +381,9 @@ enum rsc_handling_status { > > * @panic: optional callback to react to system panic, core will > > delay > > * panic at least the returned number of milliseconds > > * @coredump: collect firmware dump after the subsystem is > > shutdown > > + * @synchronize_vqs: optional callback to guarantee all memory > > operations > > + * on the virtqueue before it are visible to the > > + * rproc_vq_interrupt(). > > */ > > struct rproc_ops { > > int (*prepare)(struct rproc *rproc); > > @@ -403,6 +406,7 @@ struct rproc_ops { > > u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware > > *fw); > > unsigned long (*panic)(struct rproc *rproc); > > void (*coredump)(struct rproc *rproc); > > + void (*synchronize_vqs)(struct rproc *rproc); > > }; > > > > /** >