Fix qemu_bh_schedule race condition (bz #1165315)

CVE-2014-8106: cirrus: insufficient blit region checks
This commit is contained in:
Cole Robinson 2015-02-07 21:12:53 -05:00
parent f4055dd206
commit c700dd0ffc
4 changed files with 217 additions and 1 deletions

View File

@ -0,0 +1,53 @@
From: Stefan Hajnoczi <stefanha@redhat.com>
Date: Tue, 3 Jun 2014 11:21:01 +0200
Subject: [PATCH] aio: fix qemu_bh_schedule() bh->ctx race condition
qemu_bh_schedule() is supposed to be thread-safe at least the first time
it is called. Unfortunately this is not quite true:
bh->scheduled = 1;
aio_notify(bh->ctx);
Since another thread may run the BH callback once it has been scheduled,
there is a race condition if the callback frees the BH before
aio_notify(bh->ctx) has a chance to run.
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Stefan Priebe <s.priebe@profihost.ag>
(cherry picked from commit 924fe1293c3e7a3c787bbdfb351e7f168caee3e9)
---
async.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/async.c b/async.c
index 5ce3633..d7ec1ea 100644
--- a/async.c
+++ b/async.c
@@ -117,15 +117,21 @@ void qemu_bh_schedule_idle(QEMUBH *bh)
void qemu_bh_schedule(QEMUBH *bh)
{
+ AioContext *ctx;
+
if (bh->scheduled)
return;
+ ctx = bh->ctx;
bh->idle = 0;
- /* Make sure that idle & any writes needed by the callback are done
- * before the locations are read in the aio_bh_poll.
+ /* Make sure that:
+ * 1. idle & any writes needed by the callback are done before the
+ * locations are read in the aio_bh_poll.
+ * 2. ctx is loaded before scheduled is set and the callback has a chance
+ * to execute.
*/
- smp_wmb();
+ smp_mb();
bh->scheduled = 1;
- aio_notify(bh->ctx);
+ aio_notify(ctx);
}

View File

@ -0,0 +1,122 @@
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 19 Nov 2014 11:37:42 +0100
Subject: [PATCH] cirrus: fix blit region check
Issues:
* Doesn't check pitches correctly in case it is negative.
* Doesn't check width at all.
Turn macro into functions while being at it, also factor out the check
for one region which we then can simply call twice for src + dst.
This is CVE-2014-8106.
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit d3532a0db02296e687711b8cdc7791924efccea0)
---
hw/display/cirrus_vga.c | 61 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 44 insertions(+), 17 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index dbd1f4a..64e489e 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -172,20 +172,6 @@
#define CIRRUS_PNPMMIO_SIZE 0x1000
-#define BLTUNSAFE(s) \
- ( \
- ( /* check dst is within bounds */ \
- (s)->cirrus_blt_height * ABS((s)->cirrus_blt_dstpitch) \
- + ((s)->cirrus_blt_dstaddr & (s)->cirrus_addr_mask) > \
- (s)->vga.vram_size \
- ) || \
- ( /* check src is within bounds */ \
- (s)->cirrus_blt_height * ABS((s)->cirrus_blt_srcpitch) \
- + ((s)->cirrus_blt_srcaddr & (s)->cirrus_addr_mask) > \
- (s)->vga.vram_size \
- ) \
- )
-
struct CirrusVGAState;
typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
uint8_t * dst, const uint8_t * src,
@@ -278,6 +264,46 @@ static void cirrus_update_memory_access(CirrusVGAState *s);
*
***************************************/
+static bool blit_region_is_unsafe(struct CirrusVGAState *s,
+ int32_t pitch, int32_t addr)
+{
+ if (pitch < 0) {
+ int64_t min = addr
+ + ((int64_t)s->cirrus_blt_height-1) * pitch;
+ int32_t max = addr
+ + s->cirrus_blt_width;
+ if (min < 0 || max >= s->vga.vram_size) {
+ return true;
+ }
+ } else {
+ int64_t max = addr
+ + ((int64_t)s->cirrus_blt_height-1) * pitch
+ + s->cirrus_blt_width;
+ if (max >= s->vga.vram_size) {
+ return true;
+ }
+ }
+ return false;
+}
+
+static bool blit_is_unsafe(struct CirrusVGAState *s)
+{
+ /* should be the case, see cirrus_bitblt_start */
+ assert(s->cirrus_blt_width > 0);
+ assert(s->cirrus_blt_height > 0);
+
+ if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
+ s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
+ return true;
+ }
+ if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
+ s->cirrus_blt_srcaddr & s->cirrus_addr_mask)) {
+ return true;
+ }
+
+ return false;
+}
+
static void cirrus_bitblt_rop_nop(CirrusVGAState *s,
uint8_t *dst,const uint8_t *src,
int dstpitch,int srcpitch,
@@ -635,7 +661,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
dst = s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask);
- if (BLTUNSAFE(s))
+ if (blit_is_unsafe(s))
return 0;
(*s->cirrus_rop) (s, dst, src,
@@ -653,8 +679,9 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
{
cirrus_fill_t rop_func;
- if (BLTUNSAFE(s))
+ if (blit_is_unsafe(s)) {
return 0;
+ }
rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
rop_func(s, s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask),
s->cirrus_blt_dstpitch,
@@ -751,7 +778,7 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
{
- if (BLTUNSAFE(s))
+ if (blit_is_unsafe(s))
return 0;
cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,

View File

@ -0,0 +1,27 @@
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 19 Nov 2014 13:27:28 +0100
Subject: [PATCH] cirrus: don't overflow CirrusVGAState->cirrus_bltbuf
This is CVE-2014-8106.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
(cherry picked from commit bf25983345ca44aec3dd92c57142be45452bd38a)
---
hw/display/cirrus_vga.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 64e489e..f16960b 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -292,6 +292,10 @@ static bool blit_is_unsafe(struct CirrusVGAState *s)
assert(s->cirrus_blt_width > 0);
assert(s->cirrus_blt_height > 0);
+ if (s->cirrus_blt_width > CIRRUS_BLTBUFSIZE) {
+ return true;
+ }
+
if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
return true;

View File

@ -139,7 +139,7 @@
Summary: QEMU is a FAST! processor emulator
Name: qemu
Version: 1.6.2
Release: 12%{?dist}
Release: 13%{?dist}
Epoch: 2
License: GPLv2+ and LGPLv2+ and BSD
Group: Development/Tools
@ -380,6 +380,11 @@ Patch0423: 0423-migration-fix-parameter-validation-on-ram-load.patch
# Fix qemu-img convert corruption for unflushed files (bz #1167249)
Patch0424: 0424-block-raw-posix-Fix-disk-corruption-in-try_fiemap.patch
Patch0425: 0425-raw-posix-Drop-fiemap.patch
# Fix qemu_bh_schedule race condition (bz #1165315)
Patch0426: 0426-aio-fix-qemu_bh_schedule-bh-ctx-race-condition.patch
# CVE-2014-8106: cirrus: insufficient blit region checks
Patch0427: 0427-cirrus-fix-blit-region-check.patch
Patch0428: 0428-cirrus-don-t-overflow-CirrusVGAState-cirrus_bltbuf.patch
BuildRequires: SDL-devel
BuildRequires: zlib-devel
@ -1092,6 +1097,11 @@ CAC emulation development files.
# Fix qemu-img convert corruption for unflushed files (bz #1167249)
%patch0424 -p1
%patch0425 -p1
# Fix qemu_bh_schedule race condition (bz #1165315)
%patch0426 -p1
# CVE-2014-8106: cirrus: insufficient blit region checks
%patch0427 -p1
%patch0428 -p1
%build
@ -1799,6 +1809,10 @@ getent passwd qemu >/dev/null || \
%endif
%changelog
* Sat Feb 07 2015 Cole Robinson <crobinso@redhat.com> - 2:1.6.2-13
- Fix qemu_bh_schedule race condition (bz #1165315)
- CVE-2014-8106: cirrus: insufficient blit region checks
* Tue Dec 09 2014 Cole Robinson <crobinso@redhat.com> - 2:1.6.2-12
- Fix qemu-img convert corruption for unflushed files (bz #1167249)