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

[Frederic Konrad <konrad.frederic@yahoo.fr>]

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