Patchwork vmware_vga: Redraw only visible area

login
register
mail settings
Submitter Marek Vasut
Date Aug. 17, 2012, 2:55 a.m.
Message ID <1345172107-27092-1-git-send-email-marex@denx.de>
Download mbox | patch
Permalink /patch/178137/
State New
Headers show

Comments

Marek Vasut - Aug. 17, 2012, 2:55 a.m.
Disallow negative value boundaries of the redraw rectangle.
This fixes a segfault when using -vga vmware.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 hw/vmware_vga.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

NOTE: I tested this by emulating some recent version of ubuntu. The rect->x
      value was set to -65 for some reason at one point, which caused the
      kvm to crash. Trimming the rectangle fixed the issue.
Michael Tokarev - Aug. 17, 2012, 12:18 p.m.
On 17.08.2012 06:55, Marek Vasut wrote:
> Disallow negative value boundaries of the redraw rectangle.
> This fixes a segfault when using -vga vmware.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  hw/vmware_vga.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> NOTE: I tested this by emulating some recent version of ubuntu. The rect->x
>       value was set to -65 for some reason at one point, which caused the
>       kvm to crash. Trimming the rectangle fixed the issue.
> 
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index f5e4f44..62e5887 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -337,8 +337,8 @@ static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
>  {
>      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last ++];
>      s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> -    rect->x = x;
> -    rect->y = y;
> +    rect->x = (x < 0) ? 0 : x;
> +    rect->y = (y < 0) ? 0 : y;
>      rect->w = w;
>      rect->h = h;
>  }


Is it the same as https://bugs.launchpad.net/bugs/918791 ?
At least it appears to be the same theme...  But there,
the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
also updates width/height.  My comment:
https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/21

"So indeed, some (upstream) verification is needed here -- where these
negative values are coming from, whenever it is EVER okay to have them,
what to do with these, and where to check (I guess the check should be
done somewhere in the upper layer)."

Especially the last part about the layer.

Thanks,

/mjt
Marek Vasut - Aug. 17, 2012, 12:37 p.m.
Dear Michael Tokarev,

> On 17.08.2012 06:55, Marek Vasut wrote:
> > Disallow negative value boundaries of the redraw rectangle.
> > This fixes a segfault when using -vga vmware.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > ---
> > 
> >  hw/vmware_vga.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > NOTE: I tested this by emulating some recent version of ubuntu. The
> > rect->x
> > 
> >       value was set to -65 for some reason at one point, which caused the
> >       kvm to crash. Trimming the rectangle fixed the issue.
> > 
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index f5e4f44..62e5887 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -337,8 +337,8 @@ static inline void vmsvga_update_rect_delayed(struct
> > vmsvga_state_s *s,
> > 
> >  {
> >  
> >      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last
> >      ++]; s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> > 
> > -    rect->x = x;
> > -    rect->y = y;
> > +    rect->x = (x < 0) ? 0 : x;
> > +    rect->y = (y < 0) ? 0 : y;
> > 
> >      rect->w = w;
> >      rect->h = h;
> >  
> >  }
> 
> Is it the same as https://bugs.launchpad.net/bugs/918791 ?
> At least it appears to be the same theme...  But there,
> the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
> also updates width/height.  My comment:
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/21

Looks to be the same ... though my patch tries to squash the issue as early as 
possible.

You're right that x and y might overflow to the other side too. Also, you're 
right about w and h.

Shall I send updated patch?

> "So indeed, some (upstream) verification is needed here -- where these
> negative values are coming from, whenever it is EVER okay to have them,
> what to do with these, and where to check (I guess the check should be
> done somewhere in the upper layer)."
> 
> Especially the last part about the layer.

Where's the upper layer though, isn't that what's pouring out of the virtual 
machine itself?

> Thanks,

Thank you for guidance !

> /mjt

Best regards,
Marek Vasut
Marek Vasut - Sept. 16, 2012, 8:36 p.m.
> Dear Michael Tokarev,

Bump? Did this lead anywhere? Do you need updated patch?

> > On 17.08.2012 06:55, Marek Vasut wrote:
> > > Disallow negative value boundaries of the redraw rectangle.
> > > This fixes a segfault when using -vga vmware.
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > ---
> > > 
> > >  hw/vmware_vga.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > NOTE: I tested this by emulating some recent version of ubuntu. The
> > > rect->x
> > > 
> > >       value was set to -65 for some reason at one point, which caused
> > >       the kvm to crash. Trimming the rectangle fixed the issue.
> > > 
> > > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > > index f5e4f44..62e5887 100644
> > > --- a/hw/vmware_vga.c
> > > +++ b/hw/vmware_vga.c
> > > @@ -337,8 +337,8 @@ static inline void
> > > vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
> > > 
> > >  {
> > >  
> > >      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last
> > >      ++]; s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> > > 
> > > -    rect->x = x;
> > > -    rect->y = y;
> > > +    rect->x = (x < 0) ? 0 : x;
> > > +    rect->y = (y < 0) ? 0 : y;
> > > 
> > >      rect->w = w;
> > >      rect->h = h;
> > >  
> > >  }
> > 
> > Is it the same as https://bugs.launchpad.net/bugs/918791 ?
> > At least it appears to be the same theme...  But there,
> > the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
> > also updates width/height.  My comment:
> > https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/2
> > 1
> 
> Looks to be the same ... though my patch tries to squash the issue as early
> as possible.
> 
> You're right that x and y might overflow to the other side too. Also,
> you're right about w and h.
> 
> Shall I send updated patch?
> 
> > "So indeed, some (upstream) verification is needed here -- where these
> > negative values are coming from, whenever it is EVER okay to have them,
> > what to do with these, and where to check (I guess the check should be
> > done somewhere in the upper layer)."
> > 
> > Especially the last part about the layer.
> 
> Where's the upper layer though, isn't that what's pouring out of the
> virtual machine itself?
> 
> > Thanks,
> 
> Thank you for guidance !
> 
> > /mjt
> 
> Best regards,
> Marek Vasut

Best regards,
Marek Vasut
Michael Tokarev - Sept. 17, 2012, 4:52 a.m.
On 17.09.2012 00:36, Marek Vasut wrote:
>> Dear Michael Tokarev,
> 
> Bump? Did this lead anywhere? Do you need updated patch?


This didn't lead to anywhere.  I just pointed out that we
have similar issue and somewhat similar change, and it
weren't sufficient.  I don't know this code, and have no
idea who does, either.

Thanks,

/mjt

>>> On 17.08.2012 06:55, Marek Vasut wrote:
>>>> Disallow negative value boundaries of the redraw rectangle.
>>>> This fixes a segfault when using -vga vmware.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>>
>>>>  hw/vmware_vga.c |    4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> NOTE: I tested this by emulating some recent version of ubuntu. The
>>>> rect->x
>>>>
>>>>       value was set to -65 for some reason at one point, which caused
>>>>       the kvm to crash. Trimming the rectangle fixed the issue.
>>>>
>>>> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
>>>> index f5e4f44..62e5887 100644
>>>> --- a/hw/vmware_vga.c
>>>> +++ b/hw/vmware_vga.c
>>>> @@ -337,8 +337,8 @@ static inline void
>>>> vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
>>>>
>>>>  {
>>>>  
>>>>      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last
>>>>      ++]; s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
>>>>
>>>> -    rect->x = x;
>>>> -    rect->y = y;
>>>> +    rect->x = (x < 0) ? 0 : x;
>>>> +    rect->y = (y < 0) ? 0 : y;
>>>>
>>>>      rect->w = w;
>>>>      rect->h = h;
>>>>  
>>>>  }
>>>
>>> Is it the same as https://bugs.launchpad.net/bugs/918791 ?
>>> At least it appears to be the same theme...  But there,
>>> the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
>>> also updates width/height.  My comment:
>>> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments/2
>>> 1
>>
>> Looks to be the same ... though my patch tries to squash the issue as early
>> as possible.
>>
>> You're right that x and y might overflow to the other side too. Also,
>> you're right about w and h.
>>
>> Shall I send updated patch?
>>
>>> "So indeed, some (upstream) verification is needed here -- where these
>>> negative values are coming from, whenever it is EVER okay to have them,
>>> what to do with these, and where to check (I guess the check should be
>>> done somewhere in the upper layer)."
>>>
>>> Especially the last part about the layer.
>>
>> Where's the upper layer though, isn't that what's pouring out of the
>> virtual machine itself?
>>
>>> Thanks,
>>
>> Thank you for guidance !
>>
>>> /mjt
>>
>> Best regards,
>> Marek Vasut
> 
> Best regards,
> Marek Vasut
Marek Vasut - Sept. 17, 2012, 10:15 a.m.
Dear Michael Tokarev,

> On 17.09.2012 00:36, Marek Vasut wrote:
> >> Dear Michael Tokarev,
> > 
> > Bump? Did this lead anywhere? Do you need updated patch?
> 
> This didn't lead to anywhere.  I just pointed out that we
> have similar issue and somewhat similar change, and it
> weren't sufficient.  I don't know this code, and have no
> idea who does, either.

So what do we do? I'll try to investigate a bit more, all right?

> Thanks,
> 
> /mjt
> 
> >>> On 17.08.2012 06:55, Marek Vasut wrote:
> >>>> Disallow negative value boundaries of the redraw rectangle.
> >>>> This fixes a segfault when using -vga vmware.
> >>>> 
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> ---
> >>>> 
> >>>>  hw/vmware_vga.c |    4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>> 
> >>>> NOTE: I tested this by emulating some recent version of ubuntu. The
> >>>> rect->x
> >>>> 
> >>>>       value was set to -65 for some reason at one point, which caused
> >>>>       the kvm to crash. Trimming the rectangle fixed the issue.
> >>>> 
> >>>> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> >>>> index f5e4f44..62e5887 100644
> >>>> --- a/hw/vmware_vga.c
> >>>> +++ b/hw/vmware_vga.c
> >>>> @@ -337,8 +337,8 @@ static inline void
> >>>> vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
> >>>> 
> >>>>  {
> >>>>  
> >>>>      struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last
> >>>>      ++]; s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
> >>>> 
> >>>> -    rect->x = x;
> >>>> -    rect->y = y;
> >>>> +    rect->x = (x < 0) ? 0 : x;
> >>>> +    rect->y = (y < 0) ? 0 : y;
> >>>> 
> >>>>      rect->w = w;
> >>>>      rect->h = h;
> >>>>  
> >>>>  }
> >>> 
> >>> Is it the same as https://bugs.launchpad.net/bugs/918791 ?
> >>> At least it appears to be the same theme...  But there,
> >>> the patch (https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff)
> >>> also updates width/height.  My comment:
> >>> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791/comments
> >>> /2 1
> >> 
> >> Looks to be the same ... though my patch tries to squash the issue as
> >> early as possible.
> >> 
> >> You're right that x and y might overflow to the other side too. Also,
> >> you're right about w and h.
> >> 
> >> Shall I send updated patch?
> >> 
> >>> "So indeed, some (upstream) verification is needed here -- where these
> >>> negative values are coming from, whenever it is EVER okay to have them,
> >>> what to do with these, and where to check (I guess the check should be
> >>> done somewhere in the upper layer)."
> >>> 
> >>> Especially the last part about the layer.
> >> 
> >> Where's the upper layer though, isn't that what's pouring out of the
> >> virtual machine itself?
> >> 
> >>> Thanks,
> >> 
> >> Thank you for guidance !
> >> 
> >>> /mjt
> >> 
> >> Best regards,
> >> Marek Vasut
> > 
> > Best regards,
> > Marek Vasut

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index f5e4f44..62e5887 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -337,8 +337,8 @@  static inline void vmsvga_update_rect_delayed(struct vmsvga_state_s *s,
 {
     struct vmsvga_rect_s *rect = &s->redraw_fifo[s->redraw_fifo_last ++];
     s->redraw_fifo_last &= REDRAW_FIFO_LEN - 1;
-    rect->x = x;
-    rect->y = y;
+    rect->x = (x < 0) ? 0 : x;
+    rect->y = (y < 0) ? 0 : y;
     rect->w = w;
     rect->h = h;
 }