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

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 9 Mar 2025 at 19:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 28/2/25 18:48, Peter Maydell wrote:
> > --- 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) {

Yes, thanks, good catch. The max value in the byte count
field is 2048. We subtract 6, and then look at p[len + 1],
which will be p[2048 - 6 + 1] = p[2043], where the value of
p is data+4 (because we incremented it 4 times as we dealt
with the status and byte count fields).
So p[2043] is data[2047], which is the last in-bounds byte,
and a byte-count field of 2048 is not an overrun.

(Also, I just noticed that the data sheet says that for tx
frames the transmit byte count least significant bit will be
assumed 0 by the controller regardless of the value written
in memory. So we ought to zero out the LSB of 'len' after we
read it from the packet. That's not an overflow, though
(since we already subtracted 6 from len), just a bug...
Plus it looks like we don't handle the case of "odd-length
frame and CRC field present" right, since we don't do anything
about the last-data-byte being behind the CRC field. I think
that given the unimportance of this device model I'll settle
for just fixing the overruns and leave these other nominal
bugs alone.)

thanks
-- PMM