Re: [Qemu-devel] [PATCH 1/5] hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer overrun

[Dr. David Alan Gilbert]

* Peter Maydell (address@hidden) wrote:
> The current tx_fifo code has a corner case where the guest can overrun
> the fifo buffer: if automatic CRCs are disabled we allow the guest to write
> the CRC word even if there isn't actually space for it in the FIFO.
> The datasheet is unclear about exactly how the hardware deals with this
> situation; the most plausible answer seems to be that the CRC word is
> just lost.
> 
> Implement this fix by separating the "can we stuff another word in the
> FIFO" logic from the "should we transmit the packet now" check. This
> also moves us closer to the real hardware, which has a number of ways
> it can be configured to trigger sending the packet, some of which we
> don't implement.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> ---
>  hw/net/stellaris_enet.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d04e6a4..bd844cd 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -253,10 +253,12 @@ static void stellaris_enet_write(void *opaque, hwaddr 
> offset,
>                  s->tx_fifo[s->tx_fifo_len++] = value >> 24;
>              }
>          } else {
> -            s->tx_fifo[s->tx_fifo_len++] = value;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> +            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> +                s->tx_fifo[s->tx_fifo_len++] = value;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> +            }
>              if (s->tx_fifo_len >= s->tx_frame_len) {
>                  /* We don't implement explicit CRC, so just chop it off.  */
>                  if ((s->tctl & SE_TCTL_CRC) == 0)
> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK