From patchwork Wed May 3 10:54:36 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 13230037 X-Patchwork-Delegate: herbert@gondor.apana.org.au Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C6730C77B78 for ; Wed, 3 May 2023 10:55:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229771AbjECKzA (ORCPT ); Wed, 3 May 2023 06:55:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56862 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229569AbjECKy7 (ORCPT ); Wed, 3 May 2023 06:54:59 -0400 Received: from 167-179-156-38.a7b39c.syd.nbn.aussiebb.net (167-179-156-38.a7b39c.syd.nbn.aussiebb.net [167.179.156.38]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1988849DE; Wed, 3 May 2023 03:54:57 -0700 (PDT) Received: from loth.rohan.me.apana.org.au ([192.168.167.2]) by formenos.hmeau.com with smtp (Exim 4.94.2 #2 (Debian)) id 1puA83-004ibe-9X; Wed, 03 May 2023 18:54:37 +0800 Received: by loth.rohan.me.apana.org.au (sSMTP sendmail emulation); Wed, 03 May 2023 18:54:36 +0800 Date: Wed, 3 May 2023 18:54:36 +0800 From: Herbert Xu To: Dmitry Vyukov Cc: syzbot , davem@davemloft.net, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, olivia@selenic.com, syzkaller-bugs@googlegroups.com, Jason Wang , "Michael S. Tsirkin" , Laurent Vivier , Rusty Russell Subject: [PATCH] hwrng: virtio - Fix race on data_avail and actual data Message-ID: References: <00000000000050327205f9d993b2@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-crypto@vger.kernel.org On Fri, Apr 21, 2023 at 04:52:13PM +0200, Dmitry Vyukov wrote: > > Here this: > > size = min_t(unsigned int, size, vi->data_avail); > memcpy(buf, vi->data + vi->data_idx, size); > vi->data_idx += size; > vi->data_avail -= size; > > runs concurrently with: > > if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > return; > vi->data_idx = 0; > > I did not fully grasp how/where vi->data is populated, but it looks > like it can lead to use of uninit/stale random data, or even to out of > bounds access, say if vi->data_avail is already updated, but > vi->data_idx is not yet reset to 0. Then concurrent reading will read > not where it's supposed to read. Yes this is a real race. This bug appears to have been around forever. ---8<--- The virtio rng device kicks off a new entropy request whenever the data available reaches zero. When a new request occurs at the end of a read operation, that is, when the result of that request is only needed by the next reader, then there is a race between the writing of the new data and the next reader. This is because there is no synchronisation whatsoever between the writer and the reader. Fix this by writing data_avail with smp_store_release and reading it with smp_load_acquire when we first enter read. The subsequent reads are safe because they're either protected by the first load acquire, or by the completion mechanism. Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.") Signed-off-by: Herbert Xu Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com Reported-by: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index f7690e0f92ed..e41a84e6b4b5 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -4,6 +4,7 @@ * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation */ +#include #include #include #include @@ -37,13 +38,13 @@ struct virtrng_info { static void random_recv_done(struct virtqueue *vq) { struct virtrng_info *vi = vq->vdev->priv; + unsigned int len; /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */ - if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) + if (!virtqueue_get_buf(vi->vq, &len)) return; - vi->data_idx = 0; - + smp_store_release(&vi->data_avail, len); complete(&vi->have_data); } @@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) struct scatterlist sg; reinit_completion(&vi->have_data); - vi->data_avail = 0; vi->data_idx = 0; sg_init_one(&sg, vi->data, sizeof(vi->data)); @@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) read = 0; /* copy available data */ - if (vi->data_avail) { + if (smp_load_acquire(&vi->data_avail)) { chunk = copy_data(vi, buf, size); size -= chunk; read += chunk;