Re: [Qemu-devel] [PATCH v4 3/7] Implement fw_cfg DMA interface

[Marc Marí]

On Thu, 8 Oct 2015 10:07:34 +0100
Stefan Hajnoczi <address@hidden> wrote:

> On Tue, Oct 06, 2015 at 03:53:33PM +0100, Peter Maydell wrote:
> > On 6 October 2015 at 15:44, Stefan Hajnoczi <address@hidden>
> > 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