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

["Marc Marí" <markmb@redhat.com>]

On Tue, 6 Oct 2015 15:44:53 +0100
Stefan Hajnoczi <stefanha@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!
> 
> 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.

This was proposed here:

http://lists.gnu.org/archive/html/qemu-devel/2015-08/msg04001.html

I also don't see much benefit into knowing how many bytes were read. If
the guest is trying to read an invalid entry or past the end of that
entry, the memory will be filled with zeros. The only moment when an
error would be reported is when there's some problem in the mapping.
And this problem is bad enough to just abort the whole operation.

Regards
Marc