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

[Stefan Hajnoczi]

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.