| Message ID | 20210525045838.1137-1-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: Add validation for used length | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: hawk@kernel.org daniel@iogearbox.net bpf@vger.kernel.org ast@kernel.org john.fastabend@gmail.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 0 this patch: 8 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 8 |
| netdev/header_inline | success | Link |
在 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.
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. > > > + > > 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? > > And do we need similar check in receive_big()? > It seems that the check in page_to_skb() can do similar things. Thanks, Yongji
Hi Xie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.13-rc3 next-20210525]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Xie-Yongji/virtio-net-Add-validation-for-used-length/20210525-130449
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a050a6d2b7e80ca52b2f4141eaf3420d201b72b3
config: x86_64-randconfig-a005-20210525 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 99155e913e9bad5f7f8a247f8bb3a3ff3da74af1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/9cacb0e306acf93325699401dcae01f77505f017
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xie-Yongji/virtio-net-Add-validation-for-used-length/20210525-130449
git checkout 9cacb0e306acf93325699401dcae01f77505f017
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/virtio_net.c:740:22: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
dev->name, len, GOOD_PACKET_LEN);
^~~~~~~~~~~~~~~
include/linux/printk.h:424:26: note: expanded from macro 'pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:163:22: note: expanded from macro 'dynamic_pr_debug'
pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call'
__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
drivers/net/virtio_net.c:35:25: note: expanded from macro 'GOOD_PACKET_LEN'
#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/virtio_net.c:820:21: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
dev->name, len, GOOD_PACKET_LEN);
^~~~~~~~~~~~~~~
include/linux/printk.h:424:26: note: expanded from macro 'pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:163:22: note: expanded from macro 'dynamic_pr_debug'
pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call'
__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call'
func(&id, ##__VA_ARGS__); \
^~~~~~~~~~~
drivers/net/virtio_net.c:35:25: note: expanded from macro 'GOOD_PACKET_LEN'
#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +740 drivers/net/virtio_net.c
704
705 static struct sk_buff *receive_small(struct net_device *dev,
706 struct virtnet_info *vi,
707 struct receive_queue *rq,
708 void *buf, void *ctx,
709 unsigned int len,
710 unsigned int *xdp_xmit,
711 struct virtnet_rq_stats *stats)
712 {
713 struct sk_buff *skb;
714 struct bpf_prog *xdp_prog;
715 unsigned int xdp_headroom = (unsigned long)ctx;
716 unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
717 unsigned int headroom = vi->hdr_len + header_offset;
718 unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
719 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
720 struct page *page = virt_to_head_page(buf);
721 unsigned int delta = 0;
722 struct page *xdp_page;
723 int err;
724 unsigned int metasize = 0;
725
726 len -= vi->hdr_len;
727 stats->bytes += len;
728
729 rcu_read_lock();
730 xdp_prog = rcu_dereference(rq->xdp_prog);
731 if (xdp_prog) {
732 struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
733 struct xdp_frame *xdpf;
734 struct xdp_buff xdp;
735 void *orig_data;
736 u32 act;
737
738 if (unlikely(len > GOOD_PACKET_LEN)) {
739 pr_debug("%s: rx error: len %u exceeds max size %lu\n",
> 740 dev->name, len, GOOD_PACKET_LEN);
741 dev->stats.rx_length_errors++;
742 goto err_xdp;
743 }
744
745 if (unlikely(hdr->hdr.gso_type))
746 goto err_xdp;
747
748 if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) {
749 int offset = buf - page_address(page) + header_offset;
750 unsigned int tlen = len + vi->hdr_len;
751 u16 num_buf = 1;
752
753 xdp_headroom = virtnet_get_headroom(vi);
754 header_offset = VIRTNET_RX_PAD + xdp_headroom;
755 headroom = vi->hdr_len + header_offset;
756 buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
757 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
758 xdp_page = xdp_linearize_page(rq, &num_buf, page,
759 offset, header_offset,
760 &tlen);
761 if (!xdp_page)
762 goto err_xdp;
763
764 buf = page_address(xdp_page);
765 put_page(page);
766 page = xdp_page;
767 }
768
769 xdp_init_buff(&xdp, buflen, &rq->xdp_rxq);
770 xdp_prepare_buff(&xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
771 xdp_headroom, len, true);
772 orig_data = xdp.data;
773 act = bpf_prog_run_xdp(xdp_prog, &xdp);
774 stats->xdp_packets++;
775
776 switch (act) {
777 case XDP_PASS:
778 /* Recalculate length in case bpf program changed it */
779 delta = orig_data - xdp.data;
780 len = xdp.data_end - xdp.data;
781 metasize = xdp.data - xdp.data_meta;
782 break;
783 case XDP_TX:
784 stats->xdp_tx++;
785 xdpf = xdp_convert_buff_to_frame(&xdp);
786 if (unlikely(!xdpf))
787 goto err_xdp;
788 err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
789 if (unlikely(!err)) {
790 xdp_return_frame_rx_napi(xdpf);
791 } else if (unlikely(err < 0)) {
792 trace_xdp_exception(vi->dev, xdp_prog, act);
793 goto err_xdp;
794 }
795 *xdp_xmit |= VIRTIO_XDP_TX;
796 rcu_read_unlock();
797 goto xdp_xmit;
798 case XDP_REDIRECT:
799 stats->xdp_redirects++;
800 err = xdp_do_redirect(dev, &xdp, xdp_prog);
801 if (err)
802 goto err_xdp;
803 *xdp_xmit |= VIRTIO_XDP_REDIR;
804 rcu_read_unlock();
805 goto xdp_xmit;
806 default:
807 bpf_warn_invalid_xdp_action(act);
808 fallthrough;
809 case XDP_ABORTED:
810 trace_xdp_exception(vi->dev, xdp_prog, act);
811 goto err_xdp;
812 case XDP_DROP:
813 goto err_xdp;
814 }
815 }
816 rcu_read_unlock();
817
818 if (unlikely(len > GOOD_PACKET_LEN)) {
819 pr_debug("%s: rx error: len %u exceeds max size %lu\n",
820 dev->name, len, GOOD_PACKET_LEN);
821 dev->stats.rx_length_errors++;
822 put_page(page);
823 return NULL;
824 }
825
826 skb = build_skb(buf, buflen);
827 if (!skb) {
828 put_page(page);
829 goto err;
830 }
831 skb_reserve(skb, headroom - delta);
832 skb_put(skb, len);
833 if (!xdp_prog) {
834 buf += header_offset;
835 memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
836 } /* keep zeroed vnet hdr since XDP is loaded */
837
838 if (metasize)
839 skb_metadata_set(skb, metasize);
840
841 err:
842 return skb;
843
844 err_xdp:
845 rcu_read_unlock();
846 stats->xdp_drops++;
847 stats->drops++;
848 put_page(page);
849 xdp_xmit:
850 return NULL;
851 }
852
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
在 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(+)