On Mon, Aug 10, 2020 at 7:14 PM Cédric Le Goater <c...@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 <c...@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.frede...@yahoo.fr>
> >> Cc: Mauro Matteo Cascella <mcasc...@redhat.com>
> >> Reported-by: Ziming Zhang <ezrak...@gmail.com>
> >> Signed-off-by: Cédric Le Goater <c...@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 <mcasc...@redhat.com>
--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8 1A9D 4E55 23B8 BB34 10B0
