exec: check 'bounce.in_use' flag before using buffer

Commit Message

Prasad Pandit Jan. 28, 2016, 3:15 p.m. UTC

From: Prasad J Pandit <[e-mail]>

When IDE AHCI emulation uses Frame Information Structures(FIS)
engine for data transfer, the mapped FIS buffer address is stored
in a static 'bounce.buffer'. This is freed when FIS entry is
unmapped. If multiple FIS entries are created, it leads to an
use after free error. Check 'bounce.in_use' flag to avoid it.

Reported-by: Zuozhi fzz <[e-mail]>
Signed-off-by: Prasad J Pandit <[e-mail]>
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Peter Maydell Jan. 28, 2016, 3:30 p.m. UTC | #1

On 28 January 2016 at 15:15, P J P <[e-mail]> wrote:
> From: Prasad J Pandit <[e-mail]>
>
> When IDE AHCI emulation uses Frame Information Structures(FIS)
> engine for data transfer, the mapped FIS buffer address is stored
> in a static 'bounce.buffer'. This is freed when FIS entry is
> unmapped. If multiple FIS entries are created, it leads to an
> use after free error. Check 'bounce.in_use' flag to avoid it.
>
> Reported-by: Zuozhi fzz <[e-mail]>
> Signed-off-by: Prasad J Pandit <[e-mail]>
> ---
>  exec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/exec.c b/exec.c
> index 8718a75..ccc5715 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2922,7 +2922,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>          memory_region_unref(mr);
>          return;
>      }
> -    if (is_write) {
> +    if (bounce.in_use && is_write) {
>          address_space_write(as, bounce.addr, MEMTXATTRS_UNSPECIFIED,
>                              bounce.buffer, access_len);
>      }

This doesn't look right to me. The bounce buffer gets used
if address_space_map() is called on something which isn't
simple guest RAM. In this case address_space_map() will
set bounce.in_use to true and return bounce.buffer as the
mapped address. Then when the buffer is unmapped again,
address_space_unmap() will finish using the bounce buffer
and set bounce.in_use to false. You can only ever have one
user of the bounce buffer at a time because address_space_map()
will return NULL if it would need to use the bounce buffer
but somebody else owns it.

So if we get into address_space_unmap() with a buffer
value of bounce.buffer but bounce.in_use is false then
something has already gone wrong. We need to figure out
what that is.

thanks
-- PMM

Prasad Pandit Jan. 28, 2016, 6:09 p.m. UTC | #2

Hello Peter,

+-- On Thu, 28 Jan 2016, Peter Maydell wrote --+
| This doesn't look right to me. The bounce buffer gets used
| if address_space_map() is called on something which isn't
| simple guest RAM. In this case address_space_map() will
| set bounce.in_use to true and return bounce.buffer as the
| mapped address. Then when the buffer is unmapped again,
| address_space_unmap() will finish using the bounce buffer
| and set bounce.in_use to false. You can only ever have one
| user of the bounce buffer at a time because address_space_map()
| will return NULL if it would need to use the bounce buffer
| but somebody else owns it.
| 
| So if we get into address_space_unmap() with a buffer
| value of bounce.buffer but bounce.in_use is false then
| something has already gone wrong. We need to figure out
| what that is.

  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.
2. For second address_space_map(), it returns NULL, because 'bounce.buffer' is 
   already in_use=true.

   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.

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

   void address_space_unmap(..., void *buffer,)
   {
       if (buffer != bounce.buffer) {  <== both buffers are NULL
           ...
       }
       if (is_write) { <== is_write is true
           address_space_write(..., bounce.buffer=0x0, access_len);
            address_space_write_continue
             case 4:
                /* 32 bit write access */
                val = ldl_p(buf=0x0);
                       ldl_le_p
                        le_bswap
                         ldl_he_p
                          memcpy(&r, ptr=0x0, sizeof(r));  <== crash
       }
   }

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Peter Maydell Jan. 28, 2016, 6:15 p.m. UTC | #3

On 28 January 2016 at 18:09, P J P <[e-mail]> 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

Prasad Pandit Jan. 28, 2016, 7:01 p.m. UTC | #4

+-- On Thu, 28 Jan 2016, Peter Maydell wrote --+
| ahci code should never be passing it to address_space_unmap()
| (or indeed doing anything with it at all).

  Okay.
 
| 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;
|         }

  Sorry, I think I mixed 'map_fis' & '*map_clb*'. It fails little earlier and 
throws
       error_report("AHCI: Failed to start DMA engine: "                   
                         "bad command list buffer address");
 
| 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.)

  Okay, I'll send a revised patch.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Prasad Pandit Jan. 28, 2016, 7:53 p.m. UTC | #5

+-- On Fri, 29 Jan 2016, P J P wrote --+
|   Okay, I'll send a revised patch.

I've sent it. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Patch

diff --git a/exec.c b/exec.c
index 8718a75..ccc5715 100644
--- a/exec.c
+++ b/exec.c
@@ -2922,7 +2922,7 @@  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
         memory_region_unref(mr);
         return;
     }
-    if (is_write) {
+    if (bounce.in_use && is_write) {
         address_space_write(as, bounce.addr, MEMTXATTRS_UNSPECIFIED,
                             bounce.buffer, access_len);
     }