[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_un
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_unsafe |
Date: |
Tue, 9 Feb 2016 20:08:56 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
On 02/09/16 11:59, Paolo Bonzini wrote:
> The "max" value is being compared with >=, but addr + width points to
> the first byte that will _not_ be copied. Subtract one like it is
> already done above for the height.
>
> Cc: Gerd Hoffmann <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> hw/display/cirrus_vga.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index b6ce1c8..e7939d2 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -275,14 +275,14 @@ static bool blit_region_is_unsafe(struct CirrusVGAState
> *s,
> int64_t min = addr
> + ((int64_t)s->cirrus_blt_height-1) * pitch;
> int32_t max = addr
> - + s->cirrus_blt_width;
> + + s->cirrus_blt_width-1;
> 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;
> + + s->cirrus_blt_width-1;
> if (max >= s->vga.vram_size) {
> return true;
> }
>
(a) I reported this issue in an internal discussion @RH, more than a
year ago. Please refer to Message-Id: <address@hidden>,
points (2) and (5).
(b) I think the commit message should be clearer about the fact that
this is not a security problem -- the off-by-one errs on the side of
safety (i.e., the behavior is too strict, not too lax).
(c) The patch is mathematically correct, but I'd feel safer if, rather
than decrementing max, it would replace the
max >= s->vga.vram_size
comparisons with
max > s->vga.vram_size
IIRC I spent hours reviewing the backport of d3532a0db022 (for
CVE-2014-8106). Comparing exclusive with exclusive (rather than turning
"max" into inclusive) was my suggestion back then. I'm not saying the
way it is written above is incorrect, just that I can't make the effort
this time to see if it is correct. With the relop replacement (and the
commit message update), you could get my R-b at once! :)
Thanks
Laszlo