diff mbox

[1/2] cirrus: fix patterncopy checks

Message ID 1486645341-5010-1-git-send-email-kraxel@redhat.com
State New, archived
Headers show

Commit Message

Gerd Hoffmann Feb. 9, 2017, 1:02 p.m. UTC
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>
---
 hw/display/cirrus_vga.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Dr. David Alan Gilbert Feb. 9, 2017, 2:47 p.m. UTC | #1
* 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
Wolfgang Bumiller Feb. 10, 2017, 8:05 a.m. UTC | #2
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
> 
>
Laurent Vivier Feb. 10, 2017, 11:39 a.m. UTC | #3
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);
>
diff mbox

Patch

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);