Re: [Xen-devel] [PATCH] Paravirt framebuffer backend tools [2/5]

[Steven Smith <sos22-xen@xxxxxxxxxxxxx>]

> > -- The setup protocol doesn't look much like the normal xenbus state
> >    machine.  There may be a good reason for this, but I haven't heard
> >    one yet.  I know the standard way is badly documented and
> >    non-trivial to understand from the existing implementations; sorry
> >    about that.
> Anthony wrote it that way.  I don't know why, but it's simple, and it
> works.
> 
> I'm most reluctant to throw out working code while there's so much
> code that needs fixing.  That situation is improving, though.
Okay.

> >> +  for (;;) {
> >> +          FD_ZERO(&readfds);
> >> +          FD_SET(fd, &readfds);
> >> +          tv = (struct timeval){0, 10000};
> >> +
> >> +          if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
> >> +                  if (errno == EINTR)
> >> +                          continue;
> >> +                  fprintf(stderr, "Connection to domain broke (%s)\n",
> >> +                          strerror(errno));
> > It's not really the connection to the domain that's broken as the
> > event channel device.  Not terribly important.
> Happy to reword it; suggestions?
I dunno; maybe something like ``select() failed on event channel
device (%s)\n"?  I'd rather have a message which most users would find
meaningless than one which is actually misleading.  Given that this
should almost never happen I don't think it's worth worrying about
that much.

> >                  What happens if someone has a four, five, six button
> > mouse?
> The extra buttons get mapped to negative values here (ugh), truncated
> to __u8 in xenfb_send_button() (double-ugh) and thrown away in the
> frontend.  The 256th button would get misinterpreted as the first one,
> though :)
I'd prefer to discard invalid events here in the backend if possible,
just on the general principle that sending garbage across the ring
buffer is a bad idea even if you know it's going to get discarded.

> The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:
> 
>       __u8 button;       /* the button (0, 1, 2 is right, middle, left) */
> 
> This is extensible as long as we handle unknown buttons gracefully.  I
> do hate the weird encoding of buttons, though.  Too hebrew for my
> taste.
It's certainly not how I would have done it, but I don't think it
makes much difference.

> SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
> SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).
> 
> LibVNCServer provides for 8 buttons.
> 
> The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
> BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
> BTN_TASK).
> 
> How to best map buttons from VNC and SDL to input beyond the first
> three?  If we can find a workable answer for that, providing for more
> buttons should be trivial.
*shrug* We might as well just send input layer codes across the ring
buffer and do the translation in the backend.  That makes the Linux
frontend easier and doesn't make other operating systems any harder.
If we later find that some other operating system supports buttons
which the input layer doesn't then we can figure out what to do with
them then; provided we make the frontend discard buttons it doesn't
understand it should be trivial to do this in a backwards compatible
way.

> >> +                          break;
> > ...
> >> diff -r 9977b8785570 tools/xenfb/vncfb.c
> >> --- /dev/null      Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/tools/xenfb/vncfb.c  Sat Sep 30 09:29:38 2006 +0200
> > ...
> >> +static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr cl)
> >> +{
> >> +  rfbScreenInfoPtr server = cl->screen;
> >> +  struct xenfb *xenfb = server->screenData;
> >> +  xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
> > I probably just don't understand the rfbKeySym format, but why is it
> > safe to ignore the high bits of the keycode?
> I suspect this is a rather sloppy and confusing bounds check.
> 
> I further suspect that rfbKeySym is really just an X11 keysym.  These
> are 29 bits, as far as I know.
> 
> We'll dig this up when we address the key encoding issue.  I'd like to
> leave it broken for now.
Fair enough.

> >> +#include <xs.h>
> >> +
> >> +#include "xenfb.h"
> >> +
> >> +// FIXME defend against malicous frontend?
> > Actually, you're mostly there already.  I've pointed out a few things
> > in line, but it seems pretty good for the most part.
> I'd like to keep the FIXME for now, to remind me I want a full audit.
Okay.

> > ...
> >> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
> >> +{
> >> +  xenfb->domid = -1;
> >> +  munmap(xenfb->fb, xenfb->fb_info->mem_length);
> > You can't trust mem_length to be the same as it was when you mapped
> > the page, since it's in the shared area.
> Malicious frontend.  It should be read just once, checked for
> plausibility, and saved for later use.  Same for the other values
> there.
Yep.

> Done except for the checks.
Thanks.

> >> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
> >> +{
> > ...
> >> +  if (xenfb->kbd_info == NULL)
> >> +          goto error;
> >> +
> >> +  n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE - 1)) / 
> >> XC_PAGE_SIZE;
> > If mem_length is near UINT_MAX, this could overflow and lead to you
> > not actually mapping any memory, and then crashing later.  Not that
> > big a deal, I guess.
> Yes.  See above.
Okay.

> >> +  n_fbdirs = n_fbmfns * sizeof(unsigned long);
> > ...
> >> +  n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
> >> +
> >> +  fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ, 
> >> xenfb->fb_info->pd, n_fbdirs);
> >> +  if (fbmfns == NULL)
> >> +          goto error;
> >> +
> >> +  xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ | 
> >> PROT_WRITE, fbmfns, n_fbmfns);
> >> +  if (xenfb->fb == NULL)
> >> +          goto error;
> > Irritatingly, map_foreign_batch doesn't actually return success or
> > failure through its return value, but by setting the high bits on the
> > failed entry in the array you pass in.  If the array is mapped
> > readonly, or is shared with a remote domain, there's no way to detect
> > failure.
> Sounds like a design flaw to me.
That's one way of putting it, certainly. :)

> xc_map_foreign_batch() returns NULL on obvious failures, but the
> ioctl() might cause the behavior you described.
> 
> I looked for other users of xc_map_foreign_batch() and I can't see
> them checking success other than by examining the return value.
> 
> fbmfns[] is mapped PROT_READ from the frontend's domain.
I think the correct fix here is probably just to fix up
xc_map_foreign_batch() to have a more sane calling convention.  As you
say, most of its existing users seem to make the same assumptions as
have happened here.  I can't actually see any good way of dealing with
this in the standard libxc API.

> > You might also want to consider using xc_map_foreign_ranges, since
> > that has a useful return value, but it would mean copying the pfn
> > arrays and translating them to a slightly different format.
> Isn't that function private?
Oops, yes, sorry, forgot about that.

> What do you want me to do here?
I'd leave it as a known bug for now.  We'll add a new interface to
libxc once 3.0.3's dealt with.

> >> +
> >> +  event.type = XENFB_TYPE_SET_EVENTS;
> >> +  event.set_events.flags = XENFB_FLAG_UPDATE;
> >> +  if (xenfb_fb_event(xenfb, &event))
> >> +          goto error;
> > Would it make sense to negotiate this via xenbus at connection setup
> > time?  It's not like it's likely to change during the life of the
> > buffer.
> Can you point to an example of such a negotiation between a frontend
> and a backend via xenbus?
The netif feature flags are probably the most obvious example.  For
instance, to turn on copy rx mode, you first have the backend put
feature-rx-copy=1 in its xenstore area, and then when the frontend
connects it notices this and puts request-rx-copy=1 in its area.  The
backend reads this out as part of connection setup, and rx copy mode
is turned on.

The equivalent in this case would be for the backend to set
request-update=1 in its area when it starts, and then for the frontend
to set provides-update=1 if appropriate.

Of course, this sort of thing only works well for flags which don't
change while the buffer is live.  I'd certainly expect
XENFB_FLAG_UPDATE to fit into that category, but perhaps you have some
future plans which wouldn't work well with this?

> >> +
> >> +  return true;
> >> +
> >> +error:        
> >> +  serrno = errno;
> >> +  if (xenfb->fb)
> >> +          munmap(xenfb->fb, xenfb->fb_info->mem_length);
> > mem_length is shared with the frontend, and so could have changed
> > between mapping the area and unmapping it.
> It's read exactly once now.
Excellent, thank you.

Steven.

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel