* [PATCH v2] xen-mapcache: Avoid entry->lock overflow
@ 2022-01-24 10:44 ` Ross Lagerwall via
0 siblings, 0 replies; 6+ messages in thread
From: Ross Lagerwall @ 2022-01-24 10:44 UTC (permalink / raw)
To: Stefano Stabellini, Anthony Perard, Paul Durrant
Cc: Ross Lagerwall, Igor Druzhinin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum, xen-devel,
qemu-devel
In some cases, a particular mapcache entry may be mapped 256 times
causing the lock field to wrap to 0. For example, this may happen when
using emulated NVME and the guest submits a large scatter-gather write.
At this point, the entry map be remapped causing QEMU to write the wrong
data or crash (since remap is not atomic).
Avoid this overflow by increasing the lock field to a uint32_t and also
detect it and abort rather than continuing regardless.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes in v2: Change type to uint32_t since there is a hole there
anyway. The struct size remains at 48 bytes on x86_64.
hw/i386/xen/xen-mapcache.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index bd47c3d672..f2ef977963 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
hwaddr paddr_index;
uint8_t *vaddr_base;
unsigned long *valid_mapping;
- uint8_t lock;
+ uint32_t lock;
#define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
uint8_t flags;
hwaddr size;
@@ -355,6 +355,12 @@ tryagain:
if (lock) {
MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
entry->lock++;
+ if (entry->lock == 0) {
+ fprintf(stderr,
+ "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
+ entry->paddr_index, entry->vaddr_base);
+ abort();
+ }
reventry->dma = dma;
reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
reventry->paddr_index = mapcache->last_entry->paddr_index;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] xen-mapcache: Avoid entry->lock overflow
@ 2022-01-24 10:44 ` Ross Lagerwall via
0 siblings, 0 replies; 6+ messages in thread
From: Ross Lagerwall via @ 2022-01-24 10:44 UTC (permalink / raw)
To: Stefano Stabellini, Anthony Perard, Paul Durrant
Cc: Ross Lagerwall, Igor Druzhinin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum, xen-devel,
qemu-devel
In some cases, a particular mapcache entry may be mapped 256 times
causing the lock field to wrap to 0. For example, this may happen when
using emulated NVME and the guest submits a large scatter-gather write.
At this point, the entry map be remapped causing QEMU to write the wrong
data or crash (since remap is not atomic).
Avoid this overflow by increasing the lock field to a uint32_t and also
detect it and abort rather than continuing regardless.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
Changes in v2: Change type to uint32_t since there is a hole there
anyway. The struct size remains at 48 bytes on x86_64.
hw/i386/xen/xen-mapcache.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index bd47c3d672..f2ef977963 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
hwaddr paddr_index;
uint8_t *vaddr_base;
unsigned long *valid_mapping;
- uint8_t lock;
+ uint32_t lock;
#define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
uint8_t flags;
hwaddr size;
@@ -355,6 +355,12 @@ tryagain:
if (lock) {
MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
entry->lock++;
+ if (entry->lock == 0) {
+ fprintf(stderr,
+ "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
+ entry->paddr_index, entry->vaddr_base);
+ abort();
+ }
reventry->dma = dma;
reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
reventry->paddr_index = mapcache->last_entry->paddr_index;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xen-mapcache: Avoid entry->lock overflow
2022-01-24 10:44 ` Ross Lagerwall via
@ 2022-01-24 16:43 ` Stefano Stabellini
-1 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2022-01-24 16:43 UTC (permalink / raw)
To: Ross Lagerwall
Cc: Stefano Stabellini, Anthony Perard, Paul Durrant, Igor Druzhinin,
Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Michael S. Tsirkin, Marcel Apfelbaum, xen-devel, qemu-devel
On Mon, 24 Jan 2022, Ross Lagerwall wrote:
> In some cases, a particular mapcache entry may be mapped 256 times
> causing the lock field to wrap to 0. For example, this may happen when
> using emulated NVME and the guest submits a large scatter-gather write.
> At this point, the entry map be remapped causing QEMU to write the wrong
> data or crash (since remap is not atomic).
>
> Avoid this overflow by increasing the lock field to a uint32_t and also
> detect it and abort rather than continuing regardless.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2: Change type to uint32_t since there is a hole there
> anyway. The struct size remains at 48 bytes on x86_64.
>
> hw/i386/xen/xen-mapcache.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index bd47c3d672..f2ef977963 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
> hwaddr paddr_index;
> uint8_t *vaddr_base;
> unsigned long *valid_mapping;
> - uint8_t lock;
> + uint32_t lock;
> #define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
> uint8_t flags;
> hwaddr size;
> @@ -355,6 +355,12 @@ tryagain:
> if (lock) {
> MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
> entry->lock++;
> + if (entry->lock == 0) {
> + fprintf(stderr,
> + "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
> + entry->paddr_index, entry->vaddr_base);
> + abort();
> + }
> reventry->dma = dma;
> reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
> reventry->paddr_index = mapcache->last_entry->paddr_index;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xen-mapcache: Avoid entry->lock overflow
@ 2022-01-24 16:43 ` Stefano Stabellini
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2022-01-24 16:43 UTC (permalink / raw)
To: Ross Lagerwall
Cc: Eduardo Habkost, Igor Druzhinin, Stefano Stabellini,
Paul Durrant, Michael S. Tsirkin, Richard Henderson, qemu-devel,
xen-devel, Anthony Perard, Paolo Bonzini
On Mon, 24 Jan 2022, Ross Lagerwall wrote:
> In some cases, a particular mapcache entry may be mapped 256 times
> causing the lock field to wrap to 0. For example, this may happen when
> using emulated NVME and the guest submits a large scatter-gather write.
> At this point, the entry map be remapped causing QEMU to write the wrong
> data or crash (since remap is not atomic).
>
> Avoid this overflow by increasing the lock field to a uint32_t and also
> detect it and abort rather than continuing regardless.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2: Change type to uint32_t since there is a hole there
> anyway. The struct size remains at 48 bytes on x86_64.
>
> hw/i386/xen/xen-mapcache.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index bd47c3d672..f2ef977963 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
> hwaddr paddr_index;
> uint8_t *vaddr_base;
> unsigned long *valid_mapping;
> - uint8_t lock;
> + uint32_t lock;
> #define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
> uint8_t flags;
> hwaddr size;
> @@ -355,6 +355,12 @@ tryagain:
> if (lock) {
> MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
> entry->lock++;
> + if (entry->lock == 0) {
> + fprintf(stderr,
> + "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
> + entry->paddr_index, entry->vaddr_base);
> + abort();
> + }
> reventry->dma = dma;
> reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
> reventry->paddr_index = mapcache->last_entry->paddr_index;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xen-mapcache: Avoid entry->lock overflow
2022-01-24 10:44 ` Ross Lagerwall via
@ 2022-01-25 13:55 ` Durrant, Paul
-1 siblings, 0 replies; 6+ messages in thread
From: Durrant, Paul @ 2022-01-25 13:55 UTC (permalink / raw)
To: Ross Lagerwall, Stefano Stabellini, Anthony Perard, Paul Durrant
Cc: Igor Druzhinin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum, xen-devel,
qemu-devel
On 24/01/2022 10:44, Ross Lagerwall wrote:
> In some cases, a particular mapcache entry may be mapped 256 times
> causing the lock field to wrap to 0. For example, this may happen when
> using emulated NVME and the guest submits a large scatter-gather write.
> At this point, the entry map be remapped causing QEMU to write the wrong
> data or crash (since remap is not atomic).
>
> Avoid this overflow by increasing the lock field to a uint32_t and also
> detect it and abort rather than continuing regardless.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Paul Durrant <paul@xen.org>
> ---
> Changes in v2: Change type to uint32_t since there is a hole there
> anyway. The struct size remains at 48 bytes on x86_64.
>
> hw/i386/xen/xen-mapcache.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index bd47c3d672..f2ef977963 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
> hwaddr paddr_index;
> uint8_t *vaddr_base;
> unsigned long *valid_mapping;
> - uint8_t lock;
> + uint32_t lock;
> #define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
> uint8_t flags;
> hwaddr size;
> @@ -355,6 +355,12 @@ tryagain:
> if (lock) {
> MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
> entry->lock++;
> + if (entry->lock == 0) {
> + fprintf(stderr,
> + "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
> + entry->paddr_index, entry->vaddr_base);
> + abort();
> + }
> reventry->dma = dma;
> reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
> reventry->paddr_index = mapcache->last_entry->paddr_index;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xen-mapcache: Avoid entry->lock overflow
@ 2022-01-25 13:55 ` Durrant, Paul
0 siblings, 0 replies; 6+ messages in thread
From: Durrant, Paul @ 2022-01-25 13:55 UTC (permalink / raw)
To: Ross Lagerwall, Stefano Stabellini, Anthony Perard, Paul Durrant
Cc: Eduardo Habkost, Igor Druzhinin, Michael S. Tsirkin,
Richard Henderson, qemu-devel, xen-devel, Paolo Bonzini
On 24/01/2022 10:44, Ross Lagerwall wrote:
> In some cases, a particular mapcache entry may be mapped 256 times
> causing the lock field to wrap to 0. For example, this may happen when
> using emulated NVME and the guest submits a large scatter-gather write.
> At this point, the entry map be remapped causing QEMU to write the wrong
> data or crash (since remap is not atomic).
>
> Avoid this overflow by increasing the lock field to a uint32_t and also
> detect it and abort rather than continuing regardless.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Paul Durrant <paul@xen.org>
> ---
> Changes in v2: Change type to uint32_t since there is a hole there
> anyway. The struct size remains at 48 bytes on x86_64.
>
> hw/i386/xen/xen-mapcache.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index bd47c3d672..f2ef977963 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -52,7 +52,7 @@ typedef struct MapCacheEntry {
> hwaddr paddr_index;
> uint8_t *vaddr_base;
> unsigned long *valid_mapping;
> - uint8_t lock;
> + uint32_t lock;
> #define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
> uint8_t flags;
> hwaddr size;
> @@ -355,6 +355,12 @@ tryagain:
> if (lock) {
> MapCacheRev *reventry = g_malloc0(sizeof(MapCacheRev));
> entry->lock++;
> + if (entry->lock == 0) {
> + fprintf(stderr,
> + "mapcache entry lock overflow: "TARGET_FMT_plx" -> %p\n",
> + entry->paddr_index, entry->vaddr_base);
> + abort();
> + }
> reventry->dma = dma;
> reventry->vaddr_req = mapcache->last_entry->vaddr_base + address_offset;
> reventry->paddr_index = mapcache->last_entry->paddr_index;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-25 15:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-24 10:44 [PATCH v2] xen-mapcache: Avoid entry->lock overflow Ross Lagerwall
2022-01-24 10:44 ` Ross Lagerwall via
2022-01-24 16:43 ` Stefano Stabellini
2022-01-24 16:43 ` Stefano Stabellini
2022-01-25 13:55 ` Durrant, Paul
2022-01-25 13:55 ` Durrant, Paul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.