On Wed, 20 Jul 2022 at 10:04, Jason Wang <jasow...@redhat.com> wrote: > > From: Zhang Chen <chen.zh...@intel.com> > > When COLO use only one vnet_hdr_support parameter between > filter-redirector and filter-mirror(or colo-compare), COLO will crash > with segmentation fault. Back track as follow: > > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault. > 0x0000555555cb200b in eth_get_l2_hdr_length (p=0x0) > at /home/tao/project/COLO/colo-qemu/include/net/eth.h:296 > 296 uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(p)->h_proto); > (gdb) bt > 0 0x0000555555cb200b in eth_get_l2_hdr_length (p=0x0) > at /home/tao/project/COLO/colo-qemu/include/net/eth.h:296 > 1 0x0000555555cb22b4 in parse_packet_early (pkt=0x555556a44840) at > net/colo.c:49 > 2 0x0000555555cb2b91 in is_tcp_packet (pkt=0x555556a44840) at > net/filter-rewriter.c:63 > > So wrong vnet_hdr_len will cause pkt->data become NULL. Add check to > raise error and add trace-events to track vnet_hdr_len. > > Signed-off-by: Tao Xu <tao3...@intel.com> > Signed-off-by: Zhang Chen <chen.zh...@intel.com> > Reviewed-by: Li Zhijian <lizhij...@fujitsu.com> > Signed-off-by: Jason Wang <jasow...@redhat.com>
Hi -- Coverity points out that there's a problem with this fix (CID 1490786): > @@ -46,7 +46,14 @@ int parse_packet_early(Packet *pkt) > static const uint8_t vlan[] = {0x81, 0x00}; > uint8_t *data = pkt->data + pkt->vnet_hdr_len; data here is set to pkt->data + pkt->vnet_hdr_len. If pkt->data is NULL then this addition is C undefined behaviour, and if pkt->data is not NULL but the integer added is such that the pointer ends up not pointing within data, then that is also C undefined behaviour... > uint16_t l3_proto; > - ssize_t l2hdr_len = eth_get_l2_hdr_length(data); > + ssize_t l2hdr_len; > + > + if (data == NULL) { ...so the compiler is allowed to assume that data cannot be NULL here, and this entire error checking clause is logically dead code. If you're trying to check whether vnet_hdr_len is valid, you need to do that as an integer arithmetic check before you start using it for pointer arithmetic. And you probably want to be checking against some kind of bound, not just for whether the result is going to be NULL. Overall this looks kinda sketchy and could probably use more detailed code review. Where does the bogus vnet_hdr_len come from in the first place? Is this data from the guest, or from the network (ie uncontrolled)? > + trace_colo_proxy_main_vnet_info("This packet is not parsed > correctly, " > + "pkt->vnet_hdr_len", > pkt->vnet_hdr_len); > + return 1; > + } thanks -- PMM