Re: [PATCH] net: check payload length limit for all frames

[Jason Wang]

On 2020/7/17 下午1:06, P J P wrote:
>    Hello Jason, all
>
> +-- On Fri, 17 Jul 2020, Jason Wang wrote --+
> | On 2020/7/17 上午9:21, Alexander Bulekov wrote:
> | > On 200717 0853, Li Qiang wrote:
> | >> Which issue are you trying to solve, any reference linking?
> | >> I also send a patch related this part and also a UAF.
> | >
> | > I reported a UAF privately to QEMU-Security in May. I believe the one Li
> | > is referring to is this one https://bugs.launchpad.net/qemu/+bug/1886362
> | >
> | > When I saw Prasad's email, I was worried that I reported the same bug
> | > twice, but I can still reproduce LP#1886362 with Prasad's patch.
> | >
> | > On the other hand, I cannot reproduce either issue with Li's patch:
> | > Message-Id: <20200716161453.61295-1-liq3ea@163.com>
> | >
> | > Based on this, I think there were two distinct issues.
>
>    Yes, LP#1886362 looks different that the one fixed here.
>
> | Could you describe the issue you saw in details? (E.g the calltrace?) The
> | commit log does not explain where we can get OOB or UAF.
>
> I should've included the backtrace in the commit message. It crossed my mind
> after I sent the patch. Sorry.


Thanks but I don't see a direct relation between 64K limit and this 
calltrace.

Maybe you can elaborate more on this?

Thanks


> ===
> 1040323==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000344d8 
> at pc 0x5571b8fb9ce7 bp 0x7ffede5a2290 sp 0x7ffede5a2280
> READ of size 8 at 0x6060000344d8 thread T0
>      #0 0x5571b8fb9ce6 in e1000e_write_packet_to_guest hw/net/e1000e_core.c:1587
>      #1 0x5571b8fba8fc in e1000e_receive_iov hw/net/e1000e_core.c:1709
>      #2 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213
>      #3 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544
>      #4 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620
>      #5 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633
>      #6 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664
>      #7 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743
>      #8 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934
>      #9 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451
>      #10 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265
>      #11 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109
>      ...
> 0x6060000344d8 is located 24 bytes inside of 64-byte region 
> [0x6060000344c0,0x606000034500)
> freed by thread T0 here:
>      #0 0x7f89e3b87307 in __interceptor_free (/lib64/libasan.so.6+0xb0307)
>      #1 0x7f89e37c7a6c in g_free (/lib64/libglib-2.0.so.0+0x58a6c)
>      #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103
>      #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158
>      #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695
>      #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213
>      #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544
>      #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620
>      #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633
>      #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664
>      #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743
>      #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934
>      #12 0x5571b8fbe011 in e1000e_set_tctl hw/net/e1000e_core.c:2431
>      #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265
>      #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109
>      ...
> previously allocated by thread T0 here:
>      #0 0x7f89e3b87667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667)
>      #1 0x7f89e37c7978 in g_malloc (/lib64/libglib-2.0.so.0+0x58978)
>      #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103
>      #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158
>      #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695
>      #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213
>      #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544
>      #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620
>      #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633
>      #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664
>      #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743
>      #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934
>      #12 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451
>      #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265
>      #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109
> ===
>   
> | >>> --- a/hw/net/net_tx_pkt.c
> | >>> +++ b/hw/net/net_tx_pkt.c
> | >>> @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt,
> | >>> NetClientState *nc)
> | >>>        * Since underlying infrastructure does not support IP datagrams
> | >>>        longer
> | >>>        * than 64K we should drop such packets and don't even try to send
> | >>>        */
> | >>> -    if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) {
> | >>> -        if (pkt->payload_len >
> | >>> -            ETH_MAX_IP_DGRAM_LEN -
> | >>> -            pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) {
> | >>> -            return false;
> | >>> -        }
> | >>> +    if (pkt->payload_len >
> | >>> +        ETH_MAX_IP_DGRAM_LEN -
> | >>> +        pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) {
> | >>> +        return false;
> | >>>       }
>
> Nevertheless, checking 'payload_len' for all packets irrespective of the
> 'gso_type' does seem reasonable.
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D