On Thu, 8 Oct 2015 10:07:34 +0100
Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 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.
It doesn't hurt to check the error. I'll add it.
Thanks
Marc
> > > 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
