Re: [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)

[Markus Armbruster <armbru@redhat.com>]

"Pat Campbell" <plc@novell.com> writes:

>>>> On Thu, Jan 10, 2008 at  3:18 AM, in message
> <87ejcqhtku.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com>
> wrote: 
>> "Pat Campbell" <plc@novell.com> writes:
>> 
>>> Attached patch adds multiple frame buffer size support to the xenfb PV 
>>> backend QEMU xenfb.  Backend sets feature-    resize and handles the
>>> resize frame buffer event.
>>>
>>> Corresponding frontend LINUX patch is required for functionality but this
>>> patch is not dependent on it, preserving backwards compatibility.
[...]
>> By the way, the feature negotiation only covers whether the frontend
>> is permitted to resize, not acceptable resolutions.
>
> True.  Acceptable resolutions are what fits within a 5MB framebuffer
> and has a width no greater than 1280.

Is this enough for all eternity?  Screen resolutions continue to
grow...  If it's not, then how can we ensure that we can enlarge it
easily while maintaining backwards compatibility?

Perhaps the backend could publish the maximum resolution it can
accept, instead of just a flag that it can accept resize events.

>> What if the frontend resizes to a resolution the backend can't accept?
>> The backend has no way to tell the frontend not to do that.  Would we
>> end up with a defunct display and no way for the user to fix it?
>
> I don't think this is a possible issue. Frontend code limits the size of
> the frame buffer to 5MB with a max horizontal scanline length of
> 1280.  The backend VNC server should be able to handle that without
> any problems.
>
>> 
>> What happens when a malicious frontend resizes to a resolution that
>> makes pd[] extend beyond the end of the shared page?  Nothing new
>> really, the current backend has the same problem with the initial
>> resolution, I think.
>
> Can't do that either.  Maximum frame buffer size of 5MB fits within
> the 3 page descriptors.    

What stops the frontend from putting a bogus framebuffer description
in the shared page?  If that can make the backend overrun the shared
page, we have a security problem.

I think the backend should verify that the framebuffer dimension
against he limits you quoted.

>>>  	fprintf(stderr, "FB: Waiting for KBD backend creation\n");
>>>  	xenfb_wait_for_backend(&xenfb-    >kbd, xenfb_backend_created_kbd);
>>> @@ -    510,6 +525,11 @@ static void xenfb_on_fb_event(struct xen
>>>  			}
>>>  			xenfb_guest_copy(xenfb, x, y, w, h);
>>>  			break;
>>> +		case XENFB_TYPE_RESIZE:
>>> +			xenfb-    >width  = event-    >resize.width;
>>> +			xenfb-    >height = event-    >resize.height;
>>> +			dpy_resize(xenfb-    >ds, xenfb-    >width, xenfb-    >height);
>>> +			break;
>>>  		}
>>>  	}
>>>  	mb();			/* ensure we're done with ring contents */
>>> @@ -    605,11 +625,22 @@ static int xenfb_send_motion(struct xenf
>>>  	return xenfb_kbd_event(xenfb, &event);
>>>  }
>>>  
>>> +/* Descale abs pos for older evdev_drv without AbsoluteScreen option */
>>> +static inline int xenfb_descale_for_evdev(float fb_width, float 
>> screen_width, float pos)
>> 
>> This is used both for width and height, despite the parameter names.
>> Suggest something like limit and max_limit.
>> 
>>> +{
>>> +	return(((fb_width/screen_width) * pos) + 0.5);
>> 
>> Style nitpick:
>> 
>> 	return (fb_width/screen_width) * pos + 0.5;
>> 
>>> +}
>>> +
>
> I have removed the whole scale code.  I have arbitarily decided that if
> a VM wishes to use dynamic modes and absolute mouse positioning it 
> should have an updated X evdev driver like what is in Fedora8 or
> OpenSuSE 10.3
>
> virt-manager with mouse grab works as good as it alway has.

Good move!

[...]