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! 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); return; } dma.address = be64_to_cpu(dma.address); dma.length = be32_to_cpu(dma.length); dma.control = be32_to_cpu(dma.control); > + > + if (dma.control & FW_CFG_DMA_CTL_SELECT) { > + fw_cfg_select(s, dma.control >> 16); > + } > + > + arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > + e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > + > + if (dma.control & FW_CFG_DMA_CTL_READ) { > + read = 1; > + } else if (dma.control & FW_CFG_DMA_CTL_SKIP) { > + read = 0; > + } else { > + dma.length = 0; > + } > + > + dma.control = 0; > + > + while (dma.length > 0 && !(dma.control & FW_CFG_DMA_CTL_ERROR)) { > + if (s->cur_entry == FW_CFG_INVALID || !e->data || > + s->cur_offset >= e->len) { > + len = dma.length; > + > + /* If the access is not a read access, it will be a skip access, > + * tested before. > + */ > + if (read) { > + if (dma_memory_set(s->dma_as, dma.address, 0, len)) { > + dma.control |= FW_CFG_DMA_CTL_ERROR; > + } > + } > + > + } else { > + if (dma.length <= (e->len - s->cur_offset)) { > + len = dma.length; > + } else { > + len = (e->len - s->cur_offset); > + } > + > + if (e->read_callback) { > + e->read_callback(e->callback_opaque, s->cur_offset); > + } > + > + /* If the access is not a read access, it will be a skip access, > + * tested before. > + */ > + if (read) { > + if (dma_memory_write(s->dma_as, dma.address, > + &e->data[s->cur_offset], len)) { > + dma.control |= FW_CFG_DMA_CTL_ERROR; > + } > + } > + > + s->cur_offset += len; > + } > + > + dma.address += len; > + dma.length -= len; I thought these fields are written back to guest memory? For example, so the guest knows how many bytes were read before the error occurred.