| Message ID | 20200806132106.747414-15-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: mostly cleanups and some extensions | expand |
On Thu, 6 Aug 2020 at 13:21, 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> Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > 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 >
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.
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.
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
On Thu, 6 Aug 2020 at 14:21, 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); > + } > } 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.) thanks -- PMM
On 8/11/20 2:39 PM, Peter Maydell wrote: > On Thu, 6 Aug 2020 at 14:21, 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); >> + } >> } > > 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.
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;
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(-)