Hi, On Wed, Dec 18, 2024 at 09:59:05AM +0100, Maxime Coquelin wrote: > Hi, > > On 12/18/24 08:34, Wangyunjian(wangyunjian,TongTu) wrote: > > > -----Original Message----- > > > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] > > > Sent: Tuesday, December 17, 2024 11:33 PM > > > To: dev@dpdk.org > > > Cc: Olivier Matz <olivier.m...@6wind.com>; Maxime Gouin > > > <maxime.go...@6wind.com>; Maxime Coquelin > > > <maxime.coque...@redhat.com> > > > Subject: [PATCH] net/virtio: fix Rx checksum calculation > > > > > > From: Olivier Matz <olivier.m...@6wind.com> > > > > > > If hdr->csum_start is larger than packet length, the len argument passed > > > to rte_raw_cksum_mbuf() overflows and causes a segmentation fault. > > > > > > Ignore checksum computation in this case. > > > > > > CVE-2024-11614 > > > > > > Fixes: ca7036b4af3a ("vhost: fix offload flags in Rx path") > > > > > > Signed-off-by: Maxime Gouin <maxime.go...@6wind.com> > > > Signed-off-by: Olivier Matz <olivier.m...@6wind.com> > > > Reviewed-by: Maxime Coquelin <maxime.coque...@redhat.com> > > > --- > > > lib/vhost/virtio_net.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c > > > index d764d4bc6a..69901ab3b5 100644 > > > --- a/lib/vhost/virtio_net.c > > > +++ b/lib/vhost/virtio_net.c > > > @@ -2823,6 +2823,9 @@ vhost_dequeue_offload(struct virtio_net *dev, > > > struct virtio_net_hdr *hdr, > > > */ > > > uint16_t csum = 0, off; > > > > > > + if (hdr->csum_start >= rte_pktmbuf_pkt_len(m)) > > > + return; > > > + > > > > The hdr->csum_start does two successive reads from user space to read > > a variable length data structure. The result overflow if the data structure > > changes between the two reads. > > > > We can prevent double fetch issue by using the temporary variable > > csum_start.
This is an interesting remark, thanks! However, in practical, I'd say that the hdr->csum_start is fetched in a register only once if using optimized compilation, because the compiler has no reason to think that hdr->csum_start can be modified. Olivier > > Right, that's a good catch! The exploitation od this issue seem > difficult though. > > We may systematically copy the full header, as we only do it for ones > not contiguous in host VA space. > > What do you think? Are you willing to contribute a fix? > > Thanks, > Maxime > > > > Thanks, > > Yunjian > > > > > if (rte_raw_cksum_mbuf(m, hdr->csum_start, > > > rte_pktmbuf_pkt_len(m) - > > > hdr->csum_start, &csum) < > > > 0) > > > return; > > > -- > > > 2.47.0 > > >
