Patchwork [v4,11/30] pl022: fix buffer overun on invalid state load

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 31, 2014, 2:16 p.m.
Message ID <1396275242-10810-12-git-send-email-mst@redhat.com>
Download mbox | patch
Permalink /patch/335396/
State New
Headers show

Comments

Michael S. Tsirkin - March 31, 2014, 2:16 p.m.
CVE-2013-4530

pl022.c did not bounds check tx_fifo_head and
rx_fifo_head after loading them from file and
before they are used to dereference array.

Reported-by: Michael S. Tsirkin <mst@redhat.com
Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ssi/pl022.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Peter Maydell - March 31, 2014, 3:04 p.m.
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4530
>
> pl022.c did not bounds check tx_fifo_head and
> rx_fifo_head after loading them from file and
> before they are used to dereference array.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com
> Reported-by: Anthony Liguori <anthony@codemonkey.ws>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/ssi/pl022.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
> index fd479ef..49b3f61 100644
> --- a/hw/ssi/pl022.c
> +++ b/hw/ssi/pl022.c
> @@ -240,11 +240,23 @@ static const MemoryRegionOps pl022_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static int pl022_post_load(void *opaque, int version_id)
> +{
> +    PL022State *s = opaque;
> +
> +    if (s->tx_fifo_head > ARRAY_SIZE(s->tx_fifo) ||
> +        s->rx_fifo_head > ARRAY_SIZE(s->rx_fifo)) {
> +        return -1;

Shouldn't these be '>=' checks? Also if the
incoming values are negative then we'll go
wrong in the other direction.

Given the way we do calculations involving *_fifo_len
as well, it might be best to also sanitize those,
though I think it's not possible currently to provoke
a buffer overrun with them. (NB that the _fifo_len
fields can validly be equal to the ARRAY_SIZE, unlike
the _fifo_head fields.)

thanks
-- PMM

Patch

diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
index fd479ef..49b3f61 100644
--- a/hw/ssi/pl022.c
+++ b/hw/ssi/pl022.c
@@ -240,11 +240,23 @@  static const MemoryRegionOps pl022_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static int pl022_post_load(void *opaque, int version_id)
+{
+    PL022State *s = opaque;
+
+    if (s->tx_fifo_head > ARRAY_SIZE(s->tx_fifo) ||
+        s->rx_fifo_head > ARRAY_SIZE(s->rx_fifo)) {
+        return -1;
+    }
+    return 0;
+}
+
 static const VMStateDescription vmstate_pl022 = {
     .name = "pl022_ssp",
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pl022_post_load,
     .fields      = (VMStateField[]) {
         VMSTATE_UINT32(cr0, PL022State),
         VMSTATE_UINT32(cr1, PL022State),