[PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[Peter Maydell]

This patchset fixes some potential array overflows in the
smc91c111 ethernet device model, including the one found in
https://gitlab.com/qemu-project/qemu/-/issues/2742

There are two classes of bugs:
 * we accept packet numbers from the guest, but we were not
   validating that they were in range before using them as an
   index into the data[][] array
 * we didn't sanitize the length field read from the data
   frame on tx before using it as an index to find the
   control byte at the end of the frame, so we could read off
   the end of the buffer

This patchset fixes both of these. The datasheet is sadly
silent on the h/w behaviour for these errors, so I opted to
LOG_GUEST_ERROR and silently ignore the invalid operations.

Patch 3 tidies up the existing code to use a constant defined
in patch 2; I put it last so we can cc the first two patches
to stable without having to also backport that patch.

thanks
-- PMM

Peter Maydell (3):
  hw/net/smc91c111: Sanitize packet numbers
  hw/net/smc91c111: Sanitize packet length on tx
  hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers

 hw/net/smc91c111.c | 87 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/3] hw/net/smc91c111: Sanitize packet numbers

[Peter Maydell]

The smc91c111 uses packet numbers as an index into its internal
s->data[][] array. Valid packet numbers are between 0 and 3, but
the code does not generally check this, and there are various
places where the guest can hand us an arbitrary packet number
and cause an out-of-bounds access to the data array.

Add validation of packet numbers. The datasheet is not very
helpful about how guest errors like this should be handled:
it says nothing on the subject, and none of the documented
error conditions are relevant. We choose to log the situation
with LOG_GUEST_ERROR and silently ignore the attempted operation.

In the places where we are about to access the data[][] array
using a packet number and we know the number is valid because
we got it from somewhere that has already validated, we add
an assert() to document that belief.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/smc91c111.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 0e13dfa18b2..2295c6acf25 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -118,6 +118,11 @@ static const VMStateDescription vmstate_smc91c111 = {
 #define RS_TOOSHORT     0x0400
 #define RS_MULTICAST    0x0001
 
+static inline bool packetnum_valid(int packet_num)
+{
+    return packet_num >= 0 && packet_num < NUM_PACKETS;
+}
+
 /* Update interrupt status.  */
 static void smc91c111_update(smc91c111_state *s)
 {
@@ -218,6 +223,17 @@ static void smc91c111_pop_tx_fifo_done(smc91c111_state *s)
 /* Release the memory allocated to a packet.  */
 static void smc91c111_release_packet(smc91c111_state *s, int packet)
 {
+    if (!packetnum_valid(packet)) {
+        /*
+         * Data sheet doesn't document behaviour in this guest error
+         * case, and there is no error status register to report it.
+         * Log and ignore the attempt.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "smc91c111: attempt to release invalid packet %d\n",
+                      packet);
+        return;
+    }
     s->allocated &= ~(1 << packet);
     if (s->tx_alloc == 0x80)
         smc91c111_tx_alloc(s);
@@ -239,6 +255,8 @@ static void smc91c111_do_tx(smc91c111_state *s)
         return;
     for (i = 0; i < s->tx_fifo_len; i++) {
         packetnum = s->tx_fifo[i];
+        /* queue_tx checked the packet number was valid */
+        assert(packetnum_valid(packetnum));
         p = &s->data[packetnum][0];
         /* Set status word.  */
         *(p++) = 0x01;
@@ -287,6 +305,17 @@ static void smc91c111_do_tx(smc91c111_state *s)
 /* Add a packet to the TX FIFO.  */
 static void smc91c111_queue_tx(smc91c111_state *s, int packet)
 {
+    if (!packetnum_valid(packet)) {
+        /*
+         * Datasheet doesn't document behaviour in this error case, and
+         * there's no error status register we could report it in.
+         * Log and ignore.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "smc91c111: attempt to queue invalid packet %d\n",
+                      packet);
+        return;
+    }
     if (s->tx_fifo_len == NUM_PACKETS)
         return;
     s->tx_fifo[s->tx_fifo_len++] = packet;
@@ -457,6 +486,13 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
                     n = s->rx_fifo[0];
                 else
                     n = s->packet_num;
+                if (!packetnum_valid(n)) {
+                    /* Datasheet doesn't document what to do here */
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "smc91c111: attempt to write data to invalid packet %d\n",
+                                  n);
+                    return;
+                }
                 p = s->ptr & 0x07ff;
                 if (s->ptr & 0x4000) {
                     s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff);
@@ -605,6 +641,13 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
                     n = s->rx_fifo[0];
                 else
                     n = s->packet_num;
+                if (!packetnum_valid(n)) {
+                    /* Datasheet doesn't document what to do here */
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "smc91c111: attempt to read data from invalid packet %d\n",
+                                  n);
+                    return 0;
+                }
                 p = s->ptr & 0x07ff;
                 if (s->ptr & 0x4000) {
                     s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x07ff);
@@ -713,6 +756,8 @@ static ssize_t smc91c111_receive(NetClientState *nc, const uint8_t *buf, size_t
         return -1;
     s->rx_fifo[s->rx_fifo_len++] = packetnum;
 
+    /* allocate_packet() will not hand us back an invalid packet number */
+    assert(packetnum_valid(packetnum));
     p = &s->data[packetnum][0];
     /* ??? Multicast packets?  */
     status = 0;
-- 
2.43.0

Re: [PATCH 1/3] hw/net/smc91c111: Sanitize packet numbers

[Philippe Mathieu-Daudé]

On 28/2/25 18:47, Peter Maydell wrote:
> The smc91c111 uses packet numbers as an index into its internal
> s->data[][] array. Valid packet numbers are between 0 and 3, but
> the code does not generally check this, and there are various
> places where the guest can hand us an arbitrary packet number
> and cause an out-of-bounds access to the data array.
> 
> Add validation of packet numbers. The datasheet is not very
> helpful about how guest errors like this should be handled:
> it says nothing on the subject, and none of the documented
> error conditions are relevant. We choose to log the situation
> with LOG_GUEST_ERROR and silently ignore the attempted operation.
> 
> In the places where we are about to access the data[][] array
> using a packet number and we know the number is valid because
> we got it from somewhere that has already validated, we add
> an assert() to document that belief.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/smc91c111.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Peter Maydell]

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
+ * 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) {
+            /*
+             * 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;
-- 
2.43.0

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

[Philippe Mathieu-Daudé]

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;

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

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé]

On 10/3/25 12:06, Peter Maydell wrote:
> 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.

OK, so using '>':
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> (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.)

While agreeing it is not worth fixing ASAP, a patch adding a pair
of comments with your findings could save time to future contributors
and not make your analysis in vain :)

Regards,

Phil.

[PATCH 3/3] hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers

[Peter Maydell]

Now we have a constant for the maximum packet size, we can use it
to replace various hardcoded 2048 values.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/smc91c111.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 23ca99f926a..4a8f867d96e 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -58,7 +58,7 @@ struct smc91c111_state {
     int tx_fifo_done_len;
     int tx_fifo_done[NUM_PACKETS];
     /* Packet buffer memory.  */
-    uint8_t data[NUM_PACKETS][2048];
+    uint8_t data[NUM_PACKETS][MAX_PACKET_SIZE];
     uint8_t int_level;
     uint8_t int_mask;
     MemoryRegion mmio;
@@ -86,7 +86,8 @@ static const VMStateDescription vmstate_smc91c111 = {
         VMSTATE_INT32_ARRAY(rx_fifo, smc91c111_state, NUM_PACKETS),
         VMSTATE_INT32(tx_fifo_done_len, smc91c111_state),
         VMSTATE_INT32_ARRAY(tx_fifo_done, smc91c111_state, NUM_PACKETS),
-        VMSTATE_BUFFER_UNSAFE(data, smc91c111_state, 0, NUM_PACKETS * 2048),
+        VMSTATE_BUFFER_UNSAFE(data, smc91c111_state, 0,
+                              NUM_PACKETS * MAX_PACKET_SIZE),
         VMSTATE_UINT8(int_level, smc91c111_state),
         VMSTATE_UINT8(int_mask, smc91c111_state),
         VMSTATE_END_OF_LIST()
@@ -773,8 +774,9 @@ static ssize_t smc91c111_receive(NetClientState *nc, const uint8_t *buf, size_t
     if (crc)
         packetsize += 4;
     /* TODO: Flag overrun and receive errors.  */
-    if (packetsize > 2048)
+    if (packetsize > MAX_PACKET_SIZE) {
         return -1;
+    }
     packetnum = smc91c111_allocate_packet(s);
     if (packetnum == 0x80)
         return -1;
-- 
2.43.0

Re: [PATCH 3/3] hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers

[Philippe Mathieu-Daudé]

On 28/2/25 18:48, Peter Maydell wrote:
> Now we have a constant for the maximum packet size, we can use it
> to replace various hardcoded 2048 values.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/smc91c111.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[Peter Maydell]

On Fri, 28 Feb 2025 at 17:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes some potential array overflows in the
> smc91c111 ethernet device model, including the one found in
> https://gitlab.com/qemu-project/qemu/-/issues/2742
>
> There are two classes of bugs:
>  * we accept packet numbers from the guest, but we were not
>    validating that they were in range before using them as an
>    index into the data[][] array
>  * we didn't sanitize the length field read from the data
>    frame on tx before using it as an index to find the
>    control byte at the end of the frame, so we could read off
>    the end of the buffer
>
> This patchset fixes both of these. The datasheet is sadly
> silent on the h/w behaviour for these errors, so I opted to
> LOG_GUEST_ERROR and silently ignore the invalid operations.
>
> Patch 3 tidies up the existing code to use a constant defined
> in patch 2; I put it last so we can cc the first two patches
> to stable without having to also backport that patch.

See also the other smc91c111 fuzzer fix patch:
https://patchew.org/QEMU/20250228191652.1957208-1-peter.maydell@linaro.org/

(if I need to do a v2 of this series I'll put that one in too)

-- PMM

Re: [PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[Peter Maydell]

Ping for review of this series (and the other smc91c111
patch I link below), please?

thanks
-- PMM


On Fri, 28 Feb 2025 at 19:22, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 28 Feb 2025 at 17:48, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > This patchset fixes some potential array overflows in the
> > smc91c111 ethernet device model, including the one found in
> > https://gitlab.com/qemu-project/qemu/-/issues/2742
> >
> > There are two classes of bugs:
> >  * we accept packet numbers from the guest, but we were not
> >    validating that they were in range before using them as an
> >    index into the data[][] array
> >  * we didn't sanitize the length field read from the data
> >    frame on tx before using it as an index to find the
> >    control byte at the end of the frame, so we could read off
> >    the end of the buffer
> >
> > This patchset fixes both of these. The datasheet is sadly
> > silent on the h/w behaviour for these errors, so I opted to
> > LOG_GUEST_ERROR and silently ignore the invalid operations.
> >
> > Patch 3 tidies up the existing code to use a constant defined
> > in patch 2; I put it last so we can cc the first two patches
> > to stable without having to also backport that patch.
>
> See also the other smc91c111 fuzzer fix patch:
> https://patchew.org/QEMU/20250228191652.1957208-1-peter.maydell@linaro.org/
>
> (if I need to do a v2 of this series I'll put that one in too)

Re: [PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[Philippe Mathieu-Daudé]

Hi Peter, Jason,

On 28/2/25 18:47, Peter Maydell wrote:
> This patchset fixes some potential array overflows in the
> smc91c111 ethernet device model, including the one found in
> https://gitlab.com/qemu-project/qemu/-/issues/2742
> 
> There are two classes of bugs:
>   * we accept packet numbers from the guest, but we were not
>     validating that they were in range before using them as an
>     index into the data[][] array
>   * we didn't sanitize the length field read from the data
>     frame on tx before using it as an index to find the
>     control byte at the end of the frame, so we could read off
>     the end of the buffer
> 
> This patchset fixes both of these. The datasheet is sadly
> silent on the h/w behaviour for these errors, so I opted to
> LOG_GUEST_ERROR and silently ignore the invalid operations.
> 
> Patch 3 tidies up the existing code to use a constant defined
> in patch 2; I put it last so we can cc the first two patches
> to stable without having to also backport that patch.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>    hw/net/smc91c111: Sanitize packet numbers
>    hw/net/smc91c111: Sanitize packet length on tx
>    hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers

Since Jason just sent his network pull request, I'll take these
patches via my hw-misc tree (with patch #2 fixed up), except if
one of you object.

Thanks,

Phil.

Re: [PATCH 0/3] hw/net/smc91c111: Fix potential array overflows

[Jason Wang]

On Tue, Mar 11, 2025 at 5:00 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Peter, Jason,
>
> On 28/2/25 18:47, Peter Maydell wrote:
> > This patchset fixes some potential array overflows in the
> > smc91c111 ethernet device model, including the one found in
> > https://gitlab.com/qemu-project/qemu/-/issues/2742
> >
> > There are two classes of bugs:
> >   * we accept packet numbers from the guest, but we were not
> >     validating that they were in range before using them as an
> >     index into the data[][] array
> >   * we didn't sanitize the length field read from the data
> >     frame on tx before using it as an index to find the
> >     control byte at the end of the frame, so we could read off
> >     the end of the buffer
> >
> > This patchset fixes both of these. The datasheet is sadly
> > silent on the h/w behaviour for these errors, so I opted to
> > LOG_GUEST_ERROR and silently ignore the invalid operations.
> >
> > Patch 3 tidies up the existing code to use a constant defined
> > in patch 2; I put it last so we can cc the first two patches
> > to stable without having to also backport that patch.
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (3):
> >    hw/net/smc91c111: Sanitize packet numbers
> >    hw/net/smc91c111: Sanitize packet length on tx
> >    hw/net/smc91c111: Use MAX_PACKET_SIZE instead of magic numbers
>
> Since Jason just sent his network pull request, I'll take these
> patches via my hw-misc tree (with patch #2 fixed up), except if
> one of you object.
>
> Thanks,
>
> Phil.
>

Fine with me.

Thanks