On 8/11/20 2:39 PM, Peter Maydell wrote: > On Thu, 6 Aug 2020 at 14:21, 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); >> + } >> } > > If you want to be picky, this will unnecessarily fail for the case of > a packet that is big enough for the vlan header but which has been > split up into multiple tx descriptors such that the first one is > smaller than the size of the eth_header. You could fix that by > doing the insertion of the vlan tag when you process the TXDES0_LTS > descriptor rather than when you process the TXDES0_FTS one. (We > already save the des1 info where the INS_VLANTAG flag is in the > 'flags' variable, so we have that available for the LTS descriptor code.)
yes. Good idea. The code is cleaner and the driver can even survive a bogus frame. I will send a new version, without the Tested and Reviewed tags. To reproduce, I have created a little kernel module tester based on the POC proposed by Ziming, which was for another MAC. https://github.com/legoater/ftgmac100-test Thanks, C.