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

[Stefan Hajnoczi]

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.

> > 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