The xlnx_dpdma DMA engine allows the guest to do DMA to host buffers for audio and graphics data. These buffers are set up by the xlnx_dp device, which calls xlnx_dpdma_set_host_data_location() to tell the DMA engine where they are. However, this API doesn't provide any way to say how big the host buffers are, and the DMA engine does not check whether the guest is trying to write off the end of the buffer.
Add a length argument to xlnx_dpdma_set_host_data_location(), and bounds check the guest's DMA writes against it. The DPDMA ISR has bits for each channel for "descriptor payload is too large", so we report such overflows there. Cc: [email protected] Fixes: d3c6369a96a8 ("introduce xlnx-dpdma") Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4134 Signed-off-by: Peter Maydell <[email protected]> Reviewed-by: Alistair Francis <[email protected]> Message-id: [email protected] --- include/hw/dma/xlnx_dpdma.h | 5 +++-- hw/display/xlnx_dp.c | 30 ++++++++++++++++++++++++------ hw/dma/xlnx_dpdma.c | 13 ++++++++++++- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/include/hw/dma/xlnx_dpdma.h b/include/hw/dma/xlnx_dpdma.h index 7cf67aef6e1..0fc24e23ad3 100644 --- a/include/hw/dma/xlnx_dpdma.h +++ b/include/hw/dma/xlnx_dpdma.h @@ -38,6 +38,7 @@ struct XlnxDPDMAState { MemoryRegion iomem; uint32_t registers[XLNX_DPDMA_REG_ARRAY_SIZE]; uint8_t *data[6]; + uint64_t datalen[6]; bool operation_finished[6]; qemu_irq irq; }; @@ -69,10 +70,10 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel, * @s The DPDMA state. * @channel The channel associated to the pointer. * @p The buffer where to store the data. + * @len: The length of the buffer in bytes. */ -/* XXX: add a maximum size arg and send an interrupt in case of overflow. */ void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t channel, - void *p); + void *p, uint64_t len); /* * xlnx_dpdma_trigger_vsync_irq: Trigger a VSYNC IRQ when the display is diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c index d6b4e7bba61..ad6c4952065 100644 --- a/hw/display/xlnx_dp.c +++ b/hw/display/xlnx_dp.c @@ -333,9 +333,11 @@ static inline void xlnx_dp_audio_activate(XlnxDPState *s) & DP_TX_AUD_CTRL) != 0); audio_be_set_active_out(s->audio_be, s->amixer_output_stream, activated); xlnx_dpdma_set_host_data_location(s->dpdma, DP_AUDIO_DMA_CHANNEL(0), - &s->audio_buffer_0); + &s->audio_buffer_0, + sizeof(s->audio_buffer_0)); xlnx_dpdma_set_host_data_location(s->dpdma, DP_AUDIO_DMA_CHANNEL(1), - &s->audio_buffer_1); + &s->audio_buffer_1, + sizeof(s->audio_buffer_1)); } static inline void xlnx_dp_audio_mix_buffer(XlnxDPState *s) @@ -566,6 +568,18 @@ static void xlnx_dp_aux_set_command(XlnxDPState *s, uint32_t value) s->core_registers[DP_INTERRUPT_SIGNAL_STATE] |= 0x04; } +/* + * Return the size of the surface in bytes. Pixman doesn't provide + * a helper function/accessor for this, so we must calculate it + * from the stride and height. Note that a pixman stride can + * potentially be negative, hence the abs(). + */ +static inline uint64_t surface_data_size(DisplaySurface *s) +{ + /* Can't overflow because stride and height are both int */ + return (uint64_t)abs(surface_stride(s)) * surface_height(s); +} + static void xlnx_dp_set_dpdma(const Object *obj, const char *name, Object *val, Error **errp) { @@ -580,7 +594,8 @@ static void xlnx_dp_set_dpdma(const Object *obj, const char *name, Object *val, DisplaySurface *surface = qemu_console_surface(s->console); XlnxDPDMAState *dma = XLNX_DPDMA(val); xlnx_dpdma_set_host_data_location(dma, DP_GRAPHIC_DMA_CHANNEL, - surface_data(surface)); + surface_data(surface), + surface_data_size(surface)); } } @@ -644,9 +659,11 @@ static void xlnx_dp_recreate_surface(XlnxDPState *s) } xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL, - surface_data(s->g_plane.surface)); + surface_data(s->g_plane.surface), + surface_data_size(s->g_plane.surface)); xlnx_dpdma_set_host_data_location(s->dpdma, DP_VIDEO_DMA_CHANNEL, - surface_data(s->v_plane.surface)); + surface_data(s->v_plane.surface), + surface_data_size(s->v_plane.surface)); } } @@ -1396,7 +1413,8 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp) s->console = qemu_graphic_console_create(dev, 0, &xlnx_dp_gfx_ops, s); surface = qemu_console_surface(s->console); xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL, - surface_data(surface)); + surface_data(surface), + surface_data_size(surface)); as.freq = 44100; as.nchannels = 2; diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c index 32f56036a4c..9d8e7593698 100644 --- a/hw/dma/xlnx_dpdma.c +++ b/hw/dma/xlnx_dpdma.c @@ -589,6 +589,7 @@ static void xlnx_dpdma_reset(DeviceState *dev) for (i = 0; i < 6; i++) { s->data[i] = NULL; + s->datalen[i] = 0; s->operation_finished[i] = true; } } @@ -760,6 +761,14 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel, int64_t transfer_len = xlnx_dpdma_desc_get_transfer_size(&desc); uint32_t line_size = xlnx_dpdma_desc_get_line_size(&desc); uint32_t line_stride = xlnx_dpdma_desc_get_line_stride(&desc); + + if (transfer_len > s->datalen[channel]) { + s->registers[DPDMA_ISR] |= ((1 << 18) << channel); + xlnx_dpdma_update_irq(s); + DPRINTF("Descriptor payload too large..\n"); + goto update_descriptor; + } + if (xlnx_dpdma_desc_is_contiguous(&desc)) { source_addr[0] = xlnx_dpdma_desc_get_source_address(&desc, 0); while (transfer_len != 0) { @@ -809,6 +818,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel, } } + update_descriptor: if (xlnx_dpdma_desc_update_enabled(&desc)) { /* The descriptor need to be updated when it's completed. */ DPRINTF("update the descriptor with the done flag set.\n"); @@ -831,7 +841,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel, } void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t channel, - void *p) + void *p, uint64_t len) { if (!s) { qemu_log_mask(LOG_UNIMP, "DPDMA client not attached to valid DPDMA" @@ -841,6 +851,7 @@ void xlnx_dpdma_set_host_data_location(XlnxDPDMAState *s, uint8_t channel, assert(channel <= 5); s->data[channel] = p; + s->datalen[channel] = len; } void xlnx_dpdma_trigger_vsync_irq(XlnxDPDMAState *s) -- 2.43.0
