[PATCH v5] xlnx_dpdma: fix descriptor endianness bug
Add xlnx_dpdma_read_descriptor() and
xlnx_dpdma_write_descriptor() functions.
xlnx_dpdma_read_descriptor() combines reading a
descriptor from desc_addr by calling dma_memory_read()
and swapping the desc fields from guest memory order
to host memory order. xlnx_dpdma_write_descriptor()
performs similar actions when writing a descriptor.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
v5: fix subject and make xlnx_dpdma_write_descriptor() not void
v4: remove rewriting desc in place
v3: add xlnx_dpdma_write_descriptor()
v2: minor changes in xlnx_dpdma_read_descriptor()
hw/dma/xlnx_dpdma.c | 66 ++---
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..f582c1a085 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,63 @@ static void xlnx_dpdma_register_types(void)
type_register_static(_dpdma_info);
}
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+uint64_t desc_addr, DPDMADescriptor *desc)
+{
+MemTxResult res = dma_memory_read(_space_memory, desc_addr,
+, sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+if (res) {
+return res;
+}
+
+/* Convert from LE into host endianness. */
+desc->control = le32_to_cpu(desc->control);
+desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+desc->xfer_size = le32_to_cpu(desc->xfer_size);
+desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+desc->address_extension = le32_to_cpu(desc->address_extension);
+desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+desc->source_address = le32_to_cpu(desc->source_address);
+desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+desc->source_address2 = le32_to_cpu(desc->source_address2);
+desc->source_address3 = le32_to_cpu(desc->source_address3);
+desc->source_address4 = le32_to_cpu(desc->source_address4);
+desc->source_address5 = le32_to_cpu(desc->source_address5);
+desc->crc = le32_to_cpu(desc->crc);
+
+return res;
+}
+
+static MemTxResult xlnx_dpdma_write_descriptor(uint64_t desc_addr,
+DPDMADescriptor *desc)
+{
+DPDMADescriptor tmp_desc = *desc;
+
+/* Convert from host endianness into LE. */
+tmp_desc.control = cpu_to_le32(tmp_desc.control);
+tmp_desc.descriptor_id = cpu_to_le32(tmp_desc.descriptor_id);
+tmp_desc.xfer_size = cpu_to_le32(tmp_desc.xfer_size);
+tmp_desc.line_size_stride = cpu_to_le32(tmp_desc.line_size_stride);
+tmp_desc.timestamp_lsb = cpu_to_le32(tmp_desc.timestamp_lsb);
+tmp_desc.timestamp_msb = cpu_to_le32(tmp_desc.timestamp_msb);
+tmp_desc.address_extension = cpu_to_le32(tmp_desc.address_extension);
+tmp_desc.next_descriptor = cpu_to_le32(tmp_desc.next_descriptor);
+tmp_desc.source_address = cpu_to_le32(tmp_desc.source_address);
+tmp_desc.address_extension_23 = cpu_to_le32(tmp_desc.address_extension_23);
+tmp_desc.address_extension_45 = cpu_to_le32(tmp_desc.address_extension_45);
+tmp_desc.source_address2 = cpu_to_le32(tmp_desc.source_address2);
+tmp_desc.source_address3 = cpu_to_le32(tmp_desc.source_address3);
+tmp_desc.source_address4 = cpu_to_le32(tmp_desc.source_address4);
+tmp_desc.source_address5 = cpu_to_le32(tmp_desc.source_address5);
+tmp_desc.crc = cpu_to_le32(tmp_desc.crc);
+
+return dma_memory_write(_space_memory, desc_addr, _desc,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+}
+
size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
bool one_desc)
{
@@ -651,8 +708,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
}
-if (dma_memory_read(_space_memory, desc_addr, ,
-sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+if (xlnx_dpdma_read_descriptor(s, desc_addr, )) {
s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
xlnx_dpdma_update_irq(s);
s->operation_finished[channel] = true;
@@ -755,8 +811,10 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
/* The descriptor need to be updated when it's completed. */
DPRINTF("update the descriptor with the done flag set.\n");
xlnx
[PATCH v4] fix endianness bug
Add xlnx_dpdma_read_descriptor() and
xlnx_dpdma_write_descriptor() functions.
xlnx_dpdma_read_descriptor() combines reading a
descriptor from desc_addr by calling dma_memory_read()
and swapping the desc fields from guest memory order
to host memory order. xlnx_dpdma_write_descriptor()
performs similar actions when writing a descriptor.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
v4: remove rewriting desc in place
v3: add xlnx_dpdma_write_descriptor()
v2: minor changes in xlnx_dpdma_read_descriptor()
hw/dma/xlnx_dpdma.c | 63 ++---
1 file changed, 59 insertions(+), 4 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..e9133e9dcb 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,63 @@ static void xlnx_dpdma_register_types(void)
type_register_static(_dpdma_info);
}
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+uint64_t desc_addr, DPDMADescriptor *desc)
+{
+if (dma_memory_read(_space_memory, desc_addr, ,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+return MEMTX_ERROR;
+}
+
+/* Convert from LE into host endianness. */
+desc->control = le32_to_cpu(desc->control);
+desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+desc->xfer_size = le32_to_cpu(desc->xfer_size);
+desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+desc->address_extension = le32_to_cpu(desc->address_extension);
+desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+desc->source_address = le32_to_cpu(desc->source_address);
+desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+desc->source_address2 = le32_to_cpu(desc->source_address2);
+desc->source_address3 = le32_to_cpu(desc->source_address3);
+desc->source_address4 = le32_to_cpu(desc->source_address4);
+desc->source_address5 = le32_to_cpu(desc->source_address5);
+desc->crc = le32_to_cpu(desc->crc);
+
+return MEMTX_OK;
+}
+
+static void xlnx_dpdma_write_descriptor(uint64_t desc_addr,
+DPDMADescriptor *desc)
+{
+DPDMADescriptor* tmp_desc = (DPDMADescriptor
*)malloc(sizeof(DPDMADescriptor));
+memcpy(tmp_desc, desc, sizeof(desc));
+
+/* Convert from host endianness into LE. */
+tmp_desc->control = cpu_to_le32(tmp_desc->control);
+tmp_desc->descriptor_id = cpu_to_le32(tmp_desc->descriptor_id);
+tmp_desc->xfer_size = cpu_to_le32(tmp_desc->xfer_size);
+tmp_desc->line_size_stride = cpu_to_le32(tmp_desc->line_size_stride);
+tmp_desc->timestamp_lsb = cpu_to_le32(tmp_desc->timestamp_lsb);
+tmp_desc->timestamp_msb = cpu_to_le32(tmp_desc->timestamp_msb);
+tmp_desc->address_extension = cpu_to_le32(tmp_desc->address_extension);
+tmp_desc->next_descriptor = cpu_to_le32(tmp_desc->next_descriptor);
+tmp_desc->source_address = cpu_to_le32(tmp_desc->source_address);
+tmp_desc->address_extension_23 =
cpu_to_le32(tmp_desc->address_extension_23);
+tmp_desc->address_extension_45 =
cpu_to_le32(tmp_desc->address_extension_45);
+tmp_desc->source_address2 = cpu_to_le32(tmp_desc->source_address2);
+tmp_desc->source_address3 = cpu_to_le32(tmp_desc->source_address3);
+tmp_desc->source_address4 = cpu_to_le32(tmp_desc->source_address4);
+tmp_desc->source_address5 = cpu_to_le32(tmp_desc->source_address5);
+tmp_desc->crc = cpu_to_le32(tmp_desc->crc);
+
+dma_memory_write(_space_memory, desc_addr, tmp_desc,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+}
+
size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
bool one_desc)
{
@@ -651,8 +708,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
}
-if (dma_memory_read(_space_memory, desc_addr, ,
-sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+if (xlnx_dpdma_read_descriptor(s, desc_addr, )) {
s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
xlnx_dpdma_update_irq(s);
s->operation_finished[channel] = true;
@@ -755,8 +811,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
/* The de
[PATCH v2] fix bit fields extraction and prevent overflow
Add a type cast and use extract64() instead of extract32()
to avoid integer overflow on addition. Fix bit fields
extraction according to documentation.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
v2: fix typo
hw/dma/xlnx_dpdma.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..530717d188 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)desc->source_address
++ (extract64(desc->address_extension, 16, 16) << 32);
break;
case 1:
-addr = desc->source_address2
-+ (extract32(desc->address_extension_23, 0, 12) << 8);
+addr = (uint64_t)desc->source_address2
++ (extract64(desc->address_extension_23, 0, 16) << 32);
break;
case 2:
-addr = desc->source_address3
-+ (extract32(desc->address_extension_23, 16, 12) << 20);
+addr = (uint64_t)desc->source_address3
++ (extract64(desc->address_extension_23, 16, 16) << 32);
break;
case 3:
-addr = desc->source_address4
-+ (extract32(desc->address_extension_45, 0, 12) << 8);
+addr = (uint64_t)desc->source_address4
++ (extract64(desc->address_extension_45, 0, 16) << 32);
break;
case 4:
-addr = desc->source_address5
-+ (extract32(desc->address_extension_45, 16, 12) << 20);
+addr = (uint64_t)desc->source_address5
++ (extract64(desc->address_extension_45, 16, 16) << 32);
break;
default:
addr = 0;
--
2.30.2
[PATCH v3] fix endianness bug
Add xlnx_dpdma_read_descriptor() and
xlnx_dpdma_write_descriptor() functions.
xlnx_dpdma_read_descriptor() combines reading a
descriptor from desc_addr by calling dma_memory_read()
and swapping the desc fields from guest memory order
to host memory order. xlnx_dpdma_write_descriptor()
performs similar actions when writing a descriptor.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
v3: add xlnx_dpdma_write_descriptor()
v2: minor changes in xlnx_dpdma_read_descriptor()
hw/dma/xlnx_dpdma.c | 59 ++---
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..7845f43221 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,59 @@ static void xlnx_dpdma_register_types(void)
type_register_static(_dpdma_info);
}
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+uint64_t desc_addr, DPDMADescriptor *desc)
+{
+if (dma_memory_read(_space_memory, desc_addr, ,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
+return MEMTX_ERROR;
+
+/* Convert from LE into host endianness. */
+desc->control = le32_to_cpu(desc->control);
+desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+desc->xfer_size = le32_to_cpu(desc->xfer_size);
+desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+desc->address_extension = le32_to_cpu(desc->address_extension);
+desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+desc->source_address = le32_to_cpu(desc->source_address);
+desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+desc->source_address2 = le32_to_cpu(desc->source_address2);
+desc->source_address3 = le32_to_cpu(desc->source_address3);
+desc->source_address4 = le32_to_cpu(desc->source_address4);
+desc->source_address5 = le32_to_cpu(desc->source_address5);
+desc->crc = le32_to_cpu(desc->crc);
+
+return MEMTX_OK;
+}
+
+static void xlnx_dpdma_write_descriptor(uint64_t desc_addr,
+DPDMADescriptor *desc)
+{
+/* Convert from host endianness into LE. */
+desc->control = cpu_to_le32(desc->control);
+desc->descriptor_id = cpu_to_le32(desc->descriptor_id);
+desc->xfer_size = cpu_to_le32(desc->xfer_size);
+desc->line_size_stride = cpu_to_le32(desc->line_size_stride);
+desc->timestamp_lsb = cpu_to_le32(desc->timestamp_lsb);
+desc->timestamp_msb = cpu_to_le32(desc->timestamp_msb);
+desc->address_extension = cpu_to_le32(desc->address_extension);
+desc->next_descriptor = cpu_to_le32(desc->next_descriptor);
+desc->source_address = cpu_to_le32(desc->source_address);
+desc->address_extension_23 = cpu_to_le32(desc->address_extension_23);
+desc->address_extension_45 = cpu_to_le32(desc->address_extension_45);
+desc->source_address2 = cpu_to_le32(desc->source_address2);
+desc->source_address3 = cpu_to_le32(desc->source_address3);
+desc->source_address4 = cpu_to_le32(desc->source_address4);
+desc->source_address5 = cpu_to_le32(desc->source_address5);
+desc->crc = cpu_to_le32(desc->crc);
+
+dma_memory_write(_space_memory, desc_addr, ,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+}
+
size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
bool one_desc)
{
@@ -651,8 +704,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
}
-if (dma_memory_read(_space_memory, desc_addr, ,
-sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+if (xlnx_dpdma_read_descriptor(s, desc_addr, )) {
s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
xlnx_dpdma_update_irq(s);
s->operation_finished[channel] = true;
@@ -755,8 +807,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
/* The descriptor need to be updated when it's completed. */
DPRINTF("update the descriptor with the done flag set.\n");
xlnx_dpdma_desc_set_done();
-dma_memory_write(_space_memory, desc_addr, ,
- sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED);
+xlnx_dpdma_write_descriptor(desc_addr, );
}
if (xlnx_dpdma_desc_completion_interrupt()) {
--
2.30.2
[PATCH v2] fix host-endianness bug
Add a function xlnx_dpdma_read_descriptor() that
combines reading the descriptor from desc_addr
by calling dma_memory_read() and swapping desc
fields from guest memory order to host memory order.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
v2:minor changes in xlnx_dpdma_read_descriptor()
hw/dma/xlnx_dpdma.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..62a0952377 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,34 @@ static void xlnx_dpdma_register_types(void)
type_register_static(_dpdma_info);
}
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s,
+uint64_t desc_addr, DPDMADescriptor *desc)
+{
+if (dma_memory_read(_space_memory, desc_addr, ,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED))
+return MEMTX_ERROR;
+
+/* Convert from LE into host endianness. */
+desc->control = le32_to_cpu(desc->control);
+desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+desc->xfer_size = le32_to_cpu(desc->xfer_size);
+desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+desc->address_extension = le32_to_cpu(desc->address_extension);
+desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+desc->source_address = le32_to_cpu(desc->source_address);
+desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+desc->source_address2 = le32_to_cpu(desc->source_address2);
+desc->source_address3 = le32_to_cpu(desc->source_address3);
+desc->source_address4 = le32_to_cpu(desc->source_address4);
+desc->source_address5 = le32_to_cpu(desc->source_address5);
+desc->crc = le32_to_cpu(desc->crc);
+
+return MEMTX_OK;
+}
+
size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
bool one_desc)
{
@@ -651,8 +679,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
}
-if (dma_memory_read(_space_memory, desc_addr, ,
-sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+if (xlnx_dpdma_read_descriptor(s, desc_addr, )) {
s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
xlnx_dpdma_update_irq(s);
s->operation_finished[channel] = true;
--
2.30.2
[PATCH] fix bit fields extraction and prevent overflow
Add a type cast and use extract64() instead of extract32()
to avoid integer overflow on addition. Fix bit fields
extraction according to documentation.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..52e8c594fe 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)desc->source_address
++ (extract64(desc->address_extension, 16, 12) << 20);
break;
case 1:
-addr = desc->source_address2
-+ (extract32(desc->address_extension_23, 0, 12) << 8);
+addr = (uint64_t)desc->source_address2
++ (extract64(desc->address_extension_23, 0, 12) << 8);
break;
case 2:
-addr = desc->source_address3
-+ (extract32(desc->address_extension_23, 16, 12) << 20);
+addr = (uint64_t)desc->source_address3
++ (extract64(desc->address_extension_23, 16, 12) << 20);
break;
case 3:
-addr = desc->source_address4
-+ (extract32(desc->address_extension_45, 0, 12) << 8);
+addr = (uint64_t)desc->source_address4
++ (extract64(desc->address_extension_45, 0, 12) << 8);
break;
case 4:
-addr = desc->source_address5
-+ (extract32(desc->address_extension_45, 16, 12) << 20);
+addr = (uint64_t)desc->source_address5
++ (extract64(desc->address_extension_45, 16, 12) << 20);
break;
default:
addr = 0;
--
2.30.2
[PATCH] fix host-endianness bug
Add a function xlnx_dpdma_read_descriptor() that
combines reading the descriptor from desc_addr
by calling dma_memory_read() and swapping desc
fields from guest memory order to host memory order.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..5fd4e31699 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -614,6 +614,38 @@ static void xlnx_dpdma_register_types(void)
type_register_static(_dpdma_info);
}
+static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, uint8_t
channel,
+uint64_t desc_addr, DPDMADescriptor
*desc)
+{
+if (dma_memory_read(_space_memory, desc_addr, ,
+sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
+s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
+xlnx_dpdma_update_irq(s);
+s->operation_finished[channel] = true;
+return MEMTX_ERROR;
+}
+
+/* Convert from LE into host endianness. */
+desc->control = le32_to_cpu(desc->control);
+desc->descriptor_id = le32_to_cpu(desc->descriptor_id);
+desc->xfer_size = le32_to_cpu(desc->xfer_size);
+desc->line_size_stride = le32_to_cpu(desc->line_size_stride);
+desc->timestamp_lsb = le32_to_cpu(desc->timestamp_lsb);
+desc->timestamp_msb = le32_to_cpu(desc->timestamp_msb);
+desc->address_extension = le32_to_cpu(desc->address_extension);
+desc->next_descriptor = le32_to_cpu(desc->next_descriptor);
+desc->source_address = le32_to_cpu(desc->source_address);
+desc->address_extension_23 = le32_to_cpu(desc->address_extension_23);
+desc->address_extension_45 = le32_to_cpu(desc->address_extension_45);
+desc->source_address2 = le32_to_cpu(desc->source_address2);
+desc->source_address3 = le32_to_cpu(desc->source_address3);
+desc->source_address4 = le32_to_cpu(desc->source_address4);
+desc->source_address5 = le32_to_cpu(desc->source_address5);
+desc->crc = le32_to_cpu(desc->crc);
+
+return MEMTX_OK;
+}
+
size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s, uint8_t channel,
bool one_desc)
{
@@ -651,11 +683,7 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
desc_addr = xlnx_dpdma_descriptor_next_address(s, channel);
}
-if (dma_memory_read(_space_memory, desc_addr, ,
-sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED)) {
-s->registers[DPDMA_EISR] |= ((1 << 1) << channel);
-xlnx_dpdma_update_irq(s);
-s->operation_finished[channel] = true;
+if (xlnx_dpdma_read_descriptor(s, channel, desc_addr, )) {
DPRINTF("Can't get the descriptor.\n");
break;
}
--
2.30.2
[PATCH v2 RFC] fix host-endianness bug and prevent overflow
Add a type cast and use extract64() instead of extract32()
to avoid integer overflow on addition. Fix bit fields
extraction according to documentation.
Also fix host-endianness bug by swapping desc fields from guest
memory order to host memory order after dma_memory_read().
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 38 --
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index dd66be5265..d22b942274 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)desc->source_address
++ (extract64(desc->address_extension, 16, 16) << 32);
break;
case 1:
-addr = desc->source_address2
-+ (extract32(desc->address_extension_23, 0, 12) << 8);
+addr = (uint64_t)desc->source_address2
++ (extract64(desc->address_extension_23, 0, 16) << 32);
break;
case 2:
-addr = desc->source_address3
-+ (extract32(desc->address_extension_23, 16, 12) << 20);
+addr = (uint64_t)desc->source_address3
++ (extract64(desc->address_extension_23, 16, 16) << 32);
break;
case 3:
-addr = desc->source_address4
-+ (extract32(desc->address_extension_45, 0, 12) << 8);
+addr = (uint64_t)desc->source_address4
++ (extract64(desc->address_extension_45, 0, 16) << 32);
break;
case 4:
-addr = desc->source_address5
-+ (extract32(desc->address_extension_45, 16, 12) << 20);
+addr = (uint64_t)desc->source_address5
++ (extract64(desc->address_extension_45, 16, 16) << 32);
break;
default:
addr = 0;
@@ -660,6 +660,24 @@ size_t xlnx_dpdma_start_operation(XlnxDPDMAState *s,
uint8_t channel,
break;
}
+/* Convert from LE into host endianness. */
+desc.control = le32_to_cpu(desc.control);
+desc.descriptor_id = le32_to_cpu(desc.descriptor_id);
+desc.xfer_size = le32_to_cpu(desc.xfer_size);
+desc.line_size_stride = le32_to_cpu(desc.line_size_stride);
+desc.timestamp_lsb = le32_to_cpu(desc.timestamp_lsb);
+desc.timestamp_msb = le32_to_cpu(desc.timestamp_msb);
+desc.address_extension = le32_to_cpu(desc.address_extension);
+desc.next_descriptor = le32_to_cpu(desc.next_descriptor);
+desc.source_address = le32_to_cpu(desc.source_address);
+desc.address_extension_23 = le32_to_cpu(desc.address_extension_23);
+desc.address_extension_45 = le32_to_cpu(desc.address_extension_45);
+desc.source_address2 = le32_to_cpu(desc.source_address2);
+desc.source_address3 = le32_to_cpu(desc.source_address3);
+desc.source_address4 = le32_to_cpu(desc.source_address4);
+desc.source_address5 = le32_to_cpu(desc.source_address5);
+desc.crc = le32_to_cpu(desc.crc);
+
xlnx_dpdma_update_desc_info(s, channel, );
#ifdef DEBUG_DPDMA
--
2.30.2
Re: [PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()
17/04/24 13:05, Konrad, Frederic пишет:
Hi,
-Original Message-
From: qemu-devel-bounces+fkonrad=amd@nongnu.org
On Behalf Of
Peter Maydell
Sent: Friday, April 12, 2024 12:07 PM
To: Alexandra Diupina
Cc: Alistair Francis ; Edgar E. Iglesias
; qemu-...@nongnu.org; qemu-
de...@nongnu.org; sdl.q...@linuxtesting.org
Subject: Re: [PATCH RFC] prevent overflow in
xlnx_dpdma_desc_get_source_address()
On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina wrote:
Overflow can occur in a situation where desc->source_address
has a maximum value (pow(2, 32) - 1), so add a cast to a
larger type before the assignment.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..224259225c 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)(desc->source_address
++ (extract32(desc->address_extension, 16, 12) << 20));
Unless I'm confused, this cast doesn't help, because we
will have already done a 32-bit addition of desc->source_address
and the value from the address_extension part, so it doesn't
change the result.
If we want to do the addition at 64 bits then using extract64()
would be the simplest way to arrange for that.
However, I can't figure out what this code is trying to do and
make that line up with the data sheet; maybe this isn't the
right datasheet for this device?
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
The datasheet suggests that we should take 32 bits of the address
from one field (here desc->source_address) and 16 bits of the
address from another (here desc->address_extension's high bits)
and combine them to make a 48 bit address. But this code is only
looking at 12 bits of the high 16 in address_extension, and it
doesn't shift them right enough to put them into bits [47:32]
of the final address.
Xilinx folks: what hardware is this modelling, and is it
really the right behaviour?
Looks like this is the right documentation. Most probably the descriptor field
changed
since I did that model, or I got really confused.
Also, this device looks like it has a host-endianness bug: the
DPDMADescriptor struct is read directly from guest memory in
dma_memory_read(), but the device never does anything to swap
the fields from guest memory order to host memory order. So
this is likely broken on big-endian hosts.
Yes indeed.
Best Regards,
Fred
thanks
-- PMM
hw/dma/xlnx_dpdma.c defines a dma_ops structure
with the endianness field set to DEVICE_NATIVE_ENDIAN.
Doesn't this guarantee that there will be no endian errors?
Alexandra
Re: [PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()
Peter, thank you! I agree with you that
as mentioned in the documentation
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field,
we should take 32 bits of the address from one field
(for example, case 1, SRC_ADDR2_EXT - in code it is desc->source_address2)
and 16 bits (high or low) of the address from another field
(ADDR_EXT_23 - in code it is desc->address_extension_23, we need [15:0]
bits)
and combine them to make a 48 bit address.
Therefore, I suggest making the following changes to the code
so that it matches the documentation:
static uint64_t xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
uint8_t frag)
{
uint64_t addr = 0;
assert(frag < 5);
switch (frag) {
case 0:
addr = (uint64_t)desc->source_address
+ (extract64(desc->address_extension, 16, 16) << 32);
break;
case 1:
addr = (uint64_t)desc->source_address2
+ (extract64(desc->address_extension_23, 0, 16) << 32);
break;
case 2:
addr = (uint64_t)desc->source_address3
+ (extract64(desc->address_extension_23, 16, 16) << 32);
break;
case 3:
addr = (uint64_t)desc->source_address4
+ (extract64(desc->address_extension_45, 0, 16) << 32);
break;
case 4:
addr = (uint64_t)desc->source_address5
+ (extract64(desc->address_extension_45, 16, 16) << 32);
break;
default:
addr = 0;
break;
}
return addr;
}
This change adds a type cast and also uses extract64() instead of
extract32()
to avoid integer overflow on addition (there was a typo in the previous
letter).
Also in extract64() extracts a bit field with a length of 16 bits
instead of 12,
the shift is changed to 32 so that the extracted field fits into bits
[47:32] of the final address.
if this calculation is correct, I'm ready to create a second version of
the patch.
12/04/24 13:06, Peter Maydell пишет:
On Fri, 12 Apr 2024 at 09:13, Alexandra Diupina wrote:
Overflow can occur in a situation where desc->source_address
has a maximum value (pow(2, 32) - 1), so add a cast to a
larger type before the assignment.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..224259225c 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)(desc->source_address
++ (extract32(desc->address_extension, 16, 12) << 20));
Unless I'm confused, this cast doesn't help, because we
will have already done a 32-bit addition of desc->source_address
and the value from the address_extension part, so it doesn't
change the result.
If we want to do the addition at 64 bits then using extract64()
would be the simplest way to arrange for that.
However, I can't figure out what this code is trying to do and
make that line up with the data sheet; maybe this isn't the
right datasheet for this device?
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/ADDR_EXT-Field
The datasheet suggests that we should take 32 bits of the address
from one field (here desc->source_address) and 16 bits of the
address from another (here desc->address_extension's high bits)
and combine them to make a 48 bit address. But this code is only
looking at 12 bits of the high 16 in address_extension, and it
doesn't shift them right enough to put them into bits [47:32]
of the final address.
Xilinx folks: what hardware is this modelling, and is it
really the right behaviour?
Also, this device looks like it has a host-endianness bug: the
DPDMADescriptor struct is read directly from guest memory in
dma_memory_read(), but the device never does anything to swap
the fields from guest memory order to host memory order. So
this is likely broken on big-endian hosts.
thanks
-- PMM
[PATCH RFC] prevent overflow in xlnx_dpdma_desc_get_source_address()
Overflow can occur in a situation where desc->source_address
has a maximum value (pow(2, 32) - 1), so add a cast to a
larger type before the assignment.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d3c6369a96 ("introduce xlnx-dpdma")
Signed-off-by: Alexandra Diupina
---
hw/dma/xlnx_dpdma.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
index 1f5cd64ed1..224259225c 100644
--- a/hw/dma/xlnx_dpdma.c
+++ b/hw/dma/xlnx_dpdma.c
@@ -175,24 +175,24 @@ static uint64_t
xlnx_dpdma_desc_get_source_address(DPDMADescriptor *desc,
switch (frag) {
case 0:
-addr = desc->source_address
-+ (extract32(desc->address_extension, 16, 12) << 20);
+addr = (uint64_t)(desc->source_address
++ (extract32(desc->address_extension, 16, 12) << 20));
break;
case 1:
-addr = desc->source_address2
-+ (extract32(desc->address_extension_23, 0, 12) << 8);
+addr = (uint64_t)(desc->source_address2
++ (extract32(desc->address_extension_23, 0, 12) << 8));
break;
case 2:
-addr = desc->source_address3
-+ (extract32(desc->address_extension_23, 16, 12) << 20);
+addr = (uint64_t)(desc->source_address3
++ (extract32(desc->address_extension_23, 16, 12) << 20));
break;
case 3:
-addr = desc->source_address4
-+ (extract32(desc->address_extension_45, 0, 12) << 8);
+addr = (uint64_t)(desc->source_address4
++ (extract32(desc->address_extension_45, 0, 12) << 8));
break;
case 4:
-addr = desc->source_address5
-+ (extract32(desc->address_extension_45, 16, 12) << 20);
+addr = (uint64_t)(desc->source_address5
++ (extract32(desc->address_extension_45, 16, 12) << 20));
break;
default:
addr = 0;
--
2.30.2
[PATCH v2] esp: process the result of scsi_device_find()
Add a 'current_lun' check for a null value
to avoid null pointer dereferencing and
recover host if NULL return
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 4eb8606560 (esp: store lun coming from the MESSAGE OUT phase)
Signed-off-by: Alexandra Diupina
---
v2: duplicate the scsi_device_find() logic from esp_select()
hw/scsi/esp.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9b11d8c573..03fdd53de6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -292,6 +292,16 @@ static void do_command_phase(ESPState *s)
esp_fifo_pop_buf(>cmdfifo, buf, cmdlen);
current_lun = scsi_device_find(>bus, 0, s->current_dev->id, s->lun);
+
+if (!current_lun) {
+/* No such drive */
+s->rregs[ESP_RSTAT] = 0;
+s->rregs[ESP_RINTR] = INTR_DC;
+s->rregs[ESP_RSEQ] = SEQ_0;
+esp_raise_irq(s);
+return;
+}
+
s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s);
datalen = scsi_req_enqueue(s->current_req);
s->ti_size = datalen;
--
2.30.2
[PATCH] esp: process the result of scsi_device_find()
Add a 'current_lun' check for a null value
to avoid null pointer dereferencing
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 4eb8606560 (esp: store lun coming from the MESSAGE OUT phase)
Signed-off-by: Alexandra Diupina
---
hw/scsi/esp.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9b11d8c573..3a2ec35f9b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -292,6 +292,11 @@ static void do_command_phase(ESPState *s)
esp_fifo_pop_buf(>cmdfifo, buf, cmdlen);
current_lun = scsi_device_find(>bus, 0, s->current_dev->id, s->lun);
+
+if (!current_lun) {
+return;
+}
+
s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s);
datalen = scsi_req_enqueue(s->current_req);
s->ti_size = datalen;
--
2.30.2
[PATCH] hw/display/vmware_vga: fix probably typo
When calling trace_vmware_verify_rect_greater_than_bound() replace
"y" with "h" and y with h
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 02218aedb1 ("hw/display/vmware_vga: replace fprintf calls with trace
events")
Signed-off-by: Alexandra Diupina
---
hw/display/vmware_vga.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 7490d43881..3f26bea190 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -336,8 +336,8 @@ static inline bool vmsvga_verify_rect(DisplaySurface
*surface,
return false;
}
if (h > SVGA_MAX_HEIGHT) {
-trace_vmware_verify_rect_greater_than_bound(name, "y", SVGA_MAX_HEIGHT,
-y);
+trace_vmware_verify_rect_greater_than_bound(name, "h", SVGA_MAX_HEIGHT,
+h);
return false;
}
if (y + h > surface_height(surface)) {
--
2.30.2
