Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

On Mon, Aug 10, 2020 at 7:14 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/10/20 3:43 PM, Mauro Matteo Cascella wrote:
> > On Thu, Aug 6, 2020 at 3:21 PM Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> When inserting the VLAN tag in packets, memmove() can generate an
> >> integer overflow for packets whose length is less than 12 bytes.
> >>
> >> Check length against the size of the ethernet header (14 bytes) to
> >> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
> >> like a good modeling choice even if Aspeed does not specify anything
> >> in that case.
> >>
> >> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
> >> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> >> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/net/ftgmac100.c | 19 +++++++++++++++----
> >>  1 file changed, 15 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> >> index 280aa3d3a1e2..987b843fabc4 100644
> >> --- a/hw/net/ftgmac100.c
> >> +++ b/hw/net/ftgmac100.c
> >> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, 
> >> uint32_t tx_ring,
> >>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
> >>                  len =  sizeof(s->frame) - frame_size - 4;
> >>              }
> >> -            memmove(ptr + 16, ptr + 12, len - 12);
> >> -            stw_be_p(ptr + 12, ETH_P_VLAN);
> >> -            stw_be_p(ptr + 14, bd.des1);
> >> -            len += 4;
> >> +
> >> +            if (len < sizeof(struct eth_header)) {
> >> +                qemu_log_mask(LOG_GUEST_ERROR,
> >> +                         "%s: frame too small for VLAN insertion : %d 
> >> bytes\n",
> >> +                         __func__, len);
> >> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> >> +            } else {
> >> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
> >> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
> >> +
> >> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
> >> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
> >> +                stw_be_p(vlan_hdr + 2, 
> >> FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
> >> +                len += sizeof(struct vlan_header);
> >> +            }
> >>          }
> >>
> >>          ptr += len;
> >> --
> >> 2.25.4
> >>
> >
> > I can confirm that I can't reproduce the issue with the above patch. Thank 
> > you.
> >
>
> Can I count that as a Tested-by ?
>
> Thanks,
>
> C.
>
>

Sure. I wonder whether we should make 'len' unsigned, though I think
it doesn't really matter due to FTGMAC100_TXDES0_TXBUF_SIZE. What do
you think?

Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>


--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0