| Message ID | 20240531124628.476938-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/dma/xlnx_dpdma: Read descriptor into buffer, not into pointer-to-buffer | expand |
On 31/5/24 14:46, Peter Maydell wrote: > In fdf029762f501 we factored out the handling of reading and writing > DMA descriptors from guest memory. Unfortunately we accidentally > made the descriptor-read read the descriptor into the address of the > buffer rather than into the buffer, because we didn't notice we > needed to update the arguments to the dma_memory_read() call. Before > the refactoring, "&desc" is the address of a local struct DPDMADescriptor > variable in xlnx_dpdma_start_operation(), which is the correct target > for the guest-memory-read. But after the refactoring 'desc' is the > "DPDMADescriptor *desc" argument to the new function, and so it is > already an address. > > This bug is an overrun of a stack variable, since a pointer is at > most 8 bytes long and we try to read 64 bytes, as well as being > incorrect behaviour. > > Pass 'desc' rather than '&desc' as the dma_memory_read() argument > to fix this. > > (The same bug is not present in xlnx_dpdma_write_descriptor(), > because there we are writing the descriptor from a local struct > variable "DPDMADescriptor tmp_desc" and so passing &tmp_desc to > dma_memory_write() is correct.) > > Spotted by Coverity: CID 1546649 > > Fixes: fdf029762f50101 ("xlnx_dpdma: fix descriptor endianness bug") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/dma/xlnx_dpdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Fri, May 31, 2024 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote: > In fdf029762f501 we factored out the handling of reading and writing > DMA descriptors from guest memory. Unfortunately we accidentally > made the descriptor-read read the descriptor into the address of the > buffer rather than into the buffer, because we didn't notice we > needed to update the arguments to the dma_memory_read() call. Before > the refactoring, "&desc" is the address of a local struct DPDMADescriptor > variable in xlnx_dpdma_start_operation(), which is the correct target > for the guest-memory-read. But after the refactoring 'desc' is the > "DPDMADescriptor *desc" argument to the new function, and so it is > already an address. > > This bug is an overrun of a stack variable, since a pointer is at > most 8 bytes long and we try to read 64 bytes, as well as being > incorrect behaviour. > > Pass 'desc' rather than '&desc' as the dma_memory_read() argument > to fix this. > > (The same bug is not present in xlnx_dpdma_write_descriptor(), > because there we are writing the descriptor from a local struct > variable "DPDMADescriptor tmp_desc" and so passing &tmp_desc to > dma_memory_write() is correct.) > > Spotted by Coverity: CID 1546649 > > + CC Fred. Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > Fixes: fdf029762f50101 ("xlnx_dpdma: fix descriptor endianness bug") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/dma/xlnx_dpdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c > index dde4aeca401..a685bd28bb8 100644 > --- a/hw/dma/xlnx_dpdma.c > +++ b/hw/dma/xlnx_dpdma.c > @@ -619,7 +619,7 @@ static MemTxResult > xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, > DPDMADescriptor *desc) > { > MemTxResult res = dma_memory_read(&address_space_memory, desc_addr, > - &desc, sizeof(DPDMADescriptor), > + desc, sizeof(DPDMADescriptor), > MEMTXATTRS_UNSPECIFIED); > if (res) { > return res; > -- > 2.34.1 > >
On 31/5/24 14:46, Peter Maydell wrote: > In fdf029762f501 we factored out the handling of reading and writing > DMA descriptors from guest memory. Unfortunately we accidentally > made the descriptor-read read the descriptor into the address of the > buffer rather than into the buffer, because we didn't notice we > needed to update the arguments to the dma_memory_read() call. Before > the refactoring, "&desc" is the address of a local struct DPDMADescriptor > variable in xlnx_dpdma_start_operation(), which is the correct target > for the guest-memory-read. But after the refactoring 'desc' is the > "DPDMADescriptor *desc" argument to the new function, and so it is > already an address. > > This bug is an overrun of a stack variable, since a pointer is at > most 8 bytes long and we try to read 64 bytes, as well as being > incorrect behaviour. > > Pass 'desc' rather than '&desc' as the dma_memory_read() argument > to fix this. > > (The same bug is not present in xlnx_dpdma_write_descriptor(), > because there we are writing the descriptor from a local struct > variable "DPDMADescriptor tmp_desc" and so passing &tmp_desc to > dma_memory_write() is correct.) > > Spotted by Coverity: CID 1546649 > > Fixes: fdf029762f50101 ("xlnx_dpdma: fix descriptor endianness bug") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/dma/xlnx_dpdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, patch queued via hw-misc (except if you rather your arm tree).
diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c index dde4aeca401..a685bd28bb8 100644 --- a/hw/dma/xlnx_dpdma.c +++ b/hw/dma/xlnx_dpdma.c @@ -619,7 +619,7 @@ static MemTxResult xlnx_dpdma_read_descriptor(XlnxDPDMAState *s, DPDMADescriptor *desc) { MemTxResult res = dma_memory_read(&address_space_memory, desc_addr, - &desc, sizeof(DPDMADescriptor), + desc, sizeof(DPDMADescriptor), MEMTXATTRS_UNSPECIFIED); if (res) { return res;
In fdf029762f501 we factored out the handling of reading and writing DMA descriptors from guest memory. Unfortunately we accidentally made the descriptor-read read the descriptor into the address of the buffer rather than into the buffer, because we didn't notice we needed to update the arguments to the dma_memory_read() call. Before the refactoring, "&desc" is the address of a local struct DPDMADescriptor variable in xlnx_dpdma_start_operation(), which is the correct target for the guest-memory-read. But after the refactoring 'desc' is the "DPDMADescriptor *desc" argument to the new function, and so it is already an address. This bug is an overrun of a stack variable, since a pointer is at most 8 bytes long and we try to read 64 bytes, as well as being incorrect behaviour. Pass 'desc' rather than '&desc' as the dma_memory_read() argument to fix this. (The same bug is not present in xlnx_dpdma_write_descriptor(), because there we are writing the descriptor from a local struct variable "DPDMADescriptor tmp_desc" and so passing &tmp_desc to dma_memory_write() is correct.) Spotted by Coverity: CID 1546649 Fixes: fdf029762f50101 ("xlnx_dpdma: fix descriptor endianness bug") Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/dma/xlnx_dpdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)