From: Jason Wang <jasowang@redhat.com> To: "Michael S. Tsirkin" <mst@redhat.com>, Xie Yongji <xieyongji@bytedance.com> Cc: stefanha@redhat.com, amit@kernel.org, arei.gonglei@huawei.com, airlied@linux.ie, kraxel@redhat.com, dan.j.williams@intel.com, johannes@sipsolutions.net, ohad@wizery.com, bjorn.andersson@linaro.org, david@redhat.com, vgoyal@redhat.com, miklos@szeredi.hu, sgarzare@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length Date: Tue, 25 May 2021 09:31:33 +0800 [thread overview] Message-ID: <3cda643a-1363-65bf-be84-f6dea6714477@redhat.com> (raw) In-Reply-To: <20210517193641-mutt-send-email-mst@kernel.org> 在 2021/5/18 上午7:39, Michael S. Tsirkin 写道: > On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote: >> This adds validation for used length (might come >> from an untrusted device) when it will be used by >> virtio device driver. >> >> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >> --- >> drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >> index d999a1d6d271..7d4845d06f21 100644 >> --- a/drivers/virtio/virtio_ring.c >> +++ b/drivers/virtio/virtio_ring.c >> @@ -68,11 +68,13 @@ >> struct vring_desc_state_split { >> void *data; /* Data for callback. */ >> struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ >> + u32 in_len; /* Total length of writable buffer */ >> }; >> >> struct vring_desc_state_packed { >> void *data; /* Data for callback. */ >> struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */ >> + u32 in_len; /* Total length of writable buffer */ >> u16 num; /* Descriptor list length. */ >> u16 last; /* The last desc state in a list. */ >> }; > > Hmm for packed it's aligned to 64 bit anyway, so we are not making it > any worse. But for split this pushes struct size up by 1/3 increasing > cache pressure. We can eliminate this by validating through virtio device driver instead of virtio core. E.g for virtio-net we know the rx buffer size so there's no need to store in twice in the core. Thanks > > >> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> struct vring_virtqueue *vq = to_vvq(_vq); >> struct scatterlist *sg; >> struct vring_desc *desc; >> - unsigned int i, n, avail, descs_used, prev, err_idx; >> + unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0; >> int head; >> bool indirect; >> >> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> VRING_DESC_F_NEXT | >> VRING_DESC_F_WRITE, >> indirect); >> + in_len += sg->length; >> } >> } >> /* Last one doesn't continue. */ >> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> >> /* Store token and indirect buffer state. */ >> vq->split.desc_state[head].data = data; >> + vq->split.desc_state[head].in_len = in_len; >> if (indirect) >> vq->split.desc_state[head].indir_desc = desc; >> else >> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, >> BAD_RING(vq, "id %u is not a head!\n", i); >> return NULL; >> } >> + if (unlikely(len && vq->split.desc_state[i].in_len < *len)) { >> + BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len); >> + return NULL; >> + } >> >> /* detach_buf_split clears data, so grab it now. */ >> ret = vq->split.desc_state[i].data; >> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> { >> struct vring_packed_desc *desc; >> struct scatterlist *sg; >> - unsigned int i, n, err_idx; >> + unsigned int i, n, err_idx, in_len = 0; >> u16 head, id; >> dma_addr_t addr; >> >> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> if (vring_mapping_error(vq, addr)) >> goto unmap_release; >> >> + in_len += (n < out_sgs) ? 0 : sg->length; >> desc[i].flags = cpu_to_le16(n < out_sgs ? >> 0 : VRING_DESC_F_WRITE); >> desc[i].addr = cpu_to_le64(addr); >> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> vq->packed.desc_state[id].data = data; >> vq->packed.desc_state[id].indir_desc = desc; >> vq->packed.desc_state[id].last = id; >> + vq->packed.desc_state[id].in_len = in_len; >> >> vq->num_added += 1; >> >> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> struct vring_virtqueue *vq = to_vvq(_vq); >> struct vring_packed_desc *desc; >> struct scatterlist *sg; >> - unsigned int i, n, c, descs_used, err_idx; >> + unsigned int i, n, c, descs_used, err_idx, in_len = 0; >> __le16 head_flags, flags; >> u16 head, id, prev, curr, avail_used_flags; >> >> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> if (vring_mapping_error(vq, addr)) >> goto unmap_release; >> >> + in_len += (n < out_sgs) ? 0 : sg->length; >> flags = cpu_to_le16(vq->packed.avail_used_flags | >> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) | >> (n < out_sgs ? 0 : VRING_DESC_F_WRITE)); >> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> vq->packed.desc_state[id].data = data; >> vq->packed.desc_state[id].indir_desc = ctx; >> vq->packed.desc_state[id].last = prev; >> + vq->packed.desc_state[id].in_len = in_len; >> >> /* >> * A driver MUST NOT make the first descriptor in the list >> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, >> BAD_RING(vq, "id %u is not a head!\n", id); >> return NULL; >> } >> + if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) { >> + BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len); >> + return NULL; >> + } >> >> /* detach_buf_packed clears data, so grab it now. */ >> ret = vq->packed.desc_state[id].data; >> -- >> 2.11.0
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: "Michael S. Tsirkin" <mst@redhat.com>, Xie Yongji <xieyongji@bytedance.com> Cc: ohad@wizery.com, amit@kernel.org, airlied@linux.ie, linux-kernel@vger.kernel.org, bjorn.andersson@linaro.org, miklos@szeredi.hu, stefanha@redhat.com, dan.j.williams@intel.com, virtualization@lists.linux-foundation.org, johannes@sipsolutions.net, vgoyal@redhat.com Subject: Re: [RFC PATCH 17/17] virtio_ring: Add validation for used length Date: Tue, 25 May 2021 09:31:33 +0800 [thread overview] Message-ID: <3cda643a-1363-65bf-be84-f6dea6714477@redhat.com> (raw) In-Reply-To: <20210517193641-mutt-send-email-mst@kernel.org> 在 2021/5/18 上午7:39, Michael S. Tsirkin 写道: > On Mon, May 17, 2021 at 05:08:36PM +0800, Xie Yongji wrote: >> This adds validation for used length (might come >> from an untrusted device) when it will be used by >> virtio device driver. >> >> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >> --- >> drivers/virtio/virtio_ring.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >> index d999a1d6d271..7d4845d06f21 100644 >> --- a/drivers/virtio/virtio_ring.c >> +++ b/drivers/virtio/virtio_ring.c >> @@ -68,11 +68,13 @@ >> struct vring_desc_state_split { >> void *data; /* Data for callback. */ >> struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ >> + u32 in_len; /* Total length of writable buffer */ >> }; >> >> struct vring_desc_state_packed { >> void *data; /* Data for callback. */ >> struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */ >> + u32 in_len; /* Total length of writable buffer */ >> u16 num; /* Descriptor list length. */ >> u16 last; /* The last desc state in a list. */ >> }; > > Hmm for packed it's aligned to 64 bit anyway, so we are not making it > any worse. But for split this pushes struct size up by 1/3 increasing > cache pressure. We can eliminate this by validating through virtio device driver instead of virtio core. E.g for virtio-net we know the rx buffer size so there's no need to store in twice in the core. Thanks > > >> @@ -486,7 +488,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> struct vring_virtqueue *vq = to_vvq(_vq); >> struct scatterlist *sg; >> struct vring_desc *desc; >> - unsigned int i, n, avail, descs_used, prev, err_idx; >> + unsigned int i, n, avail, descs_used, prev, err_idx, in_len = 0; >> int head; >> bool indirect; >> >> @@ -570,6 +572,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> VRING_DESC_F_NEXT | >> VRING_DESC_F_WRITE, >> indirect); >> + in_len += sg->length; >> } >> } >> /* Last one doesn't continue. */ >> @@ -604,6 +607,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >> >> /* Store token and indirect buffer state. */ >> vq->split.desc_state[head].data = data; >> + vq->split.desc_state[head].in_len = in_len; >> if (indirect) >> vq->split.desc_state[head].indir_desc = desc; >> else >> @@ -784,6 +788,10 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, >> BAD_RING(vq, "id %u is not a head!\n", i); >> return NULL; >> } >> + if (unlikely(len && vq->split.desc_state[i].in_len < *len)) { >> + BAD_RING(vq, "id %u has invalid length: %u!\n", i, *len); >> + return NULL; >> + } >> >> /* detach_buf_split clears data, so grab it now. */ >> ret = vq->split.desc_state[i].data; >> @@ -1059,7 +1067,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> { >> struct vring_packed_desc *desc; >> struct scatterlist *sg; >> - unsigned int i, n, err_idx; >> + unsigned int i, n, err_idx, in_len = 0; >> u16 head, id; >> dma_addr_t addr; >> >> @@ -1084,6 +1092,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> if (vring_mapping_error(vq, addr)) >> goto unmap_release; >> >> + in_len += (n < out_sgs) ? 0 : sg->length; >> desc[i].flags = cpu_to_le16(n < out_sgs ? >> 0 : VRING_DESC_F_WRITE); >> desc[i].addr = cpu_to_le64(addr); >> @@ -1141,6 +1150,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, >> vq->packed.desc_state[id].data = data; >> vq->packed.desc_state[id].indir_desc = desc; >> vq->packed.desc_state[id].last = id; >> + vq->packed.desc_state[id].in_len = in_len; >> >> vq->num_added += 1; >> >> @@ -1173,7 +1183,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> struct vring_virtqueue *vq = to_vvq(_vq); >> struct vring_packed_desc *desc; >> struct scatterlist *sg; >> - unsigned int i, n, c, descs_used, err_idx; >> + unsigned int i, n, c, descs_used, err_idx, in_len = 0; >> __le16 head_flags, flags; >> u16 head, id, prev, curr, avail_used_flags; >> >> @@ -1223,6 +1233,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> if (vring_mapping_error(vq, addr)) >> goto unmap_release; >> >> + in_len += (n < out_sgs) ? 0 : sg->length; >> flags = cpu_to_le16(vq->packed.avail_used_flags | >> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) | >> (n < out_sgs ? 0 : VRING_DESC_F_WRITE)); >> @@ -1268,6 +1279,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, >> vq->packed.desc_state[id].data = data; >> vq->packed.desc_state[id].indir_desc = ctx; >> vq->packed.desc_state[id].last = prev; >> + vq->packed.desc_state[id].in_len = in_len; >> >> /* >> * A driver MUST NOT make the first descriptor in the list >> @@ -1456,6 +1468,10 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, >> BAD_RING(vq, "id %u is not a head!\n", id); >> return NULL; >> } >> + if (unlikely(len && vq->packed.desc_state[id].in_len < *len)) { >> + BAD_RING(vq, "id %u has invalid length: %u!\n", id, *len); >> + return NULL; >> + } >> >> /* detach_buf_packed clears data, so grab it now. */ >> ret = vq->packed.desc_state[id].data; >> -- >> 2.11.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-05-25 1:31 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-17 9:08 [RFC PATCH 00/17] Add validation for used length Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 01/17] virtio_ring: Avoid reading unneeded " Xie Yongji 2021-05-17 9:11 ` Johannes Berg 2021-05-17 9:11 ` Johannes Berg 2021-05-17 9:41 ` Yongji Xie 2021-05-17 9:08 ` [RFC PATCH 02/17] virtio-blk: Remove unused " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 03/17] virtio_console: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 04/17] crypto: virtio - " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 05/17] drm/virtio: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 06/17] caif_virtio: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 07/17] virtio_net: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 08/17] mac80211_hwsim: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 09/17] virtio_pmem: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 10/17] rpmsg: virtio: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 11/17] virtio_scsi: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 12/17] virtio_balloon: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 13/17] virtio_input: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 14/17] virtio_mem: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 15/17] virtiofs: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 16/17] vsock: " Xie Yongji 2021-05-17 9:08 ` [RFC PATCH 17/17] virtio_ring: Add validation for " Xie Yongji 2021-05-17 23:39 ` Michael S. Tsirkin 2021-05-17 23:39 ` Michael S. Tsirkin 2021-05-25 1:31 ` Jason Wang [this message] 2021-05-25 1:31 ` Jason Wang 2021-05-25 5:08 ` Yongji Xie 2021-05-17 23:40 ` [RFC PATCH 00/17] " Michael S. Tsirkin 2021-05-17 23:40 ` Michael S. Tsirkin 2021-05-18 8:29 ` Yongji Xie 2021-05-18 9:52 ` Michael S. Tsirkin 2021-05-18 9:52 ` Michael S. Tsirkin 2021-05-18 12:28 ` Yongji Xie
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3cda643a-1363-65bf-be84-f6dea6714477@redhat.com \ --to=jasowang@redhat.com \ --cc=airlied@linux.ie \ --cc=amit@kernel.org \ --cc=arei.gonglei@huawei.com \ --cc=bjorn.andersson@linaro.org \ --cc=dan.j.williams@intel.com \ --cc=david@redhat.com \ --cc=johannes@sipsolutions.net \ --cc=kraxel@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=mst@redhat.com \ --cc=ohad@wizery.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=vgoyal@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xieyongji@bytedance.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.