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 5F0AEC433EF for ; Fri, 25 Mar 2022 01:27:50 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A624683ADB; Fri, 25 Mar 2022 02:27:47 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.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=gmail.com header.i=@gmail.com header.b="FiARPm9a"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1C1A084117; Fri, 25 Mar 2022 02:27:46 +0100 (CET) Received: from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com [IPv6:2607:f8b0:4864:20::b33]) (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 40C7883971 for ; Fri, 25 Mar 2022 02:27:43 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=bmeng.cn@gmail.com Received: by mail-yb1-xb33.google.com with SMTP id y142so11522831ybe.11 for ; Thu, 24 Mar 2022 18:27:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0trgtgVGgoUo0+8vxjCaubsRkNYp+PbPZd1TYa4w4U4=; b=FiARPm9a8XfUot030rY7bTa6opP8wGGpDTlunjc0n9PFQGw3H8FueqfSFEpdLRiXaj SJCwjcqA6DWY3A43t52uazJjJGy1mAvDN3qYPt++85PJbYRkrJpiKFsK4JTuj/loEdce QIEld+iy+11VGQVfwOsMbbTxMUjI/8zz1pnTdabsaF3EaNa/06C3RaFlDpBDS9KzoYpA kaVb+noqpAW3X1xGtYBW0MerqkTqZ8k0b/Sj0mNnK7E6SMx1LyYWJuRrJy0qzTdq9AC9 bjBlBw1fqimaud0ZHShoZFYyq238+I0RHzGjcFdH/d+KnLdtaVFEsWLxAizbO5GfFN68 6bfQ== 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=0trgtgVGgoUo0+8vxjCaubsRkNYp+PbPZd1TYa4w4U4=; b=Y3yPe1pUeE+G5eTs/n0ibe61UJ9mDqwXXbv5+K7lXgmQ5/GiPIKzTogkT0qX9arGnf eKmZnv8oJqUP78UidhnT1n/z+QvtY4nXuKSRZD8KYP+2vDbUnyGdfOifbF5ctzebDkwC lC+r3YIiMzweQi95a6E0uoveyv8WFWE6xhhCtpBYQ9f+i3QgPS6eVHeH1xBwuuXQXNk8 02FU4zwCVbulvrma26e76OErBqa6wG9XPDDHospdhoUixJOcmS+gyclO0EZzsCNkhsQ4 ngbgp1JDA7c2wuvNFIz9N80UvslJGc5Nv3dTOVpbe/ubR257/+JGC4c/Xsk3tYpztndY 4Czw== X-Gm-Message-State: AOAM5334n7DUJSOruFGlV3sqvQ3+bopb+la8XO0+d34hWdoWBFV+KUPm AHrzZUuHhl/iIou91XC95k1WK4lccEdAaYqkym4= X-Google-Smtp-Source: ABdhPJzVnTyEaPNZBXZAKd5rDYEVSEWR4sKH/0RmDN2PR3YQTPUubXs3KLp+nbGBKV05rJRoDiM0hos04sK63e1OgXE= X-Received: by 2002:a25:8c86:0:b0:628:a042:9529 with SMTP id m6-20020a258c86000000b00628a0429529mr7655970ybl.231.1648171662117; Thu, 24 Mar 2022 18:27:42 -0700 (PDT) MIME-Version: 1.0 References: <20220320114118.2237795-1-ascull@google.com> <20220320114118.2237795-6-ascull@google.com> In-Reply-To: From: Bin Meng Date: Fri, 25 Mar 2022 09:27:30 +0800 Message-ID: Subject: Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds To: Andrew Scull 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 Fri, Mar 25, 2022 at 12:27 AM Andrew Scull wrote: > > 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. I would use assert() for such case. > > > > 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