* [PATCH vhost v2 0/2] strictly check the acccess to the common cfg @ 2023-10-08 9:38 Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 0 siblings, 2 replies; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 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 common size virtio_pci: add build offset check for the new common cfg items drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- drivers/virtio/virtio_pci_modern_dev.c | 8 ++++++-- include/linux/virtio_pci_modern.h | 1 + 3 files changed, 26 insertions(+), 3 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] 6+ messages in thread
* [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo @ 2023-10-08 9:38 ` Xuan Zhuo 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-08 9:38 ` [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items Xuan Zhuo 1 sibling, 1 reply; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 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_modern_common_cfg to map more space. Meanwhile, this patch removes the feature(_F_RING_ERSET and _F_NOTIFICATION_DATA) when the common cfg size does not match the corresponding feature. Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset") Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- include/linux/virtio_pci_modern.h | 1 + 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index d6bb68ba84e5..c0b9d2363ddb 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -22,8 +22,26 @@ static u64 vp_get_features(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); + u64 features = vp_modern_get_features(&vp_dev->mdev); + +#define check_feature(feature, field) \ + do { \ + if (features & BIT_ULL(feature)) { \ + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \ + if (unlikely(vp_dev->mdev.common_len < offset)) \ + features &= ~BIT_ULL(feature); \ + } \ + } while (0) + + /* For buggy devices, if the common len does not match the feature, we + * remove the feature. + */ + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data); + check_feature(VIRTIO_F_RING_RESET, queue_reset); + +#undef check_feature - return vp_modern_get_features(&vp_dev->mdev); + return features; } static void vp_transport_features(struct virtio_device *vdev, u64 features) diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index aad7d9296e77..33f319da1558 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -291,8 +291,8 @@ 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), + &mdev->common_len, NULL); if (!mdev->common) goto err_map_common; mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h index 067ac1d789bc..edf62bae0474 100644 --- a/include/linux/virtio_pci_modern.h +++ b/include/linux/virtio_pci_modern.h @@ -28,6 +28,7 @@ struct virtio_pci_modern_device { /* So we can sanity-check accesses. */ size_t notify_len; size_t device_len; + size_t common_len; /* Capability for when we need to map notifications per-vq. */ int notify_map_cap; -- 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] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo @ 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-09 1:15 ` Xuan Zhuo 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2023-10-08 10:42 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization On Sun, Oct 08, 2023 at 05:38:41PM +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_modern_common_cfg > to map more space. > > Meanwhile, this patch removes the feature(_F_RING_ERSET and typo > _F_NOTIFICATION_DATA) when the common cfg size does not match > the corresponding feature. > > Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset") > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- > drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- > include/linux/virtio_pci_modern.h | 1 + > 3 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index d6bb68ba84e5..c0b9d2363ddb 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -22,8 +22,26 @@ > static u64 vp_get_features(struct virtio_device *vdev) > { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + u64 features = vp_modern_get_features(&vp_dev->mdev); > + > +#define check_feature(feature, field) \ > + do { \ > + if (features & BIT_ULL(feature)) { \ > + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \ > + if (unlikely(vp_dev->mdev.common_len < offset)) \ > + features &= ~BIT_ULL(feature); \ > + } \ > + } while (0) > + > + /* For buggy devices, if the common len does not match the feature, we > + * remove the feature. I don't like doing this in vp_get_features. userspace won't be able to see the actual device features at all. Also, we should print an info message at least. I am still debating with myself whether clearing feature bits or just failing finalize_features (and thus, probe) is best. > + */ > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data); > + check_feature(VIRTIO_F_RING_RESET, queue_reset); > + > +#undef check_feature this macro's too scary. just pass offset and feature bit as parameters to an inline function. > > - return vp_modern_get_features(&vp_dev->mdev); > + return features; > } > > static void vp_transport_features(struct virtio_device *vdev, u64 features) > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > index aad7d9296e77..33f319da1558 100644 > --- a/drivers/virtio/virtio_pci_modern_dev.c > +++ b/drivers/virtio/virtio_pci_modern_dev.c > @@ -291,8 +291,8 @@ 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), > + &mdev->common_len, NULL); > if (!mdev->common) > goto err_map_common; > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h > index 067ac1d789bc..edf62bae0474 100644 > --- a/include/linux/virtio_pci_modern.h > +++ b/include/linux/virtio_pci_modern.h > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device { > /* So we can sanity-check accesses. */ > size_t notify_len; > size_t device_len; > + size_t common_len; > > /* Capability for when we need to map notifications per-vq. */ > int notify_map_cap; > -- > 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] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-08 10:42 ` Michael S. Tsirkin @ 2023-10-09 1:15 ` Xuan Zhuo 2023-10-09 10:37 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Xuan Zhuo @ 2023-10-09 1:15 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Oct 08, 2023 at 05:38:41PM +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_modern_common_cfg > > to map more space. > > > > Meanwhile, this patch removes the feature(_F_RING_ERSET and > > typo > > > _F_NOTIFICATION_DATA) when the common cfg size does not match > > the corresponding feature. > > > > Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset") > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- > > drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- > > include/linux/virtio_pci_modern.h | 1 + > > 3 files changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index d6bb68ba84e5..c0b9d2363ddb 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -22,8 +22,26 @@ > > static u64 vp_get_features(struct virtio_device *vdev) > > { > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > + u64 features = vp_modern_get_features(&vp_dev->mdev); > > + > > +#define check_feature(feature, field) \ > > + do { \ > > + if (features & BIT_ULL(feature)) { \ > > + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \ > > + if (unlikely(vp_dev->mdev.common_len < offset)) \ > > + features &= ~BIT_ULL(feature); \ > > + } \ > > + } while (0) > > + > > + /* For buggy devices, if the common len does not match the feature, we > > + * remove the feature. > > I don't like doing this in vp_get_features. userspace won't be able > to see the actual device features at all. > Also, we should print an info message at least. > > I am still debating with myself whether clearing feature bits > or just failing finalize_features (and thus, probe) is best. For me, I think failing probe is best. Then the developer of the device can find that firstly. And I think we should print an info message when we detect this error. If we clear the feature bits, the developer of the device may ignore this error. > > > > + */ > > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data); > > + check_feature(VIRTIO_F_RING_RESET, queue_reset); > > + > > +#undef check_feature > > this macro's too scary. just pass offset and feature bit as > parameters to an inline function. I should pass the features as a parameter. Thanks. > > > > > - return vp_modern_get_features(&vp_dev->mdev); > > + return features; > > } > > > > static void vp_transport_features(struct virtio_device *vdev, u64 features) > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > > index aad7d9296e77..33f319da1558 100644 > > --- a/drivers/virtio/virtio_pci_modern_dev.c > > +++ b/drivers/virtio/virtio_pci_modern_dev.c > > @@ -291,8 +291,8 @@ 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), > > + &mdev->common_len, NULL); > > if (!mdev->common) > > goto err_map_common; > > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, > > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h > > index 067ac1d789bc..edf62bae0474 100644 > > --- a/include/linux/virtio_pci_modern.h > > +++ b/include/linux/virtio_pci_modern.h > > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device { > > /* So we can sanity-check accesses. */ > > size_t notify_len; > > size_t device_len; > > + size_t common_len; > > > > /* Capability for when we need to map notifications per-vq. */ > > int notify_map_cap; > > -- > > 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] 6+ messages in thread
* Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size 2023-10-09 1:15 ` Xuan Zhuo @ 2023-10-09 10:37 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2023-10-09 10:37 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization On Mon, Oct 09, 2023 at 09:15:31AM +0800, Xuan Zhuo wrote: > On Sun, 8 Oct 2023 06:42:37 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Sun, Oct 08, 2023 at 05:38:41PM +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_modern_common_cfg > > > to map more space. > > > > > > Meanwhile, this patch removes the feature(_F_RING_ERSET and > > > > typo > > > > > _F_NOTIFICATION_DATA) when the common cfg size does not match > > > the corresponding feature. > > > > > > Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset") > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > --- > > > drivers/virtio/virtio_pci_modern.c | 20 +++++++++++++++++++- > > > drivers/virtio/virtio_pci_modern_dev.c | 4 ++-- > > > include/linux/virtio_pci_modern.h | 1 + > > > 3 files changed, 22 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > index d6bb68ba84e5..c0b9d2363ddb 100644 > > > --- a/drivers/virtio/virtio_pci_modern.c > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > @@ -22,8 +22,26 @@ > > > static u64 vp_get_features(struct virtio_device *vdev) > > > { > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > + u64 features = vp_modern_get_features(&vp_dev->mdev); > > > + > > > +#define check_feature(feature, field) \ > > > + do { \ > > > + if (features & BIT_ULL(feature)) { \ > > > + u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field); \ > > > + if (unlikely(vp_dev->mdev.common_len < offset)) \ > > > + features &= ~BIT_ULL(feature); \ > > > + } \ > > > + } while (0) > > > + > > > + /* For buggy devices, if the common len does not match the feature, we > > > + * remove the feature. > > > > I don't like doing this in vp_get_features. userspace won't be able > > to see the actual device features at all. > > Also, we should print an info message at least. > > > > I am still debating with myself whether clearing feature bits > > or just failing finalize_features (and thus, probe) is best. > > For me, I think failing probe is best. > > Then the developer of the device can find that firstly. > And I think we should print an info message when we detect > this error. If you fail probe - maybe even a warning. > If we clear the feature bits, the developer of the device may > ignore this error. > > > > > > > > + */ > > > + check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data); > > > + check_feature(VIRTIO_F_RING_RESET, queue_reset); > > > + > > > +#undef check_feature > > > > this macro's too scary. just pass offset and feature bit as > > parameters to an inline function. > > I should pass the features as a parameter. > > Thanks. > > > > > > > > > > > - return vp_modern_get_features(&vp_dev->mdev); > > > + return features; > > > } > > > > > > static void vp_transport_features(struct virtio_device *vdev, u64 features) > > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c > > > index aad7d9296e77..33f319da1558 100644 > > > --- a/drivers/virtio/virtio_pci_modern_dev.c > > > +++ b/drivers/virtio/virtio_pci_modern_dev.c > > > @@ -291,8 +291,8 @@ 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), > > > + &mdev->common_len, NULL); > > > if (!mdev->common) > > > goto err_map_common; > > > mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1, > > > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h > > > index 067ac1d789bc..edf62bae0474 100644 > > > --- a/include/linux/virtio_pci_modern.h > > > +++ b/include/linux/virtio_pci_modern.h > > > @@ -28,6 +28,7 @@ struct virtio_pci_modern_device { > > > /* So we can sanity-check accesses. */ > > > size_t notify_len; > > > size_t device_len; > > > + size_t common_len; > > > > > > /* Capability for when we need to map notifications per-vq. */ > > > int notify_map_cap; > > > -- > > > 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] 6+ messages in thread
* [PATCH vhost v2 2/2] virtio_pci: add build offset check for the new common cfg items 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo @ 2023-10-08 9:38 ` Xuan Zhuo 1 sibling, 0 replies; 6+ messages in thread From: Xuan Zhuo @ 2023-10-08 9:38 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 33f319da1558..e2a1fe7bb66c 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -203,6 +203,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] 6+ messages in thread
end of thread, other threads:[~2023-10-09 10:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-08 9:38 [PATCH vhost v2 0/2] strictly check the acccess to the common cfg Xuan Zhuo 2023-10-08 9:38 ` [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size Xuan Zhuo 2023-10-08 10:42 ` Michael S. Tsirkin 2023-10-09 1:15 ` Xuan Zhuo 2023-10-09 10:37 ` Michael S. Tsirkin 2023-10-08 9:38 ` [PATCH vhost v2 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).