* [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops"
2017-02-09 13:02 [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Gerd Hoffmann
@ 2017-02-09 13:02 ` Gerd Hoffmann
2017-02-09 14:52 ` Dr. David Alan Gilbert
2017-02-10 11:39 ` Laurent Vivier
2017-02-09 14:47 ` [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Dr. David Alan Gilbert
` (2 subsequent siblings)
3 siblings, 2 replies; 7+ messages in thread
From: Gerd Hoffmann @ 2017-02-09 13:02 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Wolfgang Bumiller, Dr. David Alan Gilbert
This reverts commit 5858dd1801883309bdd208d72ddb81c4e9fee30c.
Conflicts:
hw/display/cirrus_vga.c
Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/display/cirrus_vga.c | 26 ++++++++------------------
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 6bd13fc..0e47cf8 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -272,6 +272,9 @@ 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) {
+ return true;
+ }
if (pitch < 0) {
int64_t min = addr
+ ((int64_t)s->cirrus_blt_height - 1) * pitch
@@ -290,11 +293,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
return false;
}
-static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
- bool zero_src_pitch_ok)
+static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
{
- int32_t check_pitch;
-
/* should be the case, see cirrus_bitblt_start */
assert(s->cirrus_blt_width > 0);
assert(s->cirrus_blt_height > 0);
@@ -303,10 +303,6 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
return true;
}
- if (!s->cirrus_blt_dstpitch) {
- return true;
- }
-
if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
s->cirrus_blt_dstaddr)) {
return true;
@@ -314,13 +310,7 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
if (dst_only) {
return false;
}
-
- check_pitch = s->cirrus_blt_srcpitch;
- if (!zero_src_pitch_ok && !check_pitch) {
- check_pitch = s->cirrus_blt_width;
- }
-
- if (blit_region_is_unsafe(s, check_pitch,
+ if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
s->cirrus_blt_srcaddr)) {
return true;
}
@@ -715,7 +705,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
src = s->cirrus_bltbuf;
}
- if (blit_is_unsafe(s, true, true)) {
+ if (blit_is_unsafe(s, true)) {
return 0;
}
@@ -734,7 +724,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
{
cirrus_fill_t rop_func;
- if (blit_is_unsafe(s, true, true)) {
+ if (blit_is_unsafe(s, true)) {
return 0;
}
rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
@@ -834,7 +824,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
{
- if (blit_is_unsafe(s, false, false))
+ if (blit_is_unsafe(s, false))
return 0;
return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops"
2017-02-09 13:02 ` [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops" Gerd Hoffmann
@ 2017-02-09 14:52 ` Dr. David Alan Gilbert
2017-02-10 11:39 ` Laurent Vivier
1 sibling, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2017-02-09 14:52 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Wolfgang Bumiller
* Gerd Hoffmann (kraxel@redhat.com) wrote:
> This reverts commit 5858dd1801883309bdd208d72ddb81c4e9fee30c.
>
> Conflicts:
> hw/display/cirrus_vga.c
>
> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/display/cirrus_vga.c | 26 ++++++++------------------
> 1 file changed, 8 insertions(+), 18 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 6bd13fc..0e47cf8 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -272,6 +272,9 @@ 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) {
> + return true;
> + }
> if (pitch < 0) {
> int64_t min = addr
> + ((int64_t)s->cirrus_blt_height - 1) * pitch
> @@ -290,11 +293,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> return false;
> }
>
> -static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> - bool zero_src_pitch_ok)
> +static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
> {
> - int32_t check_pitch;
> -
> /* should be the case, see cirrus_bitblt_start */
> assert(s->cirrus_blt_width > 0);
> assert(s->cirrus_blt_height > 0);
> @@ -303,10 +303,6 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> return true;
> }
>
> - if (!s->cirrus_blt_dstpitch) {
> - return true;
> - }
> -
> if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
> s->cirrus_blt_dstaddr)) {
> return true;
> @@ -314,13 +310,7 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> if (dst_only) {
> return false;
> }
> -
> - check_pitch = s->cirrus_blt_srcpitch;
> - if (!zero_src_pitch_ok && !check_pitch) {
> - check_pitch = s->cirrus_blt_width;
> - }
> -
> - if (blit_region_is_unsafe(s, check_pitch,
> + if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
> s->cirrus_blt_srcaddr)) {
> return true;
> }
> @@ -715,7 +705,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
> src = s->cirrus_bltbuf;
> }
>
> - if (blit_is_unsafe(s, true, true)) {
> + if (blit_is_unsafe(s, true)) {
> return 0;
> }
>
> @@ -734,7 +724,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
> {
> cirrus_fill_t rop_func;
>
> - if (blit_is_unsafe(s, true, true)) {
> + if (blit_is_unsafe(s, true)) {
> return 0;
> }
> rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
> @@ -834,7 +824,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
>
> static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
> {
> - if (blit_is_unsafe(s, false, false))
> + if (blit_is_unsafe(s, false))
> return 0;
>
> return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
> --
> 1.8.3.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops"
2017-02-09 13:02 ` [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops" Gerd Hoffmann
2017-02-09 14:52 ` Dr. David Alan Gilbert
@ 2017-02-10 11:39 ` Laurent Vivier
1 sibling, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2017-02-10 11:39 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: Wolfgang Bumiller, Dr. David Alan Gilbert
On 09/02/2017 14:02, Gerd Hoffmann wrote:
> This reverts commit 5858dd1801883309bdd208d72ddb81c4e9fee30c.
>
> Conflicts:
> hw/display/cirrus_vga.c
>
> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/display/cirrus_vga.c | 26 ++++++++------------------
> 1 file changed, 8 insertions(+), 18 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 6bd13fc..0e47cf8 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -272,6 +272,9 @@ 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) {
> + return true;
> + }
> if (pitch < 0) {
> int64_t min = addr
> + ((int64_t)s->cirrus_blt_height - 1) * pitch
> @@ -290,11 +293,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> return false;
> }
>
> -static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> - bool zero_src_pitch_ok)
> +static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
> {
> - int32_t check_pitch;
> -
> /* should be the case, see cirrus_bitblt_start */
> assert(s->cirrus_blt_width > 0);
> assert(s->cirrus_blt_height > 0);
> @@ -303,10 +303,6 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> return true;
> }
>
> - if (!s->cirrus_blt_dstpitch) {
> - return true;
> - }
> -
> if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
> s->cirrus_blt_dstaddr)) {
> return true;
> @@ -314,13 +310,7 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only,
> if (dst_only) {
> return false;
> }
> -
> - check_pitch = s->cirrus_blt_srcpitch;
> - if (!zero_src_pitch_ok && !check_pitch) {
> - check_pitch = s->cirrus_blt_width;
> - }
> -
> - if (blit_region_is_unsafe(s, check_pitch,
> + if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
> s->cirrus_blt_srcaddr)) {
> return true;
> }
> @@ -715,7 +705,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
> src = s->cirrus_bltbuf;
> }
>
> - if (blit_is_unsafe(s, true, true)) {
> + if (blit_is_unsafe(s, true)) {
> return 0;
> }
>
> @@ -734,7 +724,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
> {
> cirrus_fill_t rop_func;
>
> - if (blit_is_unsafe(s, true, true)) {
> + if (blit_is_unsafe(s, true)) {
> return 0;
> }
> rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
> @@ -834,7 +824,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
>
> static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
> {
> - if (blit_is_unsafe(s, false, false))
> + if (blit_is_unsafe(s, false))
> return 0;
>
> return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks
2017-02-09 13:02 [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Gerd Hoffmann
2017-02-09 13:02 ` [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops" Gerd Hoffmann
@ 2017-02-09 14:47 ` Dr. David Alan Gilbert
2017-02-10 8:05 ` Wolfgang Bumiller
2017-02-10 11:39 ` Laurent Vivier
3 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2017-02-09 14:47 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Wolfgang Bumiller
* Gerd Hoffmann (kraxel@redhat.com) wrote:
> The blit_region_is_unsafe checks don't work correctly for the
> patterncopy source. It's a fixed-sized region, which doesn't
> depend on cirrus_blt_{width,height}. So go do the check in
> cirrus_bitblt_common_patterncopy instead, then tell blit_is_unsafe that
> it doesn't need to verify the source. Also handle the case where we
> blit from cirrus_bitbuf correctly.
>
> This patch replaces 5858dd1801883309bdd208d72ddb81c4e9fee30c.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Security impact: I think for the most part error on the safe side this
> time, refusing blits which should have been allowed.
It might be worth adding a trace in at some point for denied blits so when
someone complains that $OS doesn't render properly we can see if stuff
is being dropped.
Dave
> Only exception is placing the blit source at the end of the video ram,
> so cirrus_blt_srcaddr + 256 goes beyond the end of video memory. But
> even in that case I'm not fully sure this actually allows read access to
> host memory. To trick the commit 5858dd18 security checks one has to
> pick very small cirrus_blt_{width,height} values, which in turn implies
> only a fraction of the blit source will actually be used.
>
> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/display/cirrus_vga.c | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 16f27e8..6bd13fc 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -683,14 +683,39 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
> }
> }
>
> -static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
> - const uint8_t * src)
> +static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
> {
> + uint32_t patternsize;
> uint8_t *dst;
> + uint8_t *src;
>
> dst = s->vga.vram_ptr + s->cirrus_blt_dstaddr;
>
> - if (blit_is_unsafe(s, false, true)) {
> + if (videosrc) {
> + switch (s->vga.get_bpp(&s->vga)) {
> + case 8:
> + patternsize = 64;
> + break;
> + case 15:
> + case 16:
> + patternsize = 128;
> + break;
> + case 24:
> + case 32:
> + default:
> + patternsize = 256;
> + break;
> + }
> + s->cirrus_blt_srcaddr &= ~(patternsize - 1);
> + if (s->cirrus_blt_srcaddr + patternsize > s->vga.vram_size) {
> + return 0;
> + }
> + src = s->vga.vram_ptr + s->cirrus_blt_srcaddr;
> + } else {
> + src = s->cirrus_bltbuf;
> + }
> +
> + if (blit_is_unsafe(s, true, true)) {
> return 0;
> }
>
> @@ -731,8 +756,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
>
> static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
> {
> - return cirrus_bitblt_common_patterncopy(s, s->vga.vram_ptr +
> - (s->cirrus_blt_srcaddr & ~7));
> + return cirrus_bitblt_common_patterncopy(s, true);
> }
>
> static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
> @@ -831,7 +855,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
>
> if (s->cirrus_srccounter > 0) {
> if (s->cirrus_blt_mode & CIRRUS_BLTMODE_PATTERNCOPY) {
> - cirrus_bitblt_common_patterncopy(s, s->cirrus_bltbuf);
> + cirrus_bitblt_common_patterncopy(s, false);
> the_end:
> s->cirrus_srccounter = 0;
> cirrus_bitblt_reset(s);
> --
> 1.8.3.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks
2017-02-09 13:02 [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Gerd Hoffmann
2017-02-09 13:02 ` [Qemu-devel] [PATCH 2/2] Revert "cirrus: allow zero source pitch in pattern fill rops" Gerd Hoffmann
2017-02-09 14:47 ` [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Dr. David Alan Gilbert
@ 2017-02-10 8:05 ` Wolfgang Bumiller
2017-02-10 11:39 ` Laurent Vivier
3 siblings, 0 replies; 7+ messages in thread
From: Wolfgang Bumiller @ 2017-02-10 8:05 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel, Dr. David Alan Gilbert
Seems to work.
Took me a while to realize whether the else case was safe, but given the
patternfill functions' access patterns and CIRRUS_BLTBUFSIZE being 8k it
should be fine.
On Thu, Feb 09, 2017 at 02:02:20PM +0100, Gerd Hoffmann wrote:
> The blit_region_is_unsafe checks don't work correctly for the
> patterncopy source. It's a fixed-sized region, which doesn't
> depend on cirrus_blt_{width,height}. So go do the check in
> cirrus_bitblt_common_patterncopy instead, then tell blit_is_unsafe that
> it doesn't need to verify the source. Also handle the case where we
> blit from cirrus_bitbuf correctly.
>
> This patch replaces 5858dd1801883309bdd208d72ddb81c4e9fee30c.
>
> Security impact: I think for the most part error on the safe side this
> time, refusing blits which should have been allowed.
>
> Only exception is placing the blit source at the end of the video ram,
> so cirrus_blt_srcaddr + 256 goes beyond the end of video memory. But
> even in that case I'm not fully sure this actually allows read access to
> host memory. To trick the commit 5858dd18 security checks one has to
> pick very small cirrus_blt_{width,height} values, which in turn implies
> only a fraction of the blit source will actually be used.
>
> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> hw/display/cirrus_vga.c | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 16f27e8..6bd13fc 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -683,14 +683,39 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
> }
> }
>
> -static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
> - const uint8_t * src)
> +static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
> {
> + uint32_t patternsize;
> uint8_t *dst;
> + uint8_t *src;
>
> dst = s->vga.vram_ptr + s->cirrus_blt_dstaddr;
>
> - if (blit_is_unsafe(s, false, true)) {
> + if (videosrc) {
> + switch (s->vga.get_bpp(&s->vga)) {
> + case 8:
> + patternsize = 64;
> + break;
> + case 15:
> + case 16:
> + patternsize = 128;
> + break;
> + case 24:
> + case 32:
> + default:
> + patternsize = 256;
> + break;
> + }
> + s->cirrus_blt_srcaddr &= ~(patternsize - 1);
> + if (s->cirrus_blt_srcaddr + patternsize > s->vga.vram_size) {
> + return 0;
> + }
> + src = s->vga.vram_ptr + s->cirrus_blt_srcaddr;
> + } else {
> + src = s->cirrus_bltbuf;
> + }
> +
> + if (blit_is_unsafe(s, true, true)) {
> return 0;
> }
>
> @@ -731,8 +756,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
>
> static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
> {
> - return cirrus_bitblt_common_patterncopy(s, s->vga.vram_ptr +
> - (s->cirrus_blt_srcaddr & ~7));
> + return cirrus_bitblt_common_patterncopy(s, true);
> }
>
> static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
> @@ -831,7 +855,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
>
> if (s->cirrus_srccounter > 0) {
> if (s->cirrus_blt_mode & CIRRUS_BLTMODE_PATTERNCOPY) {
> - cirrus_bitblt_common_patterncopy(s, s->cirrus_bltbuf);
> + cirrus_bitblt_common_patterncopy(s, false);
> the_end:
> s->cirrus_srccounter = 0;
> cirrus_bitblt_reset(s);
> --
> 1.8.3.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks
2017-02-09 13:02 [Qemu-devel] [PATCH 1/2] cirrus: fix patterncopy checks Gerd Hoffmann
` (2 preceding siblings ...)
2017-02-10 8:05 ` Wolfgang Bumiller
@ 2017-02-10 11:39 ` Laurent Vivier
3 siblings, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2017-02-10 11:39 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: Wolfgang Bumiller, Dr. David Alan Gilbert
On 09/02/2017 14:02, Gerd Hoffmann wrote:
> The blit_region_is_unsafe checks don't work correctly for the
> patterncopy source. It's a fixed-sized region, which doesn't
> depend on cirrus_blt_{width,height}. So go do the check in
> cirrus_bitblt_common_patterncopy instead, then tell blit_is_unsafe that
> it doesn't need to verify the source. Also handle the case where we
> blit from cirrus_bitbuf correctly.
>
> This patch replaces 5858dd1801883309bdd208d72ddb81c4e9fee30c.
>
> Security impact: I think for the most part error on the safe side this
> time, refusing blits which should have been allowed.
>
> Only exception is placing the blit source at the end of the video ram,
> so cirrus_blt_srcaddr + 256 goes beyond the end of video memory. But
> even in that case I'm not fully sure this actually allows read access to
> host memory. To trick the commit 5858dd18 security checks one has to
> pick very small cirrus_blt_{width,height} values, which in turn implies
> only a fraction of the blit source will actually be used.
>
> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---
> hw/display/cirrus_vga.c | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 16f27e8..6bd13fc 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -683,14 +683,39 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
> }
> }
>
> -static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
> - const uint8_t * src)
> +static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
> {
> + uint32_t patternsize;
> uint8_t *dst;
> + uint8_t *src;
>
> dst = s->vga.vram_ptr + s->cirrus_blt_dstaddr;
>
> - if (blit_is_unsafe(s, false, true)) {
> + if (videosrc) {
> + switch (s->vga.get_bpp(&s->vga)) {
> + case 8:
> + patternsize = 64;
> + break;
> + case 15:
> + case 16:
> + patternsize = 128;
> + break;
> + case 24:
> + case 32:
> + default:
> + patternsize = 256;
> + break;
> + }
> + s->cirrus_blt_srcaddr &= ~(patternsize - 1);
> + if (s->cirrus_blt_srcaddr + patternsize > s->vga.vram_size) {
> + return 0;
> + }
> + src = s->vga.vram_ptr + s->cirrus_blt_srcaddr;
> + } else {
> + src = s->cirrus_bltbuf;
> + }
> +
> + if (blit_is_unsafe(s, true, true)) {
> return 0;
> }
>
> @@ -731,8 +756,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
>
> static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
> {
> - return cirrus_bitblt_common_patterncopy(s, s->vga.vram_ptr +
> - (s->cirrus_blt_srcaddr & ~7));
> + return cirrus_bitblt_common_patterncopy(s, true);
> }
>
> static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
> @@ -831,7 +855,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
>
> if (s->cirrus_srccounter > 0) {
> if (s->cirrus_blt_mode & CIRRUS_BLTMODE_PATTERNCOPY) {
> - cirrus_bitblt_common_patterncopy(s, s->cirrus_bltbuf);
> + cirrus_bitblt_common_patterncopy(s, false);
> the_end:
> s->cirrus_srccounter = 0;
> cirrus_bitblt_reset(s);
>
^ permalink raw reply [flat|nested] 7+ messages in thread