Re: [Xen-devel] Fix PVFB backend to validate frontend's frame buffer description

[Pat Campbell <plc@xxxxxxxxxx>]

Markus Armbruster wrote:
> A buggy or malicious frontend can describe its shared framebuffer to
> the backend in a way that makes the backend map an arbitrary amount of
>
>   
snipped out, see inline question below.
>
> diff -r 0a8fc1a62796 tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c  Mon May 12 11:19:09 2008 +0100
> +++ b/tools/ioemu/hw/xenfb.c  Tue May 13 14:53:58 2008 +0200
> @@ -28,8 +28,6 @@
>  #ifndef BTN_LEFT
>  #define BTN_LEFT 0x110 /* from <linux/input.h> */
>  #endif
> -
> -// FIXME defend against malicious frontend?
>  
>  struct xenfb;
>  
> @@ -484,6 +482,68 @@ void xenfb_shutdown(struct xenfb *xenfb)
>       free(xenfb);
>  }
>  
> +static int xenfb_configure_fb(struct xenfb *xenfb, size_t fb_len_lim,
> +                           int width, int height, int depth,
> +                           size_t fb_len, int offset, int row_stride)
> +{
> +     size_t mfn_sz = sizeof(*((struct xenfb_page *)0)->pd);
> +     size_t pd_len = sizeof(((struct xenfb_page *)0)->pd) / mfn_sz;
> +     size_t fb_pages = pd_len * XC_PAGE_SIZE / mfn_sz;
> +     size_t fb_len_max = fb_pages * XC_PAGE_SIZE;
> +     int max_width, max_height;
> +
> +     if (fb_len_lim > fb_len_max) {
> +             fprintf(stderr,
> +                     "FB: fb size limit %zu exceeds %zu, corrected\n",
> +                     fb_len_lim, fb_len_max);
> +             fb_len_lim = fb_len_max;
> +     }
> +     if (fb_len > fb_len_lim) {
> +             fprintf(stderr,
> +                     "FB: frontend fb size %zu limited to %zu\n",
> +                     fb_len, fb_len_lim);
>   
Do we need to set fb_len to fb_len_lim here?
fb_len = fb_len_lim;
> +     }
> +     if (depth != 8 && depth != 16 && depth != 24 && depth != 32) {
> +             fprintf(stderr,
> +                     "FB: can't handle frontend fb depth %d\n",
> +                     depth);
> +             return -1;
> +     }
> +     if (row_stride < 0 || row_stride > fb_len) {
> +             fprintf(stderr,
> +                     "FB: invalid frontend stride %d\n", row_stride);
> +             return -1;
> +     }
> +     max_width = row_stride / (depth / 8);
> +     if (width < 0 || width > max_width) {
> +             fprintf(stderr,
> +                     "FB: invalid frontend width %d limited to %d\n",
> +                     width, max_width);
> +             width = max_width;
> +     }
> +     if (offset < 0 || offset >= fb_len) {
> +             fprintf(stderr,
> +                     "FB: invalid frontend offset %d (max %zu)\n",
> +                     offset, fb_len - 1);
> +             return -1;
> +     }
> +     max_height = (fb_len - offset) / row_stride;
> +     if (height < 0 || height > max_height) {
> +             fprintf(stderr,
> +                     "FB: invalid frontend height %d limited to %d\n",
> +                     height, max_height);
> +             height = max_height;
> +     }
> +     xenfb->fb_len = fb_len;
> +     xenfb->row_stride = row_stride;
> +     xenfb->depth = depth;
> +     xenfb->width = width;
> +     xenfb->height = height;
> +     xenfb->offset = offset;
> +     fprintf(stderr, "Framebuffer %dx%dx%d offset %d stride %d\n",
> +             width, height, depth, offset, row_stride);
> +     return 0;
> +}
>  
>  static void xenfb_on_fb_event(struct xenfb *xenfb)
>  {
> @@ -514,16 +574,18 @@ static void xenfb_on_fb_event(struct xen
>                           || h != event->update.height) {
>                               fprintf(stderr, "%s bogus update clipped\n",
>                                       xenfb->fb.nodename);
> -                             break;
>                       }
>                       xenfb_guest_copy(xenfb, x, y, w, h);
>                       break;
>               case XENFB_TYPE_RESIZE:
> -                     xenfb->width  = event->resize.width;
> -                     xenfb->height = event->resize.height;
> -                     xenfb->depth = event->resize.depth;
> -                     xenfb->row_stride = event->resize.stride;
> -                     xenfb->offset = event->resize.offset;
> +                     if (xenfb_configure_fb(xenfb, xenfb->fb_len,
> +                                            event->resize.width,
> +                                            event->resize.height,
> +                                            event->resize.depth,
> +                                            xenfb->fb_len,
> +                                            event->resize.offset,
> +                                            event->resize.stride) < 0)
> +                             break;
>                       dpy_colourdepth(xenfb->ds, xenfb->depth);
>                       dpy_resize(xenfb->ds, xenfb->width, xenfb->height, 
> xenfb->row_stride);
>                       if (xenfb->ds->shared_buf)
> @@ -745,29 +807,18 @@ static int xenfb_read_frontend_fb_config
>          xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", 
> "1");
>          xenfb->refresh_period = -1;
>  
> -        /* TODO check for permitted ranges */
> -        fb_page = xenfb->fb.page;
> -        xenfb->depth = fb_page->depth;
> -        xenfb->width = fb_page->width;
> -        xenfb->height = fb_page->height;
> -        /* TODO check for consistency with the above */
> -        xenfb->fb_len = fb_page->mem_length;
> -        xenfb->row_stride = fb_page->line_length;
> -
> -        /* Protect against hostile frontend, limit fb_len to max allowed */
>          if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
>                              &videoram) < 0)
>                  videoram = 0;
> -        videoram = videoram * 1024 * 1024;
> -        if (videoram && xenfb->fb_len > videoram) {
> -                fprintf(stderr, "Framebuffer requested length of %zd 
> exceeded allowed %d\n",
> -                        xenfb->fb_len, videoram);
> -                xenfb->fb_len = videoram;
> -                if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
> -                        xenfb->height = xenfb->fb_len / xenfb->row_stride;
> -        }
> -        fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
> -                fb_page->depth, fb_page->width, fb_page->height, 
> fb_page->line_length);
> +     fb_page = xenfb->fb.page;
> +     if (xenfb_configure_fb(xenfb, videoram * 1024 * 1024U,
> +                            fb_page->width, fb_page->height, fb_page->depth,
> +                            fb_page->mem_length, 0, fb_page->line_length)
> +         < 0) {
> +             errno = EINVAL;
> +             return -1;
> +     }
> +
>          if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
>               return -1;
>  
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>   


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