* [PATCH vhost v1 0/2] strictly check the acccess to the common cfg @ 2023-10-08 7:45 Xuan Zhuo 2023-10-08 7:45 ` [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset Xuan Zhuo 2023-10-08 7:45 ` [PATCH vhost v1 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 0 siblings, 2 replies; 4+ messages in thread From: Xuan Zhuo @ 2023-10-08 7:45 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin 1. fix the length of the pci_iomap_range() to the common cfg 2. add size check for the vq-reset 3. add build size check to the new common cfg items Xuan Zhuo (2): virtio_pci: fix the common map size and add check for vq-reset virtio_pci: add build offset check for the new common cfg items drivers/virtio/virtio_pci_modern_dev.c | 31 ++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) -- 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset 2023-10-08 7:45 [PATCH vhost v1 0/2] strictly check the acccess to the common cfg Xuan Zhuo @ 2023-10-08 7:45 ` Xuan Zhuo 2023-10-08 8:48 ` Michael S. Tsirkin 2023-10-08 7:45 ` [PATCH vhost v1 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 1 sibling, 1 reply; 4+ messages in thread From: Xuan Zhuo @ 2023-10-08 7:45 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin Now, the function vp_modern_map_capability() takes the size parameter, which corresponds to the size of virtio_pci_common_cfg. As a result, this indicates the size of memory area to map. However, if the _F_RING_RESET is negotiated, the extra items will be used. Therefore, we need to use the size of virtio_pci_modre_common_cfg to map more space. Meanwhile, this patch checks the common cfg size when _F_RING_ERSET is negotiated. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_pci_modern_dev.c | 27 ++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index aad7d9296e77..45d41e6c7799 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include <linux/virtio_pci_modern.h> +#include <linux/virtio_config.h> #include <linux/module.h> #include <linux/pci.h> #include <linux/delay.h> @@ -142,6 +143,22 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type, return 0; } +static inline int check_common_size(struct virtio_pci_modern_device *mdev, + size_t common_len) +{ + u64 features; + + features = vp_modern_get_features(mdev); + + if (features & BIT_ULL(VIRTIO_F_RING_RESET)) { + if (unlikely(common_len < offsetofend(struct virtio_pci_modern_common_cfg, + queue_reset))) + return -ENOENT; + } + + return 0; +} + /* This is part of the ABI. Don't screw with it. */ static inline void check_offsets(void) { @@ -218,6 +235,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) int err, common, isr, notify, device; u32 notify_length; u32 notify_offset; + size_t common_len; int devid; check_offsets(); @@ -291,10 +309,14 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) err = -EINVAL; mdev->common = vp_modern_map_capability(mdev, common, sizeof(struct virtio_pci_common_cfg), 4, - 0, sizeof(struct virtio_pci_common_cfg), - NULL, NULL); + 0, sizeof(struct virtio_pci_modern_common_cfg), + &common_len, NULL); if (!mdev->common) goto err_map_common; + + if (check_common_size(mdev, common_len)) + goto err_common_size; + mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, 0, 1, NULL, NULL); @@ -353,6 +375,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) err_map_notify: pci_iounmap(pci_dev, mdev->isr); err_map_isr: +err_common_size: pci_iounmap(pci_dev, mdev->common); err_map_common: pci_release_selected_regions(pci_dev, mdev->modern_bars); -- 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset 2023-10-08 7:45 ` [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset Xuan Zhuo @ 2023-10-08 8:48 ` Michael S. Tsirkin 0 siblings, 0 replies; 4+ messages in thread From: Michael S. Tsirkin @ 2023-10-08 8:48 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization On Sun, Oct 08, 2023 at 03:45:34PM +0800, Xuan Zhuo wrote: > Now, the function vp_modern_map_capability() takes the size parameter, > which corresponds to the size of virtio_pci_common_cfg. As a result, > this indicates the size of memory area to map. > > However, if the _F_RING_RESET is negotiated, the extra items will be > used. Therefore, we need to use the size of virtio_pci_modre_common_cfg typo > to map more space. > > Meanwhile, this patch checks the common cfg size when _F_RING_ERSET is > negotiated. > Fixes: tag please? > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_pci_modern_dev.c | 27 ++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > index aad7d9296e77..45d41e6c7799 100644 > --- a/drivers/virtio/virtio_pci_modern_dev.c > +++ b/drivers/virtio/virtio_pci_modern_dev.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-or-later > > #include <linux/virtio_pci_modern.h> > +#include <linux/virtio_config.h> > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/delay.h> > @@ -142,6 +143,22 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type, > return 0; > } > > +static inline int check_common_size(struct virtio_pci_modern_device *mdev, > + size_t common_len) > +{ > + u64 features; > + > + features = vp_modern_get_features(mdev); Better to combine this assignment into definition. Or even better just drop the variable. But more importantly this is called before DRIVER is set, right? Not good then, this is a spec violation to read feature bits before setting DRIVER: The driver MUST follow this sequence to initialize a device: \begin{enumerate} \item Reset the device. \item Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. \item Set the DRIVER status bit: the guest OS knows how to drive the device. \item\label{itm:General Initialization And Device Operation / Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it. I guess we can add common_len alongside notify_len and device_len? I think would also prefer to just clear VIRTIO_F_RING_RESET and not fail probe. > + > + if (features & BIT_ULL(VIRTIO_F_RING_RESET)) { > + if (unlikely(common_len < offsetofend(struct virtio_pci_modern_common_cfg, > + queue_reset))) > + return -ENOENT; Why ENOENT? Also as long as you are doing this, please give the same treatment to VIRTIO_F_NOTIFICATION_DATA. > + } > + > + return 0; > +} > + > /* This is part of the ABI. Don't screw with it. */ > static inline void check_offsets(void) > { > @@ -218,6 +235,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) > int err, common, isr, notify, device; > u32 notify_length; > u32 notify_offset; > + size_t common_len; > int devid; > > check_offsets(); > @@ -291,10 +309,14 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) > err = -EINVAL; > mdev->common = vp_modern_map_capability(mdev, common, > sizeof(struct virtio_pci_common_cfg), 4, > - 0, sizeof(struct virtio_pci_common_cfg), > - NULL, NULL); > + 0, sizeof(struct virtio_pci_modern_common_cfg), > + &common_len, NULL); > if (!mdev->common) > goto err_map_common; > + > + if (check_common_size(mdev, common_len)) > + goto err_common_size; > + > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, > 0, 1, > NULL, NULL); > @@ -353,6 +375,7 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) > err_map_notify: > pci_iounmap(pci_dev, mdev->isr); > err_map_isr: > +err_common_size: > pci_iounmap(pci_dev, mdev->common); > err_map_common: > pci_release_selected_regions(pci_dev, mdev->modern_bars); > -- > 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH vhost v1 2/2] virtio_pci: add build offset check for the new common cfg items 2023-10-08 7:45 [PATCH vhost v1 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 7:45 ` [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset Xuan Zhuo @ 2023-10-08 7:45 ` Xuan Zhuo 1 sibling, 0 replies; 4+ messages in thread From: Xuan Zhuo @ 2023-10-08 7:45 UTC (permalink / raw) To: virtualization; +Cc: Xuan Zhuo, Michael S. Tsirkin Add checks to the check_offsets(void) for queue_notify_data and queue_reset. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Acked-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio_pci_modern_dev.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index 45d41e6c7799..3a49bc963a71 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -220,6 +220,10 @@ static inline void check_offsets(void) offsetof(struct virtio_pci_common_cfg, queue_used_lo)); BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_USEDHI != offsetof(struct virtio_pci_common_cfg, queue_used_hi)); + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_NDATA != + offsetof(struct virtio_pci_modern_common_cfg, queue_notify_data)); + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_RESET != + offsetof(struct virtio_pci_modern_common_cfg, queue_reset)); } /* -- 2.32.0.3.g01195cf9f _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-08 8:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-08 7:45 [PATCH vhost v1 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 7:45 ` [PATCH vhost v1 1/2] virtio_pci: fix the common map size and add check for vq-reset Xuan Zhuo 2023-10-08 8:48 ` Michael S. Tsirkin 2023-10-08 7:45 ` [PATCH vhost v1 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).