On 01/25/17 11:50, Wolfgang Bumiller wrote:
> On Wed, Jan 25, 2017 at 11:35:44AM +0100, Laszlo Ersek wrote:
>> On 01/25/17 10:50, Gerd Hoffmann wrote:
>>> On Mi, 2017-01-25 at 09:30 +0100, Wolfgang Bumiller wrote:
>>>> On Wed, Jan 25, 2017 at 08:07:05AM +0100, Gerd Hoffmann wrote:
>>>>> From: Li Qiang <liqiang...@360.cn>
>>>>>
>>>>> When doing bitblt copy in backward mode, we should minus the
>>>>> blt width first just like the adding in the forward mode. This
>>>>> can avoid the oob access of the front of vga's vram.
>>>>>
>>>>> Signed-off-by: Li Qiang <liqiang...@360.cn>
>>>>> Message-id: 5887254f.863a240a.2c122.5...@mx.google.com
>>>>>
>>>>> { kraxel: with backward blits (negative pitch) addr is the topmost
>>>>> address, so check it as-is against vram size ]
>>>>>
>>>>> Cc: qemu-sta...@nongnu.org
>>>>> Cc: P J P <ppan...@redhat.com>
>>>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>>>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>>>>> Cc: Wolfgang Bumiller <w.bumil...@proxmox.com>
>>>>> Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106)
>>>>> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
>>>>> ---
>>>>> hw/display/cirrus_vga.c | 7 +++----
>>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>>>> index 379910d..b8c29a6 100644
>>>>> --- a/hw/display/cirrus_vga.c
>>>>> +++ b/hw/display/cirrus_vga.c
>>>>> @@ -277,10 +277,9 @@ static bool blit_region_is_unsafe(struct
>>>>> CirrusVGAState *s,
>>>>> }
>>>>> 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) {
>>>>> + + ((int64_t)s->cirrus_blt_height - 1) * pitch
>>>>> + - s->cirrus_blt_width;
>>>>> + if (min < 0 || addr > s->vga.vram_size) {
>>>>
>>>> Call me paranoid, but shouldn't this be '>='? Missed this yesterday
>>>> apparently, correct me if I'm wrong:
>>>> If VRAM goes from 0..7 it has a size of 8, and this would accept
>>>> address 8 as it's not > size.
>>>
>>> I think you are right. The bkwd ops first execute the op, then
>>> decrement, so addr is inclusive and the check is off by one.
>>
>> That's right IMO; however, in that case we also have to posit that "min"
>> is exclusive. Assume that we have 16 pixels in the VGA memory (4x4), and
>> that we are massaging the bottom right quadrant:
>>
>> 0 1 2 3
>> 4 5 6 7
>> 8 9 10 11
>> 12 13 14 15
>>
>> addr = 15
>> height = 2
>> width = 2
>> pitch = -4
>>
>> Then
>>
>> min = addr + (height - 1) * pitch - width
>> = 15 + ( 2 - 1) * (-4) - 2
>> = 9
>>
>> Which is the address right before the top left pixel; that is, it marks
>> the first pixel *not* accessed.
>>
>> If that value was (-1), then the operation would still be valid.
>>
>> So we should accept (min == -1) -- this is dictated by plain symmetry.
>> If "max" -- here, "addr" -- is inclusive, then "min" becomes exclusive.
>
> You're right.
>
> You'd think it wouldn't take so many different people to notice these
> things :(. It was right there, I should have noticed it.
>
I could tell you the same about "addr" pointing to bottom-left vs.
bottom-right, in the original patch :(
