[RFC,04/10] ide: Correct handling of malformed/short PRDTs

This impacts both BMDMA and AHCI HBA interfaces for IDE.
Currently, we confuse the difference between a PRD having
"0 bytes" and a PRD having "0 complete sectors."

This leads to, in the BMDMA case, leaked memory for short PRDTs,
and infinite loops in the AHCI case.

the "prepare_buf" callback is reworked to return 0 if it could
not allocate a full sector's worth of buffer space, instead of
returning non-zero if it allocated any number of bytes.

ide_dma_cb adds a call to commit_buf in order to delete
the short PRDT that it will not attempt to use to finish
the DMA operation.

This patch corrects both occurrences and adds an assertion to
prevent future regression. This assertion is tested in the
existing ide-test, and is covered in a forthcoming AHCI test.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 dma-helpers.c | 3 +++
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 1 +
 hw/ide/pci.c  | 5 +++--
 4 files changed, 8 insertions(+), 3 deletions(-)

Il 13/09/2014 06:34, John Snow ha scritto:
> This impacts both BMDMA and AHCI HBA interfaces for IDE.
> Currently, we confuse the difference between a PRD having
> "0 bytes" and a PRD having "0 complete sectors."
> 
> This leads to, in the BMDMA case, leaked memory for short PRDTs,
> and infinite loops in the AHCI case.
> 
> the "prepare_buf" callback is reworked to return 0 if it could
> not allocate a full sector's worth of buffer space, instead of
> returning non-zero if it allocated any number of bytes.
> 
> ide_dma_cb adds a call to commit_buf in order to delete
> the short PRDT that it will not attempt to use to finish
> the DMA operation.
> 
> This patch corrects both occurrences and adds an assertion to
> prevent future regression. This assertion is tested in the
> existing ide-test, and is covered in a forthcoming AHCI test.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  dma-helpers.c | 3 +++
>  hw/ide/ahci.c | 2 +-
>  hw/ide/core.c | 1 +
>  hw/ide/pci.c  | 5 +++--
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index ba965a3..3f9766d 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
>  void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
>                        AddressSpace *as)
>  {
> +    /* If this is true, you're leaking memory. */
> +    assert(qsg->sg == NULL);
> +
>      qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>      qsg->nsg = 0;
>      qsg->nalloc = alloc_hint;
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 8e6a352..42a77c4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1132,7 +1132,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
>      s->io_buffer_size = s->sg.size;
>  
>      DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
> -    return s->io_buffer_size != 0;
> +    return s->io_buffer_size / 512 != 0;
>  }
>  
>  /**
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b2980e9..1685f6d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -726,6 +726,7 @@ void ide_dma_cb(void *opaque, int ret)
>          /* The PRDs were too short. Reset the Active bit, but don't raise an
>           * interrupt. */
>          s->status = READY_STAT | SEEK_STAT;
> +        dma_buf_commit(s, false);
>          goto eot;
>      }
>  
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 2397f35..3f643c2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
>          if (bm->cur_prd_len == 0) {
>              /* end of table (with a fail safe of one page) */
>              if (bm->cur_prd_last ||
> -                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
> -                return s->io_buffer_size != 0;
> +                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
> +                return (s->io_buffer_size / 512) != 0;
> +            }
>              pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
>              bm->cur_addr += 8;
>              prd.addr = le32_to_cpu(prd.addr);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

The changes I suggested in patch 2 shouldn't be a hurdle here.

Paolo