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

[Frederic Konrad]

Le 4/7/22 à 12:32, Peter Maydell a écrit :
> 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.)

arg, sorry for that..

I share your point, it should not be possible to access it, but using
the monitor:

(qemu) info mtree
...
00000000fd4aa000-00000000fd4aa1de (prio 0, i/o): xlnx.v-dp.v_blend
...

I can actually read that register (at least it doesn't complain, on an
older qemu version though):
(qemu) xp /w 0xfd4aa1dc
00000000fd4aa1dc: 0x00000000

So I'm not totally sure.. do you need a patch for 7.0.0?

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

I think it doesn't complain because writing to the last register doesn't
actually write into the array but update the mask register instead:

    case DP_INT_DS:
        s->core_registers[DP_INT_MASK] |= ~value;
        xlnx_dp_update_irq(s);
        break;

> thanks
> -- PMM

Best Regards,
Fred