Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer

[John Snow <jsnow@redhat.com>]

On 06/20/2018 12:29 AM, Amol Surati wrote:
> Fixes: https://bugs.launchpad.net/qemu/+bug/1777315
> 
> QEMU's short PRD policy applies to a DMA transfer of size < 512 bytes.
> But it fails to consider transfers which are >= 512 bytes, but are
> not a multiple of 512 bytes.
> 
> Such transfers are not subject to the short PRD policy. They end up
> violating the assumptions about the granularity of the IO sizes,
> upon which depend the verification of the completion of the previous
> transfer, and the advancement of the offset in preparation of the next.
> 
> Those violations result in the crash.
> 
> By forcing each transfer to be a multiple of sector size, such
> transfers are subjected to the policy, and therefore culled before they
> cause the crash.
> 

So now even if the PRDT we get is greater than a sector is not an even
multiple of 512, we reject it as too short.

That doesn't seem correct to me.

> Signed-off-by: Amol Surati <suratiamol@gmail.com>
> ---
>  hw/ide/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 2c62efc536..14d135224b 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -836,6 +836,7 @@ static void ide_dma_cb(void *opaque, int ret)
>  {
>      IDEState *s = opaque;
>      int n;
> +    int32_t size_prepared;
>      int64_t sector_num;
>      uint64_t offset;
>      bool stay_active = false;
> @@ -886,7 +887,9 @@ static void ide_dma_cb(void *opaque, int ret)
>      n = s->nsector;
>      s->io_buffer_index = 0;
>      s->io_buffer_size = n * 512;
> -    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
> +    size_prepared = s->bus->dma->ops->prepare_buf(s->bus->dma,
> +                                                  s->io_buffer_size);
> +    if (size_prepared <= 0 || size_prepared % 512) {
>          /* The PRDs were too short. Reset the Active bit, but don't raise an
>           * interrupt. */
>          s->status = READY_STAT | SEEK_STAT;
> 

-- 
—js