| Message ID | 20250228174802.1945417-3-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/smc91c111: Fix potential array overflows | expand |
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;
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
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.
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;
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(-)