Re: [Qemu-devel] [PULL 28/30] introduce xlnx-dp

[Peter Maydell]

On Tue, 14 Jun 2016 at 15:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This is the implementation of the DisplayPort.
> It has an aux-bus to access dpcd and edid.
>
> Graphic plane is connected to the channel 3.
> Video plane is connected to the channel 0.
> Audio stream are connected to the channels 4 and 5.

Very old patch, but Coverity has just pointed out an array
overrun in it (CID 1487260):

We define a set of offsets for V_BLEND registers, of which
the largest is this one:

> +#define V_BLEND_CHROMA_KEY_COMP3            (0x01DC >> 2)

> +static void xlnx_dp_vblend_write(void *opaque, hwaddr offset,
> +                                 uint64_t value, unsigned size)
> +{
> +    XlnxDPState *s = XLNX_DP(opaque);
> +    bool alpha_was_enabled;
> +
> +    DPRINTF("vblend: write @0x%" HWADDR_PRIX " = 0x%" PRIX32 "\n", offset,
> +                                                               
> (uint32_t)value);
> +    offset = offset >> 2;
> +
> +    switch (offset) {

> +    case V_BLEND_CHROMA_KEY_COMP1:
> +    case V_BLEND_CHROMA_KEY_COMP2:
> +    case V_BLEND_CHROMA_KEY_COMP3:
> +        s->vblend_registers[offset] = value & 0x0FFF0FFF;

We use V_BLEND_CHROMA_KEY_COMP3 as an index into the vblend_registers array...

> +        break;
> +    default:
> +        s->vblend_registers[offset] = value;
> +        break;
> +    }
> +}

> +#define DP_CORE_REG_ARRAY_SIZE              (0x3AF >> 2)
> +#define DP_AVBUF_REG_ARRAY_SIZE             (0x238 >> 2)
> +#define DP_VBLEND_REG_ARRAY_SIZE            (0x1DF >> 2)
> +#define DP_AUDIO_REG_ARRAY_SIZE             (0x50 >> 2)

> +    uint32_t vblend_registers[DP_VBLEND_REG_ARRAY_SIZE];

...but we have defined DP_VBLEND_REG_ARRAY_SIZE to 0x1DF >> 2,
which is the same as 0x1DC >> 2, and so the array size is too small.

The size of the memory region is also suspicious:

+    memory_region_init_io(&s->vblend_iomem, obj, &vblend_ops, s, TYPE_XLNX_DP
+                          ".v_blend", 0x1DF);

This is a "32-bit accesses only" region, but we have defined it with a
size that is not a multiple of 4. That looks wrong... (It also means
that rather than having an array overrun I think the actual effect
is that the guest won't be able to access the last register, because
it's not entirely within the memoryregion.)

Coverity doesn't complain about it, but the DP_CORE_REG_ARRAY_SIZE
may also have a similar problem.

thanks
-- PMM