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

[Jason Wang <jasowang@redhat.com>]

On 2020/7/17 上午9:21, Alexander Bulekov wrote:
> On 200717 0853, Li Qiang wrote:
>> P J P <ppandit@redhat.com> 于2020年7月17日周五 上午3:26写道：
>>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>>
>>> While sending packets, the check that packet 'payload_len'
>>> is within 64kB limit, seems to happen only for GSO frames.
>>> It may lead to use-after-free or out-of-bounds access like
>>> issues when sending non-GSO frames. Check the 'payload_len'
>>> limit for all packets, irrespective of the gso type.
>>>
>> Hello Prasad,
>> Which issue are you trying to solve, any reference linking?
>>
>> I also send a patch related this part and also a UAF.
>>
>> Thanks,
>> Li Qiang
> Hi Li, Prasad,
> 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. Both of the
> crashes rely on e1000e tx loopback into e1000e MMIO. Since Li's
> patch adds a TX bh, it seems to mitigate such types of issues.
>
> Sorry about any confusion.
> -Alex


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.

Thanks


>
>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>>> ---
>>>   hw/net/net_tx_pkt.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>>> index 162f802dd7..e66998a8f9 100644
>>> --- 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;
>>>       }
>>>
>>>       if (pkt->has_virt_hdr ||
>>> --
>>> 2.26.2
>>>
>>>