* [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer
@ 2016-01-28 15:15 P J P
2016-01-28 15:30 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-01-28 15:15 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, John Snow, Prasad J Pandit, Zuozhi fzz
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);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer
2016-01-28 15:15 [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer P J P
@ 2016-01-28 15:30 ` Peter Maydell
2016-01-28 18:09 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2016-01-28 15:30 UTC (permalink / raw)
To: P J P
Cc: Paolo Bonzini, Zuozhi fzz, John Snow, QEMU Developers,
Prasad J Pandit
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer
2016-01-28 15:30 ` Peter Maydell
@ 2016-01-28 18:09 ` P J P
2016-01-28 18:15 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-01-28 18:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, John Snow, QEMU Developers, Zuozhi fzz
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer
2016-01-28 18:09 ` P J P
@ 2016-01-28 18:15 ` Peter Maydell
2016-01-28 19:01 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2016-01-28 18:15 UTC (permalink / raw)
To: P J P; +Cc: Paolo Bonzini, John Snow, QEMU Developers, Zuozhi fzz
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer
2016-01-28 18:15 ` Peter Maydell
@ 2016-01-28 19:01 ` P J P
2016-01-28 19:53 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2016-01-28 19:01 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, John Snow, QEMU Developers, Zuozhi fzz
+-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-28 19:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-28 15:15 [Qemu-devel] [PATCH] exec: check 'bounce.in_use' flag before using buffer P J P
2016-01-28 15:30 ` Peter Maydell
2016-01-28 18:09 ` P J P
2016-01-28 18:15 ` Peter Maydell
2016-01-28 19:01 ` P J P
2016-01-28 19:53 ` P J P
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).