hw/net/xilinx_axienet: Don't write checksums off end of packet

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

Posted by Peter Maydell 2 weeks, 5 days ago

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: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3599
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 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;
+        }
     }
 
     qemu_send_packet(qemu_get_queue(s->nic), buf, s->txpos);
-- 
2.43.0

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

Posted by Philippe Mathieu-Daudé 3 days, 3 hours ago

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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3599
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/xilinx_axienet.c | 29 ++++++++++++++++++-----------
>   1 file changed, 18 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

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

Posted by Alistair 4 days, 9 hours ago

On Mon, 2026-07-06 at 17:27 +0100, 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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3599
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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;
> +        }
>      }
>  
>      qemu_send_packet(qemu_get_queue(s->nic), buf, s->txpos);

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

Posted by Peter Maydell 6 days, 1 hour ago

Ping for review, please?

thanks
-- PMM

On Mon, 6 Jul 2026 at 17:27, Peter Maydell <peter.maydell@linaro.org> 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: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3599
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  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;
> +        }
>      }
>
>      qemu_send_packet(qemu_get_queue(s->nic), buf, s->txpos);
> --
> 2.43.0