Re: [RFC PATCH-for-9.0?] hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 10/4/24 08:53, Akihiko Odaki wrote:
> On 2024/04/10 3:04, Philippe Mathieu-Daudé wrote:
>> If a fragmented packet size is too short, do not try to
>> calculate its checksum.
>>
>> Reproduced using:
>>
>>    $ cat << EOF | qemu-system-i386 -display none -nodefaults \
>>                                    -machine q35,accel=qtest -m 32M \
>>                                    -device igb,netdev=net0 \
>>                                    -netdev user,id=net0 \
>>                                    -qtest stdio
>>    outl 0xcf8 0x80000810
>>    outl 0xcfc 0xe0000000
>>    outl 0xcf8 0x80000804
>>    outw 0xcfc 0x06
>>    write 0xe0000403 0x1 0x02
>>    writel 0xe0003808 0xffffffff
>>    write 0xe000381a 0x1 0x5b
>>    write 0xe000381b 0x1 0x00
>>    EOF
>>    Assertion failed: (offset == 0), function iov_from_buf_full, file 
>> util/iov.c, line 39.
>>    #1 0x5575e81e952a in iov_from_buf_full qemu/util/iov.c:39:5
>>    #2 0x5575e6500768 in net_tx_pkt_update_sctp_checksum 
>> qemu/hw/net/net_tx_pkt.c:144:9
>>    #3 0x5575e659f3e1 in igb_setup_tx_offloads 
>> qemu/hw/net/igb_core.c:478:11
>>    #4 0x5575e659f3e1 in igb_tx_pkt_send qemu/hw/net/igb_core.c:552:10
>>    #5 0x5575e659f3e1 in igb_process_tx_desc qemu/hw/net/igb_core.c:671:17
>>    #6 0x5575e659f3e1 in igb_start_xmit qemu/hw/net/igb_core.c:903:9
>>    #7 0x5575e659f3e1 in igb_set_tdt qemu/hw/net/igb_core.c:2812:5
>>    #8 0x5575e657d6a4 in igb_core_write qemu/hw/net/igb_core.c:4248:9
>>
>> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
>> Fixes: f199b13bc1 ("igb: Implement Tx SCTP CSO")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2273
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> No clue this makes sense, but avoids the crash...
>> ---
>>   hw/net/net_tx_pkt.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>> index 2134a18c4c..6a8640157f 100644
>> --- a/hw/net/net_tx_pkt.c
>> +++ b/hw/net/net_tx_pkt.c
>> @@ -141,6 +141,10 @@ bool net_tx_pkt_update_sctp_checksum(struct 
>> NetTxPkt *pkt)
>>       uint32_t csum = 0;
>>       struct iovec *pl_start_frag = pkt->vec + NET_TX_PKT_PL_START_FRAG;
>> +    if (iov_size(pl_start_frag, pkt->payload_frags) < sizeof(csum)) {
>> +        return false;
>> +    }
>> +
> 
> iov_from_buf() uses 8 as offset, so we should check if the size >= 8 + 
> sizeof(csum).

Indeed. I'll respin a v2.

> However I doubt that it is worth to fix. I think it is fine to remove 
> the assertion (i.e., remove the requirement that the offset specified 
> for iov_from_buf() must be inside iov and instead let the function() 
> return 0 in that case).

update_sctp_checksum() seems sensible enough to be fixed for 9.0.

Reworking iov_from_buf() is for 9.1. I'll let someone else do that.

Thanks for the quick review!

> Regards,
> Akihiko Odaki
> 
>>       if (iov_from_buf(pl_start_frag, pkt->payload_frags, 8, &csum, 
>> sizeof(csum)) < sizeof(csum)) {
>>           return false;
>>       }