Re: [PATCH] hw/net/xilinx_axienet: Don't write checksums off end of packet

On 6/7/26 18:27, Peter Maydell wrote:
> The xilinx_axienet device has ethernet checksum offloading, with a
> mode where the guest provides the offsets within the packet where
> the data to be checksummed starts, and where the final checksum
> should be written into the packet.
> 
> We don't sanity check the TX_CSINSERT offset before writing the
> checksum data into it, which means the guest can pass us a value that
> is larger than the packet itself and cause us to write the checksum
> off the end of the buffer.  We also don't explicitly check the
> TX_CSBEGIN offset; this doesn't currently cause any problems because
> we will pass a negative length to net_checksum_add() which does
> nothing, but it's a potential trap for the future if the type
> used for the length gets changed to be unsigned.
> 
> Explicitly check the offsets.  The datasheet doesn't say what happens
> if the guest misprograms this, so we choose to log an error and send
> the packet as-is.
> 
> Cc: [email protected]
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3599
> Signed-off-by: Peter Maydell <[email protected]>
> ---
>   hw/net/xilinx_axienet.c | 29 ++++++++++++++++++-----------
>   1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index 970732b162..a44dda5385 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -919,20 +919,27 @@ xilinx_axienet_data_stream_push(StreamSink *obj, uint8_t *buf, size_t size,
>       if (s->hdr[0] & 1) {
>           unsigned int start_off = s->hdr[1] >> 16;
>           unsigned int write_off = s->hdr[1] & 0xffff;
> -        uint32_t tmp_csum;
> -        uint16_t csum;
>   
> -        tmp_csum = net_checksum_add(s->txpos - start_off,
> -                                    buf + start_off);
> -        /* Accumulate the seed.  */
> -        tmp_csum += s->hdr[2] & 0xffff;
> +        if (start_off > s->txpos || write_off + 2 > s->txpos) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: offsets outside packet, skipping checksum\n",
> +                          TYPE_XILINX_AXI_ENET);
> +        } else {
> +            uint32_t tmp_csum;
> +            uint16_t csum;
>   
> -        /* Fold the 32bit partial checksum.  */
> -        csum = net_checksum_finish(tmp_csum);
> +            tmp_csum = net_checksum_add(s->txpos - start_off,
> +                                        buf + start_off);
> +            /* Accumulate the seed.  */
> +            tmp_csum += s->hdr[2] & 0xffff;
>   
> -        /* Writeback.  */
> -        buf[write_off] = csum >> 8;
> -        buf[write_off + 1] = csum & 0xff;
> +            /* Fold the 32bit partial checksum.  */
> +            csum = net_checksum_finish(tmp_csum);
> +
> +            /* Writeback.  */
> +            buf[write_off] = csum >> 8;
> +            buf[write_off + 1] = csum & 0xff;
> +        }
>       }

Output of 'git-diff --ignore-space-change' of this patch:

-- >8 --
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 9f5f65ecfac..d35f4a847d6 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -923,4 +923,10 @@ xilinx_axienet_data_stream_push(StreamSink *obj, 
uint8_t *buf, size_t size,
          unsigned int start_off = s->hdr[1] >> 16;
          unsigned int write_off = s->hdr[1] & 0xffff;
+
+        if (start_off > s->txpos || write_off + 2 > s->txpos) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: offsets outside packet, skipping 
checksum\n",
+                          TYPE_XILINX_AXI_ENET);
+        } else {
              uint32_t tmp_csum;
              uint16_t csum;
@@ -938,4 +944,5 @@ xilinx_axienet_data_stream_push(StreamSink *obj, 
uint8_t *buf, size_t size,
              buf[write_off + 1] = csum & 0xff;
          }
+    }

---

Queueing via hw-misc, thanks.