| Message ID | 20210525045838.1137-1-xieyongji@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net: Add validation for used length | expand |
在 2021/5/25 下午12:58, Xie Yongji 写道: > This adds validation for used length (might come > from an untrusted device) to avoid data corruption > or loss. > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > --- > drivers/net/virtio_net.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index c4711e23af88..2dcdc1a3c7e8 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -668,6 +668,13 @@ static struct sk_buff *receive_small(struct net_device *dev, > void *orig_data; > u32 act; > > + if (unlikely(len > GOOD_PACKET_LEN)) { > + pr_debug("%s: rx error: len %u exceeds max size %lu\n", > + dev->name, len, GOOD_PACKET_LEN); > + dev->stats.rx_length_errors++; > + goto err_xdp; > + } Need to count vi->hdr_len here? > + > if (unlikely(hdr->hdr.gso_type)) > goto err_xdp; > > @@ -739,6 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev, > } > rcu_read_unlock(); > > + if (unlikely(len > GOOD_PACKET_LEN)) { > + pr_debug("%s: rx error: len %u exceeds max size %lu\n", > + dev->name, len, GOOD_PACKET_LEN); > + dev->stats.rx_length_errors++; > + put_page(page); > + return NULL; > + } > + > skb = build_skb(buf, buflen); > if (!skb) { > put_page(page); > @@ -822,6 +837,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > void *data; > u32 act; > > + if (unlikely(len > truesize)) { > + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", > + dev->name, len, (unsigned long)ctx); > + dev->stats.rx_length_errors++; > + goto err_xdp; > + } There's a similar check after the XDP, let's simply move it here? And do we need similar check in receive_big()? Thanks > + > /* Transient failure which in theory could occur if > * in-flight packets from before XDP was enabled reach > * the receive path after XDP is loaded.
在 2021/5/25 下午4:45, Yongji Xie 写道: > On Tue, May 25, 2021 at 2:30 PM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/5/25 下午12:58, Xie Yongji 写道: >>> This adds validation for used length (might come >>> from an untrusted device) to avoid data corruption >>> or loss. >>> >>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >>> --- >>> drivers/net/virtio_net.c | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>> index c4711e23af88..2dcdc1a3c7e8 100644 >>> --- a/drivers/net/virtio_net.c >>> +++ b/drivers/net/virtio_net.c >>> @@ -668,6 +668,13 @@ static struct sk_buff *receive_small(struct net_device *dev, >>> void *orig_data; >>> u32 act; >>> >>> + if (unlikely(len > GOOD_PACKET_LEN)) { >>> + pr_debug("%s: rx error: len %u exceeds max size %lu\n", >>> + dev->name, len, GOOD_PACKET_LEN); >>> + dev->stats.rx_length_errors++; >>> + goto err_xdp; >>> + } >> >> Need to count vi->hdr_len here? >> > We did len -= vi->hdr_len before. Right. > >>> + >>> if (unlikely(hdr->hdr.gso_type)) >>> goto err_xdp; >>> >>> @@ -739,6 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev, >>> } >>> rcu_read_unlock(); >>> >>> + if (unlikely(len > GOOD_PACKET_LEN)) { >>> + pr_debug("%s: rx error: len %u exceeds max size %lu\n", >>> + dev->name, len, GOOD_PACKET_LEN); >>> + dev->stats.rx_length_errors++; >>> + put_page(page); >>> + return NULL; >>> + } >>> + >>> skb = build_skb(buf, buflen); >>> if (!skb) { >>> put_page(page); >>> @@ -822,6 +837,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >>> void *data; >>> u32 act; >>> >>> + if (unlikely(len > truesize)) { >>> + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", >>> + dev->name, len, (unsigned long)ctx); >>> + dev->stats.rx_length_errors++; >>> + goto err_xdp; >>> + } >> >> There's a similar check after the XDP, let's simply move it here? > Do we still need that in non-XDP cases? I meant we check once for both XDP and non-XDP if we do it before if (xdp_prog) > >> And do we need similar check in receive_big()? >> > It seems that the check in page_to_skb() can do similar things. Right. Thanks > > Thanks, > Yongji >
On Wed, May 26, 2021 at 3:52 PM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/5/25 下午4:45, Yongji Xie 写道: > > On Tue, May 25, 2021 at 2:30 PM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2021/5/25 下午12:58, Xie Yongji 写道: > >>> This adds validation for used length (might come > >>> from an untrusted device) to avoid data corruption > >>> or loss. > >>> > >>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > >>> --- > >>> drivers/net/virtio_net.c | 22 ++++++++++++++++++++++ > >>> 1 file changed, 22 insertions(+) > >>> > >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >>> index c4711e23af88..2dcdc1a3c7e8 100644 > >>> --- a/drivers/net/virtio_net.c > >>> +++ b/drivers/net/virtio_net.c > >>> @@ -668,6 +668,13 @@ static struct sk_buff *receive_small(struct net_device *dev, > >>> void *orig_data; > >>> u32 act; > >>> > >>> + if (unlikely(len > GOOD_PACKET_LEN)) { > >>> + pr_debug("%s: rx error: len %u exceeds max size %lu\n", > >>> + dev->name, len, GOOD_PACKET_LEN); > >>> + dev->stats.rx_length_errors++; > >>> + goto err_xdp; > >>> + } > >> > >> Need to count vi->hdr_len here? > >> > > We did len -= vi->hdr_len before. > > > Right. > > > > > >>> + > >>> if (unlikely(hdr->hdr.gso_type)) > >>> goto err_xdp; > >>> > >>> @@ -739,6 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev, > >>> } > >>> rcu_read_unlock(); > >>> > >>> + if (unlikely(len > GOOD_PACKET_LEN)) { > >>> + pr_debug("%s: rx error: len %u exceeds max size %lu\n", > >>> + dev->name, len, GOOD_PACKET_LEN); > >>> + dev->stats.rx_length_errors++; > >>> + put_page(page); > >>> + return NULL; > >>> + } > >>> + > >>> skb = build_skb(buf, buflen); > >>> if (!skb) { > >>> put_page(page); > >>> @@ -822,6 +837,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > >>> void *data; > >>> u32 act; > >>> > >>> + if (unlikely(len > truesize)) { > >>> + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", > >>> + dev->name, len, (unsigned long)ctx); > >>> + dev->stats.rx_length_errors++; > >>> + goto err_xdp; > >>> + } > >> > >> There's a similar check after the XDP, let's simply move it here? > > Do we still need that in non-XDP cases? > > > I meant we check once for both XDP and non-XDP if we do it before if > (xdp_prog) > Will do it in v2. Thanks, Yongji
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index c4711e23af88..2dcdc1a3c7e8 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -668,6 +668,13 @@ static struct sk_buff *receive_small(struct net_device *dev, void *orig_data; u32 act; + if (unlikely(len > GOOD_PACKET_LEN)) { + pr_debug("%s: rx error: len %u exceeds max size %lu\n", + dev->name, len, GOOD_PACKET_LEN); + dev->stats.rx_length_errors++; + goto err_xdp; + } + if (unlikely(hdr->hdr.gso_type)) goto err_xdp; @@ -739,6 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev, } rcu_read_unlock(); + if (unlikely(len > GOOD_PACKET_LEN)) { + pr_debug("%s: rx error: len %u exceeds max size %lu\n", + dev->name, len, GOOD_PACKET_LEN); + dev->stats.rx_length_errors++; + put_page(page); + return NULL; + } + skb = build_skb(buf, buflen); if (!skb) { put_page(page); @@ -822,6 +837,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, void *data; u32 act; + if (unlikely(len > truesize)) { + pr_debug("%s: rx error: len %u exceeds truesize %lu\n", + dev->name, len, (unsigned long)ctx); + dev->stats.rx_length_errors++; + goto err_xdp; + } + /* Transient failure which in theory could occur if * in-flight packets from before XDP was enabled reach * the receive path after XDP is loaded.
This adds validation for used length (might come from an untrusted device) to avoid data corruption or loss. Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- drivers/net/virtio_net.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)