Re: [Xen-devel] [PATCH] grant table and bogus mfns

[Kieran Mansley <kmansley@xxxxxxxxxxxxxx>]

On Sat, 2007-11-10 at 09:28 +0000, Keir Fraser wrote:
> Thanks, but actually I think the iomem integration is more broken than it
> first appears. I'm not sure why the local iomem permissions are modified at
> all. The remote permissions should be interrogated, and that should suffice
> alone. But you certainly have the gist of a fix here -- AFAICS the code
> as-is allows unprivileged domains to 'grant' each other access to arbitrary
> pages of I/O memory, which isn't good. I've cc'ed Kieran who was the
> originator of that code.

As background: this code was written to provide a means to
programatically give I/O memory permissions to unprivileged domains,
roughly equivalent to a domctl iomem_permission operation.  The
suggestion at the time was that we should use the grant tables to
achieve this.  It's supposed to work as follows:

 1) dom0 does a grant op for a page of I/O memory; at this stage no
different to a normal grant.
 2) grant reference passed (e.g. through xenstore) to domU  
 3) domU performs a map operation on that grant
 4) hypervisor notices that the grant is for an I/O memory page and
instead of mapping it to a domU virtual address it instead sets up the
I/O mem permissions for that domain to access the region (ie. calls
iomem_permit_access())
 5) domU can then call ioremap() to get a kernel virtual address for the
I/O memory region, and access it as normal.

The bug I think stems from a weakness in iomem_page_test() (recently
renamed something like is_iomem_page() I think).  I had intended that
the call to check the owner of the page was dom_io would prevent
unprivileged domains granting access, but it of course needs to also
check that the granting domain owns the page.  i.e. a quick fix would be
to check where iomem_page_test() is called in 
__gnttab_map_grant_ref() that "rd == dom_io".

I can test and provide a patch for this later today.

If we wanted it to be more general (so that unprivileged domains could
legitimately give others access to their own regions of I/O memory) then
the suggestion of testing iomem_access_permitted() on the remote domain
would then be necessary, but if we restrict it to dom0 providing access
(as was initially intended) I'm not sure that is necessary.

Kieran

>  -- Keir
> 
> On 9/11/07 15:13, "Samuel Thibault" <samuel.thibault@xxxxxxxxxxxxx> wrote:
> 
> > Hi,
> > 
> > While developping a frontend, I noticed that if I gave a grant on mfn 0
> > to a backend (which is bogus), that backend would happily be allowed to
> > map it, and hence Oops...
> > 
> > This is because mfn 0 is considered as iomem, and in that case
> > gnttab_map_grant_ref only checks that the target domain is allowed to
> > access it.  The patch below makes it check that the source domain was
> > allowed to access it (and then give the target access to it).
> > 
> > Samuel
> > 
> > Signed-Off-By: Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>
> > 
> > diff -r 4bf62459d45a xen/common/grant_table.c
> > --- a/xen/common/grant_table.c Wed Nov 07 11:29:38 2007 +0000
> > +++ b/xen/common/grant_table.c Fri Nov 09 15:01:57 2007 +0000
> > @@ -335,7 +335,8 @@ __gnttab_map_grant_ref(
> >          if ( iomem_page_test(frame, mfn_to_page(frame)) )
> >          {
> >              is_iomem = 1;
> > -            if ( iomem_permit_access(ld, frame, frame) )
> > +            if ( !iomem_access_permitted(rd, frame, frame)
> > +                  || iomem_permit_access(ld, frame, frame) )
> >              {
> >                  gdprintk(XENLOG_WARNING,
> >                           "Could not permit access to grant frame "
> > _______________________________________________
> > 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