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 832D7C4332F for ; Fri, 25 Mar 2022 07:04:05 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5842684156; Fri, 25 Mar 2022 08:04:02 +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="kNr2NRwo"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id B7EA884167; Fri, 25 Mar 2022 08:04:00 +0100 (CET) Received: from mail-qv1-xf2a.google.com (mail-qv1-xf2a.google.com [IPv6:2607:f8b0:4864:20::f2a]) (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 D26F184155 for ; Fri, 25 Mar 2022 08:03:56 +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-qv1-xf2a.google.com with SMTP id ke15so5532654qvb.11 for ; Fri, 25 Mar 2022 00:03:56 -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=xYS8U+/V3RO3LpMit20tc4qqjJJpYPVXKVQNl11lfFI=; b=kNr2NRwoejHoY+QVm2Z02LEOA9ICrcD38BI7gGfYCrsl4LKT+gKvcsKwGOF3FZrB+V H3dp8ORtrvAwvejTGtDfLbI4OVd+ejPWFLmebccYivmhvIkJy23JWZZ62mQ3m1Zfo1K5 +nnoNFPggRpBkVTy3QsUsRuyS6VSouNTMgPRIYgeoB/30SzVCg0H1Yaf3RsTU4NpKY2A /+E6JlKCSV6EF58zGdF/82R8ZvTacdezOcbu90ba9Zld0HcS3lmwFif1xE4XA81Mzh5X yKOy+1mdPm167z0U6NBzRCE3/27uxOVJrHF2kdW5UzTzTls9/BeODYfdgldGwW9ikesS loJg== 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=xYS8U+/V3RO3LpMit20tc4qqjJJpYPVXKVQNl11lfFI=; b=J2yH6EYZxlPFGa/0BhV385f75eDt7LNyhXtmtGv6yBl+KgYXC6s7uLhtS1oITjdJ6q /dsFpToXR8UszDg/BE51ayd4HpPi8iywxpOhiadPfg/KuI+j/g83k2pww50uwsEXqSRb HBnBNL+wewIY0SCOA4SrcjdueEnK5HlnLUAtV2xV7A3r5k4S8nowYicp0x5m/SN55kWm DW5LQBhHGj/DYd21zxOj3Odk/Owv6ZiEsGp8RoI8IQudy9pWyIlDgFLC89VkkGqVq3nI pdK67CZl46nKqbCoLLdPsG8it0FnuP3+Ntt8jjKA8AGRrx3AdZyYWAQRpy2zgVwUOAGD 7z5g== X-Gm-Message-State: AOAM532e452VHWEGrBVOEVW/BF2dbY7F8vO3ckmgYxgtXPgv8pZDTiAE Qjy60GHi/2qot3iBxpWwr/HfhxPqWTFkWrsJWfn9pQ== X-Google-Smtp-Source: ABdhPJxw8xmIPTlNP4xneDv1guMEglwxFGnbwwvhoLE+t7wwJGL7Xx80AE2uLTpbnZrTN/vdakiRAj7DqxNk0UFxnYo= X-Received: by 2002:ad4:4d44:0:b0:440:f857:c9d with SMTP id m4-20020ad44d44000000b00440f8570c9dmr7306804qvm.81.1648191835425; Fri, 25 Mar 2022 00:03:55 -0700 (PDT) MIME-Version: 1.0 References: <20220320114118.2237795-1-ascull@google.com> <20220320114118.2237795-7-ascull@google.com> In-Reply-To: From: Andrew Scull Date: Fri, 25 Mar 2022 07:03:44 +0000 Message-ID: Subject: Re: [PATCH 06/11] virtio: pci: Read entire capability into memory 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 Fri, 25 Mar 2022 at 04:31, Bin Meng wrote: > > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull wrote: > > > > Read the virtio PCI capability out of the device configuration space to > > a struct rather than accessing fields directly from the configuration > > space as they are needed. This both makes access to the fields easier > > and avoids re-reading fields. > > > > Re-reading fields could result in time-of-check to time-of-use problems, > > should the value in the configuration space change. The range check of > > the `bar` field and the later call to `dm_pci_read_bar32()` is an > > example of where this could happen. > > I don't see the need to avoid the time-of-check to time-of-use > problems, as it can only happen with the PCI configuration access > capability, which U-Boot driver does not touch. > > Am I missing something? U-Boot doesn't touch the configuration space but the device could have, whether that be accidently or maliciously. Linux has taken similar precautions [1] to add more safety checks and I'll be looking to do the same in other parts of the u-boot virtio drivers. [1] -- https://lwn.net/Articles/865216 > > > > Signed-off-by: Andrew Scull > > --- > > drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++-------------- > > 1 file changed, 40 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 4b346be257..38a0da0a84 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -393,15 +393,16 @@ 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 > > + * @cap: capability read from the config space > > * > > * Return: offset of the configuration structure > > */ > > static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type, > > - size_t cap_size) > > + size_t cap_size, > > + struct virtio_pci_cap *cap) > > { > > int pos; > > int offset; > > - u8 type, bar; > > > > if (cap_size < sizeof(struct virtio_pci_cap)) > > return 0; > > @@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type, > > if (cap_size > PCI_CFG_SPACE_SIZE) > > return 0; > > > > + if (!cap) > > + return 0; > > + > > 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)) { > > @@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type, > > if (PCI_CFG_SPACE_SIZE - cap_size < pos) > > return 0; > > > > + offset = pos + offsetof(struct virtio_pci_cap, cap_vndr); > > + dm_pci_read_config8(udev, offset, &cap->cap_vndr); > > + offset = pos + offsetof(struct virtio_pci_cap, cap_next); > > + dm_pci_read_config8(udev, offset, &cap->cap_next); > > + offset = pos + offsetof(struct virtio_pci_cap, cap_len); > > + dm_pci_read_config8(udev, offset, &cap->cap_len); > > offset = pos + offsetof(struct virtio_pci_cap, cfg_type); > > - dm_pci_read_config8(udev, offset, &type); > > + dm_pci_read_config8(udev, offset, &cap->cfg_type); > > offset = pos + offsetof(struct virtio_pci_cap, bar); > > - dm_pci_read_config8(udev, offset, &bar); > > + dm_pci_read_config8(udev, offset, &cap->bar); > > + offset = pos + offsetof(struct virtio_pci_cap, offset); > > + dm_pci_read_config32(udev, offset, &cap->offset); > > + offset = pos + offsetof(struct virtio_pci_cap, length); > > + dm_pci_read_config32(udev, offset, &cap->length); > > > > /* Ignore structures with reserved BAR values */ > > - if (bar > 0x5) > > + if (cap->bar > 0x5) > > continue; > > > > - if (type == cfg_type) > > + if (cap->cfg_type == cfg_type) > > return pos; > > } > > > > @@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type, > > * virtio_pci_map_capability - map base address of the capability > > * > > * @udev: the transport device > > - * @off: offset of the configuration structure > > + * @cap: capability to map > > * > > * Return: base address of the capability > > */ > > -static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off) > > +static void __iomem *virtio_pci_map_capability(struct udevice *udev, > > + const struct virtio_pci_cap *cap) > > { > > - u8 bar; > > - u32 offset; > > ulong base; > > void __iomem *p; > > > > - if (!off) > > + if (!cap) > > return NULL; > > > > - offset = off + offsetof(struct virtio_pci_cap, bar); > > - dm_pci_read_config8(udev, offset, &bar); > > - offset = off + offsetof(struct virtio_pci_cap, offset); > > - dm_pci_read_config32(udev, offset, &offset); > > - > > /* > > * TODO: adding 64-bit BAR support > > * > > * Per spec, the BAR is permitted to be either 32-bit or 64-bit. > > * For simplicity, only read the BAR address as 32-bit. > > */ > > - base = dm_pci_read_bar32(udev, bar); > > - p = (void __iomem *)base + offset; > > + base = dm_pci_read_bar32(udev, cap->bar); > > + p = (void __iomem *)base + cap->offset; > > > > return p; > > } > > @@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev) > > u16 subvendor; > > u8 revision; > > int common, notify, device; > > - u32 common_length; > > + struct virtio_pci_cap common_cap, notify_cap, device_cap; > > int offset; > > > > /* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */ > > @@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev) > > > > /* Check for a common config: if not, use legacy mode (bar 0) */ > > common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG, > > - sizeof(struct virtio_pci_cap)); > > + sizeof(struct virtio_pci_cap), > > + &common_cap); > > if (!common) { > > printf("(%s): leaving for legacy driver\n", udev->name); > > return -ENODEV; > > } > > > > - offset = common + offsetof(struct virtio_pci_cap, length); > > - dm_pci_read_config32(udev, offset, &common_length); > > - if (common_length < sizeof(struct virtio_pci_common_cfg)) { > > + if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) { > > printf("(%s): virtio common config too small\n", udev->name); > > return -EINVAL; > > } > > > > /* If common is there, notify should be too */ > > notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG, > > - sizeof(struct virtio_pci_notify_cap)); > > + sizeof(struct virtio_pci_notify_cap), > > + ¬ify_cap); > > if (!notify) { > > printf("(%s): missing capabilities %i/%i\n", udev->name, > > common, notify); > > return -EINVAL; > > } > > > > - offset = notify + offsetof(struct virtio_pci_cap, length); > > - dm_pci_read_config32(udev, offset, &priv->notify_len); > > + priv->notify_len = notify_cap.length; > > > > /* > > * Device capability is only mandatory for devices that have > > * device-specific configuration. > > */ > > 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); > > - } > > + sizeof(struct virtio_pci_cap), > > + &device_cap); > > + if (device) > > + priv->device_len = device_cap.length; > > > > /* Map configuration structures */ > > - priv->common = virtio_pci_map_capability(udev, common); > > - priv->notify_base = virtio_pci_map_capability(udev, notify); > > - priv->device = virtio_pci_map_capability(udev, device); > > + priv->common = virtio_pci_map_capability(udev, &common_cap); > > + priv->notify_base = virtio_pci_map_capability(udev, ¬ify_cap); > > + priv->device = virtio_pci_map_capability(udev, &device_cap); > > debug("(%p): common @ %p, notify base @ %p, device @ %p\n", > > udev, priv->common, priv->notify_base, priv->device); > > > > -- > > Regards, > Bin