[PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
occurs while inserting the VLAN tag in packets whose length is less than
12 bytes, as (len-12) is passed to memmove() without proper checking.
This patch is intended to fix this issue by checking the minimum
Ethernet frame size during packet transmission.

Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
 hw/net/ftgmac100.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 043ba61b86..bcf4d84aea 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -238,6 +238,11 @@ typedef struct {
  */
 #define FTGMAC100_MAX_FRAME_SIZE    9220
 
+/*
+ * Min frame size
+ */
+#define FTGMAC100_MIN_FRAME_SIZE    64
+
 /* Limits depending on the type of the frame
  *
  *   9216 for Jumbo frames (+ 4 for VLAN)
@@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
         }
 
         len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
+
+        /* drop small packets */
+        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
+            len < FTGMAC100_MIN_FRAME_SIZE) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d bytes\n",
+                          __func__, len);
+            break;
+        }
+
         if (frame_size + len > sizeof(s->frame)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
                           __func__, len);
-- 
2.26.2

Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Peter Maydell]

On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>
> An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
> occurs while inserting the VLAN tag in packets whose length is less than
> 12 bytes, as (len-12) is passed to memmove() without proper checking.
> This patch is intended to fix this issue by checking the minimum
> Ethernet frame size during packet transmission.
>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>  hw/net/ftgmac100.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 043ba61b86..bcf4d84aea 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -238,6 +238,11 @@ typedef struct {
>   */
>  #define FTGMAC100_MAX_FRAME_SIZE    9220
>
> +/*
> + * Min frame size
> + */
> +#define FTGMAC100_MIN_FRAME_SIZE    64
> +
>  /* Limits depending on the type of the frame
>   *
>   *   9216 for Jumbo frames (+ 4 for VLAN)
> @@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>          }
>
>          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
> +
> +        /* drop small packets */
> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
> +            len < FTGMAC100_MIN_FRAME_SIZE) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d bytes\n",
> +                          __func__, len);
> +            break;
> +        }
> +

Andrew, Cedric: do you have the datasheet for this devic? Do you
know if we should also be flagging the error back to the
guest somehow?

I think a 'break' here means we'll never update the
descriptor flags to hand it back to the guest, which
is probably not what the hardware does.

thanks
-- PMM

Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

On Fri, Jul 10, 2020 at 1:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
> >
> > An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
> > occurs while inserting the VLAN tag in packets whose length is less than
> > 12 bytes, as (len-12) is passed to memmove() without proper checking.
> > This patch is intended to fix this issue by checking the minimum
> > Ethernet frame size during packet transmission.
> >
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >  hw/net/ftgmac100.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > index 043ba61b86..bcf4d84aea 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -238,6 +238,11 @@ typedef struct {
> >   */
> >  #define FTGMAC100_MAX_FRAME_SIZE    9220
> >
> > +/*
> > + * Min frame size
> > + */
> > +#define FTGMAC100_MIN_FRAME_SIZE    64
> > +
> >  /* Limits depending on the type of the frame
> >   *
> >   *   9216 for Jumbo frames (+ 4 for VLAN)
> > @@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
> >          }
> >
> >          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
> > +
> > +        /* drop small packets */
> > +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
> > +            len < FTGMAC100_MIN_FRAME_SIZE) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d bytes\n",
> > +                          __func__, len);
> > +            break;
> > +        }
> > +
>
> Andrew, Cedric: do you have the datasheet for this devic? Do you
> know if we should also be flagging the error back to the
> guest somehow?
>
> I think a 'break' here means we'll never update the
> descriptor flags to hand it back to the guest, which
> is probably not what the hardware does.
>
> thanks
> -- PMM
>

I thought of setting FTGMAC100_INT_XPKT_LOST, but not sure if this is
the most appropriate flag here.

Regards,
Mauro

Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Cédric Le Goater]

On 7/10/20 1:33 PM, Peter Maydell wrote:
> On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>>
>> An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
>> occurs while inserting the VLAN tag in packets whose length is less than
>> 12 bytes, as (len-12) is passed to memmove() without proper checking.
>> This patch is intended to fix this issue by checking the minimum
>> Ethernet frame size during packet transmission.
>>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> ---
>>  hw/net/ftgmac100.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 043ba61b86..bcf4d84aea 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -238,6 +238,11 @@ typedef struct {
>>   */
>>  #define FTGMAC100_MAX_FRAME_SIZE    9220
>>
>> +/*
>> + * Min frame size
>> + */
>> +#define FTGMAC100_MIN_FRAME_SIZE    64
>> +
>>  /* Limits depending on the type of the frame
>>   *
>>   *   9216 for Jumbo frames (+ 4 for VLAN)
>> @@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>          }
>>
>>          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
>> +
>> +        /* drop small packets */
>> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
>> +            len < FTGMAC100_MIN_FRAME_SIZE) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d bytes\n",
>> +                          __func__, len);
>> +            break;
>> +        }
>> +
> 
> Andrew, Cedric: do you have the datasheet for this devic? Do you
> know if we should also be flagging the error back to the
> guest somehow?

zero is the only invalid size of a transmit buffer and the specs does
not have any special information on which bit to raise in that case.

I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our 
best option and we should add an extra 'len == 0' test in front of 
the dma_memory_read() call to raise it. A zero length is not considered 
bogus by dma_memory_read() it seems. Is address zero considered bogus ? 
If not, we need to check that also. 

The code doing the memmove() should be protected by a check on the 
length, 'sizeof(struct eth_header)' or ETH_HLEN. That will fix the 
integer overflow. 

For clarity, we could replace 12 and 16 by a L2 header size: 
'sizeof(struct eth_header)' and 
'sizeof(struct eth_header) + sizeof(struct vlan_header)'. It can be
done later.

Thanks,

C. 


> I think a 'break' here means we'll never update the
> descriptor flags to hand it back to the guest, which
> is probably not what the hardware does.
> 
> thanks
> -- PMM
> 

Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Peter Maydell]

On Mon, 13 Jul 2020 at 15:19, Cédric Le Goater <clg@kaod.org> wrote:
> On 7/10/20 1:33 PM, Peter Maydell wrote:
> > Andrew, Cedric: do you have the datasheet for this device? Do you
> > know if we should also be flagging the error back to the
> > guest somehow?
>
> zero is the only invalid size of a transmit buffer and the specs does
> not have any special information on which bit to raise in that case.

I found a datasheet which might or might not be the equivalent
bit of hardware -- does your datasheet have a note on the
TXBUF_SIZE field of a tx descriptor that says "When the size is 0,
the descriptor would be discarded" ? (Though I found another
random doc that just says it's illegal...)

> I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our
> best option and we should add an extra 'len == 0' test in front of
> the dma_memory_read() call to raise it. A zero length is not considered
> bogus by dma_memory_read() it seems.

My best guess at "what the hardware does" here would be:
 * TXBUF_SIZE in a tx descriptor can be anything: the h/w
   would happily allow you to assemble a tx packet with a
   whole series of 1-byte sized buffers, each with its own
   tx descriptor
 * zero-byte tx descriptors might just be marked "done" and
   skipped over since they have no actual data
 * any checking on max/min lengths would be done
   only on the accumulated total-packet length (we do this
   this way already for the frame-too-big check)
 * I suspect "transmit buffer unavailable" means "the ethernet
   controller needs more data but the next tx descriptor
   is still marked as owned by the guest" -- this is certainly
   what we currently do with it, and that doesn't seem like
   the best thing to signal for the "tx packet too small"
   case. It's possible that the hardware simply sends out a
   runt packet of some form if the software tells it to do
   that. My vote would be for handling it with XPKT_LOST,
   the same way we do for over-large frames. This probably
   is not what the hardware does but at least it's a
   coherent thing that the guest might be expecting to have
   happen for a tx attempt and it matches the fact that we
   really are not going to put it on the 'wire'.

Side note: I suspect that any failures from
dma_memory_read() and dma_memory_write() should be
reported as AHB_ERR (currently we have a mix of
ignoring them or using NO_NPTXBUF).

(It would in theory be possible to test some of these edge
cases on real hardware, but that kind of bare-metal test
case is usually a pain to put together and way overkill
for this situation, so I don't think we should bother.)

> Is address zero considered bogus ?
> If not, we need to check that also.

Writes to address 0 are fine, it is not a special physical address.

thanks
-- PMM

Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Cédric Le Goater]

Sorry for the late answer.

On 7/13/20 6:15 PM, Peter Maydell wrote:
> On Mon, 13 Jul 2020 at 15:19, Cédric Le Goater <clg@kaod.org> wrote:
>> On 7/10/20 1:33 PM, Peter Maydell wrote:
>>> Andrew, Cedric: do you have the datasheet for this device? Do you
>>> know if we should also be flagging the error back to the
>>> guest somehow?
>>
>> zero is the only invalid size of a transmit buffer and the specs does
>> not have any special information on which bit to raise in that case.
> 
> I found a datasheet which might or might not be the equivalent
> bit of hardware -- does your datasheet have a note on the
> TXBUF_SIZE field of a tx descriptor that says "When the size is 0,
> the descriptor would be discarded" ? (Though I found another
> random doc that just says it's illegal...)

I only have : 

  TXBUF SIZE: Transmit buffer size in byte
  The transmit buffer size can not be zero.

>> I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our
>> best option and we should add an extra 'len == 0' test in front of
>> the dma_memory_read() call to raise it. A zero length is not considered
>> bogus by dma_memory_read() it seems.
> 
> My best guess at "what the hardware does" here would be:
>  * TXBUF_SIZE in a tx descriptor can be anything: the h/w
>    would happily allow you to assemble a tx packet with a
>    whole series of 1-byte sized buffers, each with its own
>    tx descriptor

yes. 

>  * zero-byte tx descriptors might just be marked "done" and
>    skipped over since they have no actual data

yes.

>  * any checking on max/min lengths would be done
>    only on the accumulated total-packet length (we do this
>    this way already for the frame-too-big check)

yes.

>  * I suspect "transmit buffer unavailable" means "the ethernet
>    controller needs more data but the next tx descriptor
>    is still marked as owned by the guest" -- this is certainly
>    what we currently do with it, and that doesn't seem like
>    the best thing to signal for the "tx packet too small"
>    case. It's possible that the hardware simply sends out a
>    runt packet of some form if the software tells it to do
>    that. My vote would be for handling it with XPKT_LOST,
>    the same way we do for over-large frames. 

XPKT_LOST means that packets are lost due to late/excessive 
collision. I have used this status for large frames because
not other bits made sense.

>    This probably
>    is not what the hardware does but at least it's a
>    coherent thing that the guest might be expecting to have
>    happen for a tx attempt and it matches the fact that we
>    really are not going to put it on the 'wire'.

I agree.

> 
> Side note: I suspect that any failures from
> dma_memory_read() and dma_memory_write() should be
> reported as AHB_ERR (currently we have a mix of
> ignoring them or using NO_NPTXBUF).

AHB_ERR is not used in the Aspeed implementation. I checked with 
them. But I think it makes sense for other implementations, so we
can add this status for Linux.

NO_NPTXBUF means a lack a transmit descriptors.


XPKT_LOST is our best choice then.

Thanks,

C. 

> (It would in theory be possible to test some of these edge
> cases on real hardware, but that kind of bare-metal test
> case is usually a pain to put together and way overkill
> for this situation, so I don't think we should bother.)
> 
>> Is address zero considered bogus ?
>> If not, we need to check that also.
> 
> Writes to address 0 are fine, it is not a special physical address.
> 
> thanks
> -- PMM
>