Re: [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer

[Peter Maydell]

On 28 January 2016 at 18:09, P J P <address@hidden> wrote:
>   Yes, this is exactly same case, except that 'bounce.buffer' is NULL; It does
> not point to a valid address.
>
> 1. For first address_space_map() everything goes well and 'bounce.buffer' is
>    allocated.

OK

> 2. For second address_space_map(), it returns NULL, because 'bounce.buffer' is
>    already in_use=true.

OK

>
>    ahci_port_write
>     ahci_cond_start_engines
>      ahci_map_fis_address
>       map_page
>        dma_memory_map
>         address_space_map  <== returns NULL
>
> 3. For first address_space_unmap() everything goes well and 'bounce.buffer' is
>    set to NULL and 'bounce.in_use' is set to false.

OK

> 4. For the second address_space_unmap(), the 'buffer' parameter is NULL
>    because second address_space_map() returned NULL.

This is where something has gone wrong -- a NULL return means that
address_space_map() *failed*. It is not a valid mapping and the
ahci code should never be passing it to address_space_unmap()
(or indeed doing anything with it at all).

Instead it needs to handle it as an error case. But it looks like
ahci_cond_start_engines() already does that:

        if (ahci_map_fis_address(ad)) {
            pr->cmd |= PORT_CMD_FIS_ON;
        } else {
            error_report("AHCI: Failed to start FIS receive engine: "
                         "bad FIS receive buffer address");
            return -1;
        }

so perhaps the problem is that it's not correctly tracking
whether the mapping succeeded in order to avoid unmapping
something that was never mapped.

I suspect that the correct fix to this is that
ahci_unmap_fis_address() should only call dma_memory_unmap()
if ad->res_fis is not NULL. (Other calls to dma_memory_unmap()
in this file also need checking to see if they should have
similar guards.)

thanks
-- PMM