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