| Message ID | 1455015557-15106-3-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <kraxel@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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: <54B7A2D7.5010404@redhat.com>, 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
On 09/02/2016 20:08, Laszlo Ersek wrote: > 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 <kraxel@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> 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: <54B7A2D7.5010404@redhat.com>, > 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 Hmm, not sure why. We're comparing against the inclusive-exclusive range [0,s->vga.vram_size). The right way to check if something is within the range is >= min && < max; the right way to check if something is outside the range is < min || >= max. Paolo
On 02/10/16 13:32, Paolo Bonzini wrote: > > > On 09/02/2016 20:08, Laszlo Ersek wrote: >> 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 <kraxel@redhat.com> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> 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: <54B7A2D7.5010404@redhat.com>, >> 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 > > Hmm, not sure why. We're comparing against the inclusive-exclusive > range [0,s->vga.vram_size). The right way to check if something is > within the range is >= min && < max; the right way to check if something > is outside the range is < min || >= max. Absolutely: if the thing you are verifying against the interval is itself an inclusive thing, that is, a pixel or byte *drawn*. However, exactly as you stated in the commit message, for the maximum check, what we are comparing is the first offset *not* processed. As I said, my suggestion doesn't change the meaning of your patch at all, it only reformulates it. Consider, pre-patch we have the following condition for rejection (max check only): T := addr + s->cirrus_blt_width reject if (T >= s->vga.vram_size) [0] This is overprotective, because (T == s->vga.vram_size) should be accepted. (Because, as you wrote, both T and s->vga.vram_size are exclusive.) Therefore the right condition is: reject if (T > s->vga.vram_size) [1] This is equivalent to: reject if (T >= s->vga.vram_size + 1) [2] Which is further equivalent to reject if (T - 1 >= s->vga.vram_size) [3] Your patch encodes [3], by setting the "max" variable to (T-1), and keeping the >= relop. My suggestion is to preserve "max" set to T, and encode [1]: replace the >= relop with >. Mathematically they are equivalent, but in C, I feel [1] is safer (without putting in the work to see that [3] is safe as well.) Thanks Laszlo
On 10/02/2016 15:55, Laszlo Ersek wrote: >> > Hmm, not sure why. We're comparing against the inclusive-exclusive >> > range [0,s->vga.vram_size). The right way to check if something is >> > within the range is >= min && < max; the right way to check if something >> > is outside the range is < min || >= max. > Absolutely: if the thing you are verifying against the interval is > itself an inclusive thing, that is, a pixel or byte *drawn*. However, > exactly as you stated in the commit message, for the maximum check, what > we are comparing is the first offset *not* processed. Right, what my patch does is setting min and max both to pixels that are drawn. Paolo
On 02/10/16 16:29, Paolo Bonzini wrote: > > > On 10/02/2016 15:55, Laszlo Ersek wrote: >>>> Hmm, not sure why. We're comparing against the inclusive-exclusive >>>> range [0,s->vga.vram_size). The right way to check if something is >>>> within the range is >= min && < max; the right way to check if something >>>> is outside the range is < min || >= max. >> Absolutely: if the thing you are verifying against the interval is >> itself an inclusive thing, that is, a pixel or byte *drawn*. However, >> exactly as you stated in the commit message, for the maximum check, what >> we are comparing is the first offset *not* processed. > > Right, what my patch does is setting min and max both to pixels that are > drawn. Do you understand my concern with that? It's okay if you dismiss my concern (or even better if you prove it is unfounded). But I hope you at least understand it. When you set "max" to the last pixel that is drawn, you are calculating a new quantity in C that was not calculated before. By subtracting 1, you could theoretically turn "max" into a negative number. Have you checked and excluded this possibility, or proved why it doesn't matter? When I reviewed the underlying CVE fix (downstream), I spent hours on tracking down all possibilities, with Gerd's help. With your patch, that argument goes out the window, *for me*. I don't mind -- in particular because it *could* be easy to prove your patch is safe --, but I can't tell if it's going to be an easy proof without actually trying. And I'm not going to try, now. Changing the relop would be mathematically equivalent, and keep my earlier argument intact. Anyway, you don't need my personal R-b for this. Thanks Laszlo
On 10/02/2016 16:54, Laszlo Ersek wrote: > On 02/10/16 16:29, Paolo Bonzini wrote: >> >> >> On 10/02/2016 15:55, Laszlo Ersek wrote: >>>>> Hmm, not sure why. We're comparing against the inclusive-exclusive >>>>> range [0,s->vga.vram_size). The right way to check if something is >>>>> within the range is >= min && < max; the right way to check if something >>>>> is outside the range is < min || >= max. >>> Absolutely: if the thing you are verifying against the interval is >>> itself an inclusive thing, that is, a pixel or byte *drawn*. However, >>> exactly as you stated in the commit message, for the maximum check, what >>> we are comparing is the first offset *not* processed. >> >> Right, what my patch does is setting min and max both to pixels that are >> drawn. > > Do you understand my concern with that? It's okay if you dismiss my > concern (or even better if you prove it is unfounded). But I hope you at > least understand it. No, I didn't. Now I do, and you're right. > When you set "max" to the last pixel that is drawn, you are calculating > a new quantity in C that was not calculated before. By subtracting 1, > you could theoretically turn "max" into a negative number. Gotcha. > Have you checked and excluded this possibility, or proved why it doesn't > matter? It doesn't matter because width == 0 is handled properly later on; it does not do anything, including not loading and not storing anything. So the check is pointless anyway in that case. But as I said: you're right and I will proceed to send v2. Your reason for preferring a > check makes sense. An alternative possibility is to make max uint64_t, and ensure that all the addends are properly sign extended. I mention it just for the sake of completeness. :) > When I reviewed the underlying CVE fix (downstream), I spent hours on > tracking down all possibilities, with Gerd's help. With your patch, that > argument goes out the window, *for me*. I don't mind -- in particular > because it *could* be easy to prove your patch is safe --, but I can't > tell if it's going to be an easy proof without actually trying. And I'm > not going to try, now. > > Changing the relop would be mathematically equivalent, and keep my > earlier argument intact. > > Anyway, you don't need my personal R-b for this. I was interested in your reasoning, I just couldn't get it. Paolo
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; }
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 <kraxel@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/display/cirrus_vga.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)