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

[P J P <ppandit@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5374 bytes --]

  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.

===
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