Re: [PATCH 2/3] hw/net/smc91c111: Sanitize packet length on tx

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

Hi Peter,

On 28/2/25 18:48, Peter Maydell wrote:
> When the smc91c111 transmits a packet, it must read a control byte
> which is at the end of the data area and CRC.  However, we don't
> sanitize the length field in the packet buffer, so if the guest sets
> the length field to something large we will try to read past the end
> of the packet data buffer when we access the control byte.
> 
> As usual, the datasheet says nothing about the behaviour of the
> hardware if the guest misprograms it in this way.  It says only that
> the maximum valid length is 2048 bytes.  We choose to log the guest
> error and silently drop the packet.
> 
> This requires us to factor out the "mark the tx packet as complete"
> logic, so we can call it for this "drop packet" case as well as at
> the end of the loop when we send a valid packet.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2742
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/smc91c111.c | 34 +++++++++++++++++++++++++++++-----
>   1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 2295c6acf25..23ca99f926a 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -22,6 +22,13 @@
>   
>   /* Number of 2k memory pages available.  */
>   #define NUM_PACKETS 4
> +/*
> + * Maximum size of a data frame, including the leading status word
> + * and byte count fields and the trailing CRC, last data byte
> + * and control byte (per figure 8-1 in the Microchip Technology

If control byte is included, ...

> + * LAN91C111 datasheet).
> + */
> +#define MAX_PACKET_SIZE 2048
>   
>   #define TYPE_SMC91C111 "smc91c111"
>   OBJECT_DECLARE_SIMPLE_TYPE(smc91c111_state, SMC91C111)
> @@ -240,6 +247,16 @@ static void smc91c111_release_packet(smc91c111_state *s, int packet)
>       smc91c111_flush_queued_packets(s);
>   }
>   
> +static void smc91c111_complete_tx_packet(smc91c111_state *s, int packetnum)
> +{
> +    if (s->ctr & CTR_AUTO_RELEASE) {
> +        /* Race?  */
> +        smc91c111_release_packet(s, packetnum);
> +    } else if (s->tx_fifo_done_len < NUM_PACKETS) {
> +        s->tx_fifo_done[s->tx_fifo_done_len++] = packetnum;
> +    }
> +}
> +
>   /* Flush the TX FIFO.  */
>   static void smc91c111_do_tx(smc91c111_state *s)
>   {
> @@ -263,6 +280,17 @@ static void smc91c111_do_tx(smc91c111_state *s)
>           *(p++) = 0x40;
>           len = *(p++);
>           len |= ((int)*(p++)) << 8;
> +        if (len >= MAX_PACKET_SIZE) {

isn't MAX_PACKET_SIZE valid? I'm not sure at all but I'd expect:

            if (len > MAX_PACKET_SIZE) {

> +            /*
> +             * Datasheet doesn't say what to do here, and there is no
> +             * relevant tx error condition listed. Log, and drop the packet.
> +             */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "smc91c111: tx packet with bad length %d, dropping\n",
> +                          len);
> +            smc91c111_complete_tx_packet(s, packetnum);
> +            continue;
> +        }
>           len -= 6;
>           control = p[len + 1];
>           if (control & 0x20)
> @@ -291,11 +319,7 @@ static void smc91c111_do_tx(smc91c111_state *s)
>               }
>           }
>   #endif
> -        if (s->ctr & CTR_AUTO_RELEASE)
> -            /* Race?  */
> -            smc91c111_release_packet(s, packetnum);
> -        else if (s->tx_fifo_done_len < NUM_PACKETS)
> -            s->tx_fifo_done[s->tx_fifo_done_len++] = packetnum;
> +        smc91c111_complete_tx_packet(s, packetnum);
>           qemu_send_packet(qemu_get_queue(s->nic), p, len);
>       }
>       s->tx_fifo_len = 0;