|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings
On Tue, 11 Jul 2017, Alexey G wrote:
> Under certain circumstances normal xen-mapcache functioning may be broken
> by guest's actions. This may lead to either QEMU performing exit() due to
> a caught bad pointer (and with QEMU process gone the guest domain simply
> appears hung afterwards) or actual use of the incorrect pointer inside
> QEMU address space -- a write to unmapped memory is possible. The bug is
> hard to reproduce on a i440 machine as multiple DMA sources are required
> (though it's possible in theory, using multiple emulated devices), but can
> be reproduced somewhat easily on a Q35 machine using an emulated AHCI
> controller -- each NCQ queue command slot may be used as an independent
> DMA source ex. using READ FPDMA QUEUED command, so a single storage
> device on the AHCI controller port will be enough to produce multiple DMAs
> (up to 32). The detailed description of the issue follows.
>
> Xen-mapcache provides an ability to map parts of a guest memory into
> QEMU's own address space to work with.
>
> There are two types of cache lookups:
> - translating a guest physical address into a pointer in QEMU's address
> space, mapping a part of guest domain memory if necessary (while trying
> to reduce a number of such (re)mappings to a minimum)
> - translating a QEMU's pointer back to its physical address in guest RAM
>
> These lookups are managed via two linked-lists of structures.
> MapCacheEntry is used for forward cache lookups, while MapCacheRev -- for
> reverse lookups.
>
> Every guest physical address is broken down into 2 parts:
> address_index = phys_addr >> MCACHE_BUCKET_SHIFT;
> address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
>
> MCACHE_BUCKET_SHIFT depends on a system (32/64) and is equal to 20 for
> a 64-bit system (which assumed for the further description). Basically,
> this means that we deal with 1 MB chunks and offsets within those 1 MB
> chunks. All mappings are created with 1MB-granularity, i.e. 1MB/2MB/3MB
> etc. Most DMA transfers typically are less than 1MB, however, if the
> transfer crosses any 1MB border(s) - than a nearest larger mapping size
> will be used, so ex. a 512-byte DMA transfer with the start address
> 700FFF80h will actually require a 2MB range.
>
> Current implementation assumes that MapCacheEntries are unique for a given
> address_index and size pair and that a single MapCacheEntry may be reused
> by multiple requests -- in this case the 'lock' field will be larger than
> 1. On other hand, each requested guest physical address (with 'lock' flag)
> is described by each own MapCacheRev. So there may be multiple MapCacheRev
> entries corresponding to a single MapCacheEntry. The xen-mapcache code
> uses MapCacheRev entries to retrieve the address_index & size pair which
> in turn used to find a related MapCacheEntry. The 'lock' field within
> a MapCacheEntry structure is actually a reference counter which shows
> a number of corresponding MapCacheRev entries.
>
> The bug lies in ability for the guest to indirectly manipulate with the
> xen-mapcache MapCacheEntries list via a special sequence of DMA
> operations, typically for storage devices. In order to trigger the bug,
> guest needs to issue DMA operations in specific order and timing.
> Although xen-mapcache is protected by the mutex lock -- this doesn't help
> in this case, as the bug is not due to a race condition.
>
> Suppose we have 3 DMA transfers, namely A, B and C, where
> - transfer A crosses 1MB border and thus uses a 2MB mapping
> - transfers B and C are normal transfers within 1MB range
> - and all 3 transfers belong to the same address_index
>
> In this case, if all these transfers are to be executed one-by-one
> (without overlaps), no special treatment necessary -- each transfer's
> mapping lock will be set and then cleared on unmap before starting
> the next transfer.
> The situation changes when DMA transfers overlap in time, ex. like this:
>
> |===== transfer A (2MB) =====|
>
> |===== transfer B (1MB) =====|
>
> |===== transfer C (1MB) =====|
> time --->
>
> In this situation the following sequence of actions happens:
>
> 1. transfer A creates a mapping to 2MB area (lock=1)
> 2. transfer B (1MB) tries to find available mapping but cannot find one
> because transfer A is still in progress, and it has 2MB size + non-zero
> lock. So transfer B creates another mapping -- same address_index,
> but 1MB size.
> 3. transfer A completes, making 1st mapping entry available by setting its
> lock to 0
> 4. transfer C starts and tries to find available mapping entry and sees
> that 1st entry has lock=0, so it uses this entry but remaps the mapping
> to a 1MB size
> 5. transfer B completes and by this time
> - there are two locked entries in the MapCacheEntry list with the SAME
> values for both address_index and size
> - the entry for transfer B actually resides farther in list while
> transfer C's entry is first
> 6. xen_ram_addr_from_mapcache() for transfer B gets correct address_index
> and size pair from corresponding MapCacheRev entry, but then it starts
> looking for MapCacheEntry with these values and finds the first entry
> -- which belongs to transfer C.
>
> At this point there may be following possible (bad) consequences:
>
> 1. xen_ram_addr_from_mapcache() will use a wrong entry->vaddr_base value
> in this statement:
>
> raddr = (reventry->paddr_index << MCACHE_BUCKET_SHIFT) +
> ((unsigned long) ptr - (unsigned long) entry->vaddr_base);
>
> resulting in an incorrent raddr value returned from the function. The
> (ptr - entry->vaddr_base) expression may produce both positive and negative
> numbers and its actual value may differ greatly as there are many
> map/unmap operations take place. If the value will be beyond guest RAM
> limits then a "Bad RAM offset" error will be triggered and logged,
> followed by exit() in QEMU.
>
> 2. If raddr value won't exceed guest RAM boundaries, the same sequence
> of actions will be performed for xen_invalidate_map_cache_entry() on DMA
> unmap, resulting in a wrong MapCacheEntry being unmapped while DMA
> operation which uses it is still active. The above example must
> be extended by one more DMA transfer in order to allow unmapping as the
> first mapping in the list is sort of resident.
Thanks for the well written description of the problem!
> The patch modifies the behavior in which MapCacheEntry's are added to the
> list, avoiding duplicates.
I take that the idea is to always go through the whole list to check for
duplicate locked entries, right?
> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
> ---
> hw/i386/xen/xen-mapcache.c | 33 ++++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index e60156c..84f25ef 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -206,6 +206,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr,
> hwaddr size,
> uint8_t lock, bool dma)
> {
> MapCacheEntry *entry, *pentry = NULL;
> + MapCacheEntry *avl_entry = NULL, *avl_entry_prev = NULL;
> hwaddr address_index;
> hwaddr address_offset;
> hwaddr cache_size = size;
> @@ -251,14 +252,36 @@ tryagain:
>
> entry = &mapcache->entry[address_index % mapcache->nr_buckets];
>
> - while (entry && entry->lock && entry->vaddr_base &&
> - (entry->paddr_index != address_index || entry->size !=
> cache_size ||
> - !test_bits(address_offset >> XC_PAGE_SHIFT,
> - test_bit_size >> XC_PAGE_SHIFT,
> - entry->valid_mapping))) {
> + /* find a remappable entry. An existing locked entry which can be reused
> + * has a priority over all other entries (with lock=0, etc).
> + * Normally there will be just few entries for a given address_index
> + * bucket, typically 1-2 entries only
> + */
> + while (entry) {
> + if (entry->lock &&
> + entry->paddr_index == address_index &&
> + entry->size == cache_size &&
> + test_bits(address_offset >> XC_PAGE_SHIFT,
> + test_bit_size >> XC_PAGE_SHIFT, entry->valid_mapping)) {
> + break;
> + }
> + else if (!entry->lock || !entry->vaddr_base) {
> + avl_entry = entry;
> + avl_entry_prev = pentry;
> + }
> +
> pentry = entry;
> entry = entry->next;
> }
> +
> + /* if the reuseable entry was not found, use any available.
> + * Otherwise, a new entry will be created
> + */
> + if (avl_entry && !entry) {
> + pentry = avl_entry_prev;
> + entry = avl_entry;
> + }
Yes, I think this would work, but we should make sure to scan the whole
list only when lock == true. Something like the following:
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 2a1fbd1..f964143 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -234,7 +234,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
uint8_t lock, bool dma)
{
- MapCacheEntry *entry, *pentry = NULL;
+ MapCacheEntry *entry, *pentry = NULL,
+ *free_entry = NULL, *free_pentry = NULL;
hwaddr address_index;
hwaddr address_offset;
hwaddr cache_size = size;
@@ -281,14 +282,22 @@ tryagain:
entry = &mapcache->entry[address_index % mapcache->nr_buckets];
- while (entry && entry->lock && entry->vaddr_base &&
+ while (entry && (lock || entry->lock) && entry->vaddr_base &&
(entry->paddr_index != address_index || entry->size != cache_size
||
!test_bits(address_offset >> XC_PAGE_SHIFT,
test_bit_size >> XC_PAGE_SHIFT,
entry->valid_mapping))) {
+ if (!free_entry && !entry->lock) {
+ free_entry = entry;
+ free_pentry = pentry;
+ }
pentry = entry;
entry = entry->next;
}
+ if (!entry && free_entry) {
+ entry = free_entry;
+ pentry = free_pentry;
+ }
if (!entry) {
entry = g_malloc0(sizeof (MapCacheEntry));
pentry->next = entry;
Would this work?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |