| Message ID | 20240424125324.1628-1-adiupina@astralinux.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,RFC] fix host-endianness bug and prevent overflow | expand |
On Wed, 24 Apr 2024 at 13:53, Alexandra Diupina <adiupina@astralinux.ru> wrote: > > 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(). Thanks for this patch. Could you split it into two, please? The error in handling of the descriptor extension fields is a separate problem from the endianness bug, so they should be fixed in separate patches. > @@ -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, &desc); > > #ifdef DEBUG_DPDMA I would suggest factoring out a function like static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, uint64_t desc_addr, DPDMADescriptor *desc) which wraps up "read the descriptor from desc_addr and fill in desc" as a single operation (by calling dma_memory_read() and then doing the byteswap). thanks -- PMM
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, &desc); #ifdef DEBUG_DPDMA
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 <adiupina@astralinux.ru> --- hw/dma/xlnx_dpdma.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-)