| Message ID | 1453994125-23586-1-git-send-email-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 28 January 2016 at 15:15, P J P <ppandit@redhat.com> wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > 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 <zuozhi.fzz@alibaba-inc.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > 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
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
On 28 January 2016 at 18:09, P J P <ppandit@redhat.com> 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
+-- 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
+-- 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
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); }