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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Steven Smith <sos22-xen@xxxxxxxxxxxxx> writes:

[...]
>> Okay, here's the next patch.  It doesn't yet take care of shadow
>> translate mode, just because I want to get it out of the door.  Next
>> week, I hope...
> Okay.
>
>> Oh, and it also doesn't include Daniel Berrange's locking fixes to
>> LibVNCServer, which you really, really need:
>> http://lists.xensource.com/archives/html/xen-devel/2006-09/msg00371.html
> Did you look at how much work it would be to use the qemu VNC server
> instead of libVNCServer?  I suspect quite a lot, but it might be
> necessary if libVNCServer really is as bad as it appears.

Not yet, sorry.

> This new patch is really pretty good.  Here's the summary of the bits
> which might actually matter:
>
> -- The backend still isn't proof against a malicious frontend.  This
>    (might) mean that root in an unprivileged domain can get root in
>    dom0, which is a fairly major problem.  Fixing this should be
>    fairly easy.

Yes, this needs to be done.

> -- 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.

There's a TODO comment in xenfb.c to remind me that I want to try the
xenbus_device API.

> -- You don't seem to support mice with more than three buttons, or
>    scroll wheels.  It'd be perfectly acceptable to just push this
>    to a later version, though.

See mouse discussion below.

> -- As you say, the keymaping and shadow problems are still there.

One step at a time.

> Other than that, there's nothing here which would take more than
> an hour or so to fix.
>
>
>> +}
>> +
>> +int sdl2linux[1024] = {
> SDLK_LAST, maybe?

Done, plus bounds check on access.

>> +    [SDLK_a] = KEY_A,
>> +    [SDLK_b] = KEY_B,
> ...
>> +int main(int argc, char **argv)
>> +{
> ...
>> +    while ((opt = getopt_long(argc, argv, "d:t:", options,
>> +                              NULL)) != -1) {
>> +            switch (opt) {
>> +                case 'd':
>> +                    domid = strtol(optarg, &endp, 10);
>> +                    if (endp == optarg || *endp || errno) {
> Is testing errno really a good idea here?  strtol doesn't zero it on
> success, so you're relying on it already being zero, which happens to
> be true this time, as far as I can see, but is a somewhat dubious
> assumption.  The other two tests should be enough, unless I'm
> confused.

Testing errno isn't so useful here, because we silently truncate the
value anyway.  Removed.

> ...
>> +    if (!xenfb_attach_dom(xenfb, domid)) {
>> +            fprintf(stderr, "Could not connect to domain (%s)\n",
>> +                    strerror(errno));
>> +            exit(1);
>> +        }
>> +
>> +    SDL_Init(SDL_INIT_VIDEO);
> This can fail.
>
> I'm aware that a lot of code currently in the tree is less than
> perfect about checking return values and so forth, especially the
> userspace tools, and that it doesn't actually make that much
> difference in practice.  It'd still be good not to introduce any more
> of this kind of bug without good reason.
>
> (And, yeah, I know this one's been there for ages and I've missed it
> twice.  Sorry about that. )

Fixing.  Agreed on the wisdom of checking return values.

>> +
>> +    fd = xenfb_get_fileno(xenfb);
>> +
>> +    data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height, xenfb->depth,
>> +                                SDL_SWSURFACE);
> This can fail.

Fixing.

>> +
>> +    data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
>> +                                        xenfb->width, xenfb->height,
>> +                                        xenfb->depth, xenfb->row_stride,
>> +                                        0xFF0000, 0xFF00, 0xFF, 0);
> This can fail.

Fixing.

>> +
>> +        if (title == NULL)
>> +            title = strdup("xen-sdlfb");
>> +        SDL_WM_SetCaption(title, title);
>> +
>> +    r.x = r.y = 0;
>> +    r.w = xenfb->width;
>> +    r.h = xenfb->height;
>> +    SDL_BlitSurface(data.src, &r, data.dst, &r);
>> +    SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
>> +
>> +    xenfb->update = sdl_update;
>> +    xenfb->user_data = &data;
>> +
>> +    SDL_ShowCursor(0);
>> +
>> +    /*
>> +     * We need to wait for fd becoming ready or SDL events to
>> +     * arrive.  We time out the select after 10ms to poll for SDL
>> +     * events.  Clunky, but works.  Could avoid the clunkiness
>> +     * with a separate thread.
>> +     */
> Thanks for this.
>
>> +    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?

> ...
>> +            while (SDL_PollEvent(&event)) {
> ...
>> +                    case SDL_MOUSEBUTTONDOWN:
>> +                    case SDL_MOUSEBUTTONUP:
>> +                            xenfb_send_button(xenfb,
>> +                                    event.type == SDL_MOUSEBUTTONDOWN,
>> +                                    3 - event.button.button);
> Why 3 - button?

Anthony speedcoding?  %-}

>                  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 :)

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.

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.

>> +                            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.

>> +}
>> +
>> +static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr cl)
>> +{
>> +    static int last_x = -1, last_y = -1;
>> +    static int last_button = -1;
>> +    rfbScreenInfoPtr server = cl->screen;
>> +    struct xenfb *xenfb = server->screenData;
>> +
>> +    if (last_button != -1) {
>> +            int i;
>> +
>> +            for (i = 0; i < 8; i++) {
>> +                    if ((last_button & (1 << i)) !=
>> +                        (buttonMask & (1 << i))) {
>> +                            xenfb_send_button(xenfb, buttonMask & (1 << i),
>> +                                              2 - i);
> Why 2 - i?  What if button three gets pressed?  We send button 0xff to
> the frontend and it does ...?
>
> And if VNC can't generate button 3, why do we scan for eight buttons?

See button discussion above.

>> +                    }
>> +            }
>> +    }
>> +
>> +    if (last_x != -1)
>> +            xenfb_send_motion(xenfb, x - last_x, y - last_y);
>> +
>> +    last_button = buttonMask;
>> +
>> +    last_x = x;
>> +    last_y = y;
>> +}
>> +
> ...
>> +static int vnc_start_viewer(int port) 
>> +{
>> +    int pid;
>> +    char s[16];
>> +
>> +    snprintf(s, sizeof(s), ":%d", port);
>> +    switch (pid = fork()) {
>> +    case -1:
>> +            fprintf(stderr, "vncviewer failed fork\n");
>> +            exit(1);
>> +
>> +    case 0: /* child */
>> +            execlp("vncviewer", "vncviewer", s, NULL);
>> +            fprintf(stderr, "vncviewer execlp failed\n");
> Getting strerror(errno) out here would be good.

Sure.

>> +            exit(1);
>> +
>> +    default:
>> +            return pid;
>> +    }
>> +}
>> +
> ...
>> +int main(int argc, char **argv)
>> +{
>> +    rfbScreenInfoPtr server;
>> +    char *fake_argv[7] = { "vncfb", "-rfbport", "5901", 
>> +                               "-desktop", "xen-vncfb", 
>> +                               "-listen", "0.0.0.0" };
>> +    int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
> The initialisation for fake_argv is spread out over quite a lot of
> code.  It'd be a bit easier to follow if it was all in one place.  Not
> a big deal, though.
>
> ...
>> diff -r 9977b8785570 tools/xenfb/xenfb.c
>> --- /dev/null        Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/xenfb.c    Sat Sep 30 09:29:38 2006 +0200
> ...
>> +#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.

> ...
>> +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.  Done except for the checks.

>> +    munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +    munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +    xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +    xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +}
> ...
>> +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.

>> +    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.

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.

> 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?

What do you want me to do here?

>> +
>> +    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?

>> +
>> +    munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
>> +
>> +    xenfb->domid = domid;
>> +
>> +    xenfb->pub.pixels = xenfb->fb;
>> +
>> +    xenfb->pub.row_stride = xenfb->fb_info->line_length;
>> +    xenfb->pub.depth = xenfb->fb_info->depth;
>> +    xenfb->pub.width = xenfb->fb_info->width;
>> +    xenfb->pub.height = xenfb->fb_info->height;
> It'd be good if you could check that this information was plausible
> for mem_length.

Yes.  See above.

>> +
>> +    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.

>> +    if (fbmfns)
>> +            munmap(fbmfns, xenfb->fb_info->mem_length);
> Eh?  I think you mean n_fbdirs * XC_PAGE_SIZE.

Good catch.

>> +        if (xenfb->kbd_info)
>> +            munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +        if (xenfb->fb_info)
>> +            munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +        if (xenfb->kbd_port);
>> +    xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +        if (xenfb->fbdev_port)
>> +            xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +        if (xsh) 
>> +            xs_daemon_close(xsh);
>> +        errno = serrno;
>> +        return false;
>> +}
>
> Steven.

I have a few more things to fix for the next patch.

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