From mboxrd@z Thu Jan 1 00:00:00 1970 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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D096BC433F5 for ; Thu, 24 Mar 2022 16:27:45 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AB0F6840BD; Thu, 24 Mar 2022 17:27:43 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="i/RLFl28"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5B6BE840CE; Thu, 24 Mar 2022 17:27:42 +0100 (CET) Received: from mail-qk1-x729.google.com (mail-qk1-x729.google.com [IPv6:2607:f8b0:4864:20::729]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 6A046830AF for ; Thu, 24 Mar 2022 17:27:39 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ascull@google.com Received: by mail-qk1-x729.google.com with SMTP id b189so3904493qkf.11 for ; Thu, 24 Mar 2022 09:27:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DjKJgoQEWeNziHkRSZuqJZw9Epr/xV6Gm2Rk5ALnZ2Q=; b=i/RLFl282h436g/OYnqQDOpEPG2+vyf0nDikJp8XHkZPhLL0e0hfCK2YeClvpse2qT VxaOmyAzVPYesgoslrjXlaMelJYC0CEqfzOcUkQ9Ruuwr2At4gaXbMmcLZ6rf+6FEOCe 9KN+j1UDzBMjd8Rev3SxdQtbl3goSQLRf8/9X6TKfyFpDCodwknEeiAcCM7r3CU+1cQv JxwcY6fRGQPKfTHB7GfUysTlvFaOG5OoCKZws52i/1Wyhws2HaDA0Ll8PCTnBtRFeRiV AyKe1xd4djlHnuu30PJO5p5IeENv/ER8TRnbPeucYrSCSFr4Y1L9QoKaYG5qPZC+ZMLk +tZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DjKJgoQEWeNziHkRSZuqJZw9Epr/xV6Gm2Rk5ALnZ2Q=; b=sFpuuprYeIdBh6EucN+uVfsKBdXM5hummZvwAxoL1kmj20OraoQqtBhJezHEOcSC0S iy2wCpujgc5WfS5QCEEpAA1rbs/wY5E2byJInqNRhss/Jz+p15uapjc5G8wCwjZDOxCg czivdOxUL80vzgSv0x3jGiLkhpPNzE2zGS2FCEgaAFoq1gMW4WdkCjN635JHuMQBMGZr CblSi0J3nbJstNNa1FAK7HfmRQuIgQ/YT+eI6NMr3wIE9LwfpWol4wXqt8OfKg0ETY0Y Nf3jMw4gOR2MLrzg2jUbUGs3hbdJT8DDwfkvWRmoxhUbhAWUXUHg53MupMMwxfecHaai Bryw== X-Gm-Message-State: AOAM532GDuiwVBeKILV1hE1OgZEZRHz2a/nbD5W2nRk7VNofoj0GaUC5 BGcschJHcLK8XmMdi8xWC+N1AudW8FYNn4k+zZZ/yA== X-Google-Smtp-Source: ABdhPJzcS3nzf4425jOTi0yEtymfAYY6PZoEyOi84FqnSbxQkFghNdVlvnc1bsX7seOxxlQSUoD6mXWRIBs6a27d76k= X-Received: by 2002:a37:68d4:0:b0:67b:113f:c10e with SMTP id d203-20020a3768d4000000b0067b113fc10emr3950484qkc.350.1648139258119; Thu, 24 Mar 2022 09:27:38 -0700 (PDT) MIME-Version: 1.0 References: <20220320114118.2237795-1-ascull@google.com> <20220320114118.2237795-6-ascull@google.com> In-Reply-To: From: Andrew Scull Date: Thu, 24 Mar 2022 16:27:27 +0000 Message-ID: Subject: Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds To: Bin Meng Cc: U-Boot Mailing List , Simon Glass , Alistair Delva , Keir Fraser , =?UTF-8?Q?Pierre=2DCl=C3=A9ment_Tosi?= Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Thu, 24 Mar 2022 at 15:24, Bin Meng wrote: > > On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull wrote: > > > > Ensure the virtio PCI capabilities are contained within the bounds of > > the device's configuration space. The expected size of the capability is > > passed when searching for the capability to enforce this check. > > > > Signed-off-by: Andrew Scull > > --- > > drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++---- > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 3403ff5cca..4b346be257 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq) > > * > > * @udev: the transport device > > * @cfg_type: the VIRTIO_PCI_CAP_* value we seek > > + * @cap_size: expected size of the capability > > * > > * Return: offset of the configuration structure > > */ > > -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type) > > +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type, > > + size_t cap_size) > > { > > int pos; > > int offset; > > u8 type, bar; > > > > + if (cap_size < sizeof(struct virtio_pci_cap)) > > + return 0; > > + > > + if (cap_size > PCI_CFG_SPACE_SIZE) > > + return 0; > > + > > The above 2 checks are not necessary as this helper is local to this > driver, and we know the callers do things correctly. The checks aren't absolutely necessary but they do help to document the constraints and catch any future problems. I'm not sure of the philosophy the u-boot applies but I like the idea of the safeguards being in place. > > for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR); > > pos > 0; > > pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) { > > + /* Ensure the capability is within bounds */ > > + if (PCI_CFG_SPACE_SIZE - cap_size < pos) > > + return 0; > > + > > offset = pos + offsetof(struct virtio_pci_cap, cfg_type); > > dm_pci_read_config8(udev, offset, &type); > > offset = pos + offsetof(struct virtio_pci_cap, bar); > > @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev) > > uc_priv->vendor = subvendor; > > > > /* Check for a common config: if not, use legacy mode (bar 0) */ > > - common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG); > > + common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG, > > + sizeof(struct virtio_pci_cap)); > > if (!common) { > > printf("(%s): leaving for legacy driver\n", udev->name); > > return -ENODEV; > > @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev) > > } > > > > /* If common is there, notify should be too */ > > - notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG); > > + notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG, > > + sizeof(struct virtio_pci_notify_cap)); > > if (!notify) { > > printf("(%s): missing capabilities %i/%i\n", udev->name, > > common, notify); > > @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev) > > * Device capability is only mandatory for devices that have > > * device-specific configuration. > > */ > > - device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG); > > + device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG, > > + sizeof(struct virtio_pci_cap)); > > if (device) { > > offset = device + offsetof(struct virtio_pci_cap, length); > > dm_pci_read_config32(udev, offset, &priv->device_len); > > -- > > Regards, > Bin