Re: [Qemu-devel] [PATCH v4 11/30] pl022: fix buffer overun on invalid state load

[Peter Maydell]

On 31 March 2014 15:16, Michael S. Tsirkin <address@hidden> 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 <address@hidden
> Reported-by: Anthony Liguori <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  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