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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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).

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).

Regards,
Akihiko Odaki

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