* [dpdk-dev] [PATCH] vhost: fix potential buffer overflow
@ 2021-02-26 7:33 Marvin Liu
2021-03-24 8:55 ` Maxime Coquelin
2021-03-25 3:01 ` [dpdk-dev] [PATCH 1/3] vhost: fix split ring " Marvin Liu
0 siblings, 2 replies; 6+ messages in thread
From: Marvin Liu @ 2021-02-26 7:33 UTC (permalink / raw)
To: maxime.coquelin, chenbo.xia; +Cc: dev, Marvin Liu, stable
In vhost datapath, descriptor's length are mostly used in two coherent
operations. First step is used for address translation, second step is
used for memory transaction from guest to host. But the iterval between
two steps will give a window for malicious guest, in which can change
descriptor length after vhost calcuated buffer size. Thus may lead to
buffer overflow in vhost side. This potential risk can be eliminated by
accessing the descriptor length once.
Fixes: 1be4ebb1c464 ("vhost: support indirect descriptor in mergeable Rx")
Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring")
Fixes: 75ed51697820 ("vhost: add packed ring batch dequeue")
Signed-off-by: Marvin Liu <yong.liu@intel.com>
Cc: stable@dpdk.org
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 583bf379c6..0a7d008a91 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -548,10 +548,11 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
return -1;
}
- len += descs[idx].len;
+ dlen = descs[idx].len;
+ len += dlen;
if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id,
- descs[idx].addr, descs[idx].len,
+ descs[idx].addr, dlen,
perm))) {
free_ind_table(idesc);
return -1;
@@ -668,9 +669,10 @@ fill_vec_buf_packed_indirect(struct virtio_net *dev,
return -1;
}
- *len += descs[i].len;
+ dlen = descs[i].len;
+ *len += dlen;
if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id,
- descs[i].addr, descs[i].len,
+ descs[i].addr, dlen,
perm)))
return -1;
}
@@ -691,6 +693,7 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
bool wrap_counter = vq->avail_wrap_counter;
struct vring_packed_desc *descs = vq->desc_packed;
uint16_t vec_id = *vec_idx;
+ uint64_t dlen;
if (avail_idx < vq->last_avail_idx)
wrap_counter ^= 1;
@@ -723,11 +726,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
len, perm) < 0))
return -1;
} else {
- *len += descs[avail_idx].len;
+ dlen = descs[avail_idx].len;
+ *len += dlen;
if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id,
descs[avail_idx].addr,
- descs[avail_idx].len,
+ dlen,
perm)))
return -1;
}
@@ -2314,7 +2318,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev,
}
vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) {
- pkts[i]->pkt_len = descs[avail_idx + i].len - buf_offset;
+ pkts[i]->pkt_len = lens[i] - buf_offset;
pkts[i]->data_len = pkts[i]->pkt_len;
ids[i] = descs[avail_idx + i].id;
}
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH] vhost: fix potential buffer overflow 2021-02-26 7:33 [dpdk-dev] [PATCH] vhost: fix potential buffer overflow Marvin Liu @ 2021-03-24 8:55 ` Maxime Coquelin 2021-03-25 3:08 ` Liu, Yong 2021-03-25 3:01 ` [dpdk-dev] [PATCH 1/3] vhost: fix split ring " Marvin Liu 1 sibling, 1 reply; 6+ messages in thread From: Maxime Coquelin @ 2021-03-24 8:55 UTC (permalink / raw) To: Marvin Liu, chenbo.xia; +Cc: dev, stable Hi Marvin, On 2/26/21 8:33 AM, Marvin Liu wrote: > In vhost datapath, descriptor's length are mostly used in two coherent > operations. First step is used for address translation, second step is > used for memory transaction from guest to host. But the iterval between > two steps will give a window for malicious guest, in which can change > descriptor length after vhost calcuated buffer size. Thus may lead to > buffer overflow in vhost side. This potential risk can be eliminated by > accessing the descriptor length once. > > Fixes: 1be4ebb1c464 ("vhost: support indirect descriptor in mergeable Rx") > Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring") > Fixes: 75ed51697820 ("vhost: add packed ring batch dequeue") As the offending commits have been introduced in different LTS, I would prefer the patch to be split. It will make is easier for backporting later. > Signed-off-by: Marvin Liu <yong.liu@intel.com> > Cc: stable@dpdk.org > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > index 583bf379c6..0a7d008a91 100644 > --- a/lib/librte_vhost/virtio_net.c > +++ b/lib/librte_vhost/virtio_net.c > @@ -548,10 +548,11 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq, > return -1; > } > > - len += descs[idx].len; > + dlen = descs[idx].len; > + len += dlen; > > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > - descs[idx].addr, descs[idx].len, > + descs[idx].addr, dlen, > perm))) { > free_ind_table(idesc); > return -1; > @@ -668,9 +669,10 @@ fill_vec_buf_packed_indirect(struct virtio_net *dev, > return -1; > } > > - *len += descs[i].len; > + dlen = descs[i].len; > + *len += dlen; > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > - descs[i].addr, descs[i].len, > + descs[i].addr, dlen, > perm))) > return -1; > } > @@ -691,6 +693,7 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq, > bool wrap_counter = vq->avail_wrap_counter; > struct vring_packed_desc *descs = vq->desc_packed; > uint16_t vec_id = *vec_idx; > + uint64_t dlen; > > if (avail_idx < vq->last_avail_idx) > wrap_counter ^= 1; > @@ -723,11 +726,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq, > len, perm) < 0)) > return -1; > } else { > - *len += descs[avail_idx].len; > + dlen = descs[avail_idx].len; > + *len += dlen; > > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > descs[avail_idx].addr, > - descs[avail_idx].len, > + dlen, > perm))) > return -1; > } > @@ -2314,7 +2318,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev, > } > > vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) { > - pkts[i]->pkt_len = descs[avail_idx + i].len - buf_offset; > + pkts[i]->pkt_len = lens[i] - buf_offset; > pkts[i]->data_len = pkts[i]->pkt_len; > ids[i] = descs[avail_idx + i].id; > } > Other than that, the patch looks valid to me. With the split done: Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> Thanks, Maxime ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix potential buffer overflow 2021-03-24 8:55 ` Maxime Coquelin @ 2021-03-25 3:08 ` Liu, Yong 0 siblings, 0 replies; 6+ messages in thread From: Liu, Yong @ 2021-03-25 3:08 UTC (permalink / raw) To: Maxime Coquelin, Xia, Chenbo; +Cc: dev, stable > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, March 24, 2021 4:56 PM > To: Liu, Yong <yong.liu@intel.com>; Xia, Chenbo <chenbo.xia@intel.com> > Cc: dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH] vhost: fix potential buffer overflow > > Hi Marvin, > > On 2/26/21 8:33 AM, Marvin Liu wrote: > > In vhost datapath, descriptor's length are mostly used in two coherent > > operations. First step is used for address translation, second step is > > used for memory transaction from guest to host. But the iterval between > > two steps will give a window for malicious guest, in which can change > > descriptor length after vhost calcuated buffer size. Thus may lead to > > buffer overflow in vhost side. This potential risk can be eliminated by > > accessing the descriptor length once. > > > > Fixes: 1be4ebb1c464 ("vhost: support indirect descriptor in mergeable Rx") > > Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring") > > Fixes: 75ed51697820 ("vhost: add packed ring batch dequeue") > > As the offending commits have been introduced in different LTS, I would > prefer the patch to be split. It will make is easier for backporting later. > Maxime, Thanks for your suggestion, I will split this patch into three parts as they were spread over three different LTS. Regards, Marvin > > Signed-off-by: Marvin Liu <yong.liu@intel.com> > > Cc: stable@dpdk.org > > > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > > index 583bf379c6..0a7d008a91 100644 > > --- a/lib/librte_vhost/virtio_net.c > > +++ b/lib/librte_vhost/virtio_net.c > > @@ -548,10 +548,11 @@ fill_vec_buf_split(struct virtio_net *dev, struct > vhost_virtqueue *vq, > > return -1; > > } > > > > - len += descs[idx].len; > > + dlen = descs[idx].len; > > + len += dlen; > > > > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > > - descs[idx].addr, > descs[idx].len, > > + descs[idx].addr, dlen, > > perm))) { > > free_ind_table(idesc); > > return -1; > > @@ -668,9 +669,10 @@ fill_vec_buf_packed_indirect(struct virtio_net > *dev, > > return -1; > > } > > > > - *len += descs[i].len; > > + dlen = descs[i].len; > > + *len += dlen; > > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > > - descs[i].addr, descs[i].len, > > + descs[i].addr, dlen, > > perm))) > > return -1; > > } > > @@ -691,6 +693,7 @@ fill_vec_buf_packed(struct virtio_net *dev, struct > vhost_virtqueue *vq, > > bool wrap_counter = vq->avail_wrap_counter; > > struct vring_packed_desc *descs = vq->desc_packed; > > uint16_t vec_id = *vec_idx; > > + uint64_t dlen; > > > > if (avail_idx < vq->last_avail_idx) > > wrap_counter ^= 1; > > @@ -723,11 +726,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct > vhost_virtqueue *vq, > > len, perm) < 0)) > > return -1; > > } else { > > - *len += descs[avail_idx].len; > > + dlen = descs[avail_idx].len; > > + *len += dlen; > > > > if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, > > descs[avail_idx].addr, > > - descs[avail_idx].len, > > + dlen, > > perm))) > > return -1; > > } > > @@ -2314,7 +2318,7 @@ vhost_reserve_avail_batch_packed(struct > virtio_net *dev, > > } > > > > vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) { > > - pkts[i]->pkt_len = descs[avail_idx + i].len - buf_offset; > > + pkts[i]->pkt_len = lens[i] - buf_offset; > > pkts[i]->data_len = pkts[i]->pkt_len; > > ids[i] = descs[avail_idx + i].id; > > } > > > > Other than that, the patch looks valid to me. > With the split done: > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > Thanks, > Maxime ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH 1/3] vhost: fix split ring potential buffer overflow 2021-02-26 7:33 [dpdk-dev] [PATCH] vhost: fix potential buffer overflow Marvin Liu 2021-03-24 8:55 ` Maxime Coquelin @ 2021-03-25 3:01 ` Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 2/3] vhost: fix packed " Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 3/3] vhost: fix potential buffer overflow when batch dequeue Marvin Liu 1 sibling, 2 replies; 6+ messages in thread From: Marvin Liu @ 2021-03-25 3:01 UTC (permalink / raw) To: maxime.coquelin, chenbo.xia; +Cc: dev, Marvin Liu, stable In vhost datapath, descriptor's length are mostly used in two coherent operations. First step is used for address translation, second step is used for memory transaction from guest to host. But the iterval between two steps will give a window for malicious guest, in which can change descriptor length after vhost calcuated buffer size. Thus may lead to buffer overflow in vhost side. This potential risk can be eliminated by accessing the descriptor length once. Fixes: 1be4ebb1c464 ("vhost: support indirect descriptor in mergeable Rx") Cc: stable@dpdk.org Signed-off-by: Marvin Liu <yong.liu@intel.com> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 583bf379c6..576a0a20c0 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -548,10 +548,11 @@ fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq, return -1; } - len += descs[idx].len; + dlen = descs[idx].len; + len += dlen; if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, - descs[idx].addr, descs[idx].len, + descs[idx].addr, dlen, perm))) { free_ind_table(idesc); return -1; -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH 2/3] vhost: fix packed ring potential buffer overflow 2021-03-25 3:01 ` [dpdk-dev] [PATCH 1/3] vhost: fix split ring " Marvin Liu @ 2021-03-25 3:01 ` Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 3/3] vhost: fix potential buffer overflow when batch dequeue Marvin Liu 1 sibling, 0 replies; 6+ messages in thread From: Marvin Liu @ 2021-03-25 3:01 UTC (permalink / raw) To: maxime.coquelin, chenbo.xia; +Cc: dev, Marvin Liu, stable Similar as split ring, the multiple accesses of descriptor length will lead to potential risk. One-time access of descriptor length can eliminate this risk. Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring") Cc: stable@dpdk.org Signed-off-by: Marvin Liu <yong.liu@intel.com> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 576a0a20c0..de43686522 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -669,9 +669,10 @@ fill_vec_buf_packed_indirect(struct virtio_net *dev, return -1; } - *len += descs[i].len; + dlen = descs[i].len; + *len += dlen; if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, - descs[i].addr, descs[i].len, + descs[i].addr, dlen, perm))) return -1; } @@ -692,6 +693,7 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq, bool wrap_counter = vq->avail_wrap_counter; struct vring_packed_desc *descs = vq->desc_packed; uint16_t vec_id = *vec_idx; + uint64_t dlen; if (avail_idx < vq->last_avail_idx) wrap_counter ^= 1; @@ -724,11 +726,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq, len, perm) < 0)) return -1; } else { - *len += descs[avail_idx].len; + dlen = descs[avail_idx].len; + *len += dlen; if (unlikely(map_one_desc(dev, vq, buf_vec, &vec_id, descs[avail_idx].addr, - descs[avail_idx].len, + dlen, perm))) return -1; } -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH 3/3] vhost: fix potential buffer overflow when batch dequeue 2021-03-25 3:01 ` [dpdk-dev] [PATCH 1/3] vhost: fix split ring " Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 2/3] vhost: fix packed " Marvin Liu @ 2021-03-25 3:01 ` Marvin Liu 1 sibling, 0 replies; 6+ messages in thread From: Marvin Liu @ 2021-03-25 3:01 UTC (permalink / raw) To: maxime.coquelin, chenbo.xia; +Cc: dev, Marvin Liu, stable Similar as single dequeue, the multiple accesses of descriptor length will lead to potential risk. One-time access of descriptor length can eliminate this risk. Fixes: 75ed51697820 ("vhost: add packed ring batch dequeue") Cc: stable@dpdk.org Signed-off-by: Marvin Liu <yong.liu@intel.com> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index de43686522..0a7d008a91 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -2318,7 +2318,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev, } vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) { - pkts[i]->pkt_len = descs[avail_idx + i].len - buf_offset; + pkts[i]->pkt_len = lens[i] - buf_offset; pkts[i]->data_len = pkts[i]->pkt_len; ids[i] = descs[avail_idx + i].id; } -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-25 3:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-26 7:33 [dpdk-dev] [PATCH] vhost: fix potential buffer overflow Marvin Liu 2021-03-24 8:55 ` Maxime Coquelin 2021-03-25 3:08 ` Liu, Yong 2021-03-25 3:01 ` [dpdk-dev] [PATCH 1/3] vhost: fix split ring " Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 2/3] vhost: fix packed " Marvin Liu 2021-03-25 3:01 ` [dpdk-dev] [PATCH 3/3] vhost: fix potential buffer overflow when batch dequeue Marvin Liu
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).