RE: [PULL V2 25/25] net/colo.c: fix segmentation fault when packet is not parsed correctly

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, August 1, 2022 12:18 PM
> To: Zhang, Chen <chen.zhang@intel.com>; Xu, Tao3 <tao3.xu@intel.com>
> Cc: qemu-devel@nongnu.org; Li Zhijian <lizhijian@fujitsu.com>; Peter
> Maydell <peter.maydell@linaro.org>
> Subject: Re: [PULL V2 25/25] net/colo.c: fix segmentation fault when packet
> is not parsed correctly
> 
> 
> 在 2022/7/29 21:58, Peter Maydell 写道:
> > On Wed, 20 Jul 2022 at 10:04, Jason Wang <jasowang@redhat.com> wrote:
> >> From: Zhang Chen <chen.zhang@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.xu@intel.com>
> >> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> >> Signed-off-by: Jason Wang <jasowang@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...
> 
> 
> Right. And I don't see how pkt->data can be NULL, maybe a hint of bug
> elsewhere.
> 
> 
> >
> >>       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.
> 
> 
> Or we can let the caller to validate the Pkt before calling this function.
> 
> 
> >
> > 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)?
> 
> 
> If I understand correctly the vnet_hdr_len is set during handshake
> (net_fill_rstate()) which is sent from a network backend.
> 
> Chen or Xu, please post a fix and explain what happens in the changelog.

OK, we will send a patch to fix it.

Thanks
Chen

> 
> Thanks
> 
> 
> >
> >> +        trace_colo_proxy_main_vnet_info("This packet is not parsed
> correctly, "
> >> +                                        "pkt->vnet_hdr_len", 
> >> pkt->vnet_hdr_len);
> >> +        return 1;
> >> +    }
> > thanks
> > -- PMM
> >