| Message ID | 1396275242-10810-16-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Michael S. Tsirkin (mst@redhat.com) wrote: > CVE-2013-4532 > > s->tx_frame_len is read from the wire and can later used as an index > into s->tx_fifo[] for memset() when a DATA command is issued by the guest. > > In this case s->tx_frame_len is checked to avoid an overrun, but if the > value is negative a subsequently executed guest can underrun the buffer > with zeros via the memset() call. > > Additionally, tx_frame_len is used to validate that tx_fifo_len > doesn't exceed the fifo bounds - the assumption being that data model > never makes it exceed 2032. > > Fix this by failing migration if the incoming value of s->tx_frame_len > is less than -1 (the emulation code allows from -1 as a special case) > or if it exceeds 2032. > > Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/net/stellaris_enet.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c > index aed00fd..90ff950 100644 > --- a/hw/net/stellaris_enet.c > +++ b/hw/net/stellaris_enet.c > @@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) > s->mtxd = qemu_get_be32(f); > s->mrxd = qemu_get_be32(f); > s->np = qemu_get_be32(f); > - s->tx_frame_len = qemu_get_be32(f); > + v = qemu_get_be32(f); > + if (v < -1 || s->tx_frame_len > 2032) { > + return -EINVAL; > + } > + s->tx_frame_len = v; I think that's wrong; 'stellaris_enet_write' does the following: s->tx_frame_len = value & 0xffff; if (s->tx_frame_len > 2032) { DPRINTF("TX frame too long (%d)\n", s->tx_frame_len); s->tx_frame_len = 0; s->ris |= SE_INT_TXER; stellaris_enet_update(s); } else { DPRINTF("Start TX frame len=%d\n", s->tx_frame_len); /* The value written does not include the ethernet header. */ s->tx_frame_len += 14; if ((s->tctl & SE_TCTL_CRC) == 0) s->tx_frame_len += 4; So lets say that tx_frame_len is initially 2032 when written; 14 is added to it at this point, and if the CRC flag is set then another 4. Thus it seems a user can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying given the buffer is only 2048 bytes. Dave > v = qemu_get_be32(f); > /* How many bytes does data use in tx fifo. */ > sz = s->tx_frame_len == -1 ? 2 : 4; > -- > MST > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > So lets say that tx_frame_len is initially 2032 when written; 14 is added to it > at this point, and if the CRC flag is set then another 4. Thus it seems a user > can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying > given the buffer is only 2048 bytes. Yep, see my equivalent remarks in the other patch. Michael -- can we please squash these two patches into one? It's really hard to review the code for correctness when half the logic for dealing with the tx fifo is in a different patch... thanks -- PMM
On 03/31/2014 08:16 AM, Michael S. Tsirkin wrote:
> CVE-2013-4532
s/orerrun/overrun/ in the subject
On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote: > On 1 April 2014 10:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > So lets say that tx_frame_len is initially 2032 when written; 14 is added to it > > at this point, and if the CRC flag is set then another 4. Thus it seems a user > > can set the value in tx_frame_len to 2032+14+4=2050 - which is a bit worrying > > given the buffer is only 2048 bytes. > > > Yep, see my equivalent remarks in the other patch. > > Michael -- can we please squash these two patches into one? > It's really hard to review the code for correctness when > half the logic for dealing with the tx fifo is in a > different patch... > > thanks > -- PMM Will do - part 1 as well?
On 1 April 2014 16:22, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Apr 01, 2014 at 11:06:48AM +0100, Peter Maydell wrote: >> Michael -- can we please squash these two patches into one? >> It's really hard to review the code for correctness when >> half the logic for dealing with the tx fifo is in a >> different patch... > Will do - part 1 as well? No, that's OK, it's dealing with the rx logic rather than the tx logic. However, I've been writing some patches this afternoon which get rid of tx_frame_len completely, so you might want to wait for those... thanks -- PMM
diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c index aed00fd..90ff950 100644 --- a/hw/net/stellaris_enet.c +++ b/hw/net/stellaris_enet.c @@ -373,7 +373,11 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id) s->mtxd = qemu_get_be32(f); s->mrxd = qemu_get_be32(f); s->np = qemu_get_be32(f); - s->tx_frame_len = qemu_get_be32(f); + v = qemu_get_be32(f); + if (v < -1 || s->tx_frame_len > 2032) { + return -EINVAL; + } + s->tx_frame_len = v; v = qemu_get_be32(f); /* How many bytes does data use in tx fifo. */ sz = s->tx_frame_len == -1 ? 2 : 4;
CVE-2013-4532 s->tx_frame_len is read from the wire and can later used as an index into s->tx_fifo[] for memset() when a DATA command is issued by the guest. In this case s->tx_frame_len is checked to avoid an overrun, but if the value is negative a subsequently executed guest can underrun the buffer with zeros via the memset() call. Additionally, tx_frame_len is used to validate that tx_fifo_len doesn't exceed the fifo bounds - the assumption being that data model never makes it exceed 2032. Fix this by failing migration if the incoming value of s->tx_frame_len is less than -1 (the emulation code allows from -1 as a special case) or if it exceeds 2032. Reported-by: Michael Roth <mdroth@linux.vnet.ibm.com> Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/net/stellaris_enet.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)