On Tue, Oct 06, 2015 at 03:53:33PM +0100, Peter Maydell wrote:
> 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.
Even if the guest-visible behavior doesn't matter, Valgrind won't like
this. ldq_be_dma() reads from uninitialized stack memory:
#define DEFINE_LDST_DMA(_lname, _sname, _bits, _end) \
static inline uint##_bits##_t ld##_lname##_##_end##_dma(AddressSpace *as, \
dma_addr_t addr) \
{ \
uint##_bits##_t val; \
dma_memory_read(as, addr, &val, (_bits) / 8); \
return _end##_bits##_to_cpu(val); \
}
Bad QEMU, bad userspace process :).
I think we really need to check the error and at least return early.
> > 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.
That's fine because it's not a random address - it's the address that
the guest gave us.
Stefan
