On 6 October 2015 at 15:44, Stefan Hajnoczi <stefa...@gmail.com> wrote: > On Thu, Oct 01, 2015 at 02:16:55PM +0200, Marc Marí wrote: >> @@ -292,6 +307,119 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr >> addr, >> } while (i); >> } >> >> +static void fw_cfg_dma_transfer(FWCfgState *s) >> +{ >> + dma_addr_t len; >> + FWCfgDmaAccess dma; >> + int arch; >> + FWCfgEntry *e; >> + int read; >> + dma_addr_t dma_addr; >> + >> + /* Reset the address before the next access */ >> + dma_addr = s->dma_addr; >> + s->dma_addr = 0; >> + >> + dma.address = ldq_be_dma(s->dma_as, >> + dma_addr + offsetof(FWCfgDmaAccess, address)); >> + dma.length = ldl_be_dma(s->dma_as, >> + dma_addr + offsetof(FWCfgDmaAccess, length)); >> + dma.control = ldl_be_dma(s->dma_as, >> + dma_addr + offsetof(FWCfgDmaAccess, control)); > > ldq_be_dma() doesn't report errors. If dma_addr is invalid the return > value could be anything. Memory corruption inside the guest is possible > if the address/length/control values happen to cause a memory read > operation!
We discussed this in a previous revision. IMHO if the guest has passed us a bogus dma_addr it should expect memory corruption. We only need to be sure we don't allow a VM escape. > Instead, please use: > > if (dma_memory_read(s->dma_as, dma_addr, &dma, sizeof(dma))) { > stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control), > FW_CFG_DMA_CTL_ERROR); If the guest handed us a bad dma_addr then this write will also be bogus and could corrupt the guest's memory. thanks -- PMM