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@eu.citrix.com>
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@lists.xensource.com
http://lists.xensource.com/xen-devel
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. -- Keir On 9/11/07 15:13, "Samuel Thibault" <samuel.thibault@eu.citrix.com> 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@eu.citrix.com> > > 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@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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@eu.citrix.com> 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@eu.citrix.com> > > > > 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@lists.xensource.com > > http://lists.xensource.com/xen-devel > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Mon, 2007-11-12 at 09:35 +0000, Kieran Mansley wrote:> I can test and provide a patch for this later today.Here it is. I left in the check of the remote domain''s iomem permission as although it is unnecessary in the case of dom0 providing the grant it still makes logical sense. Thanks for pointing this problem out. Signed-off-by Kieran Mansley <kmansley@solarflare.com> diff -r e40591366a0f xen/common/grant_table.c --- a/xen/common/grant_table.c Mon Nov 12 09:53:14 2007 +0000 +++ b/xen/common/grant_table.c Mon Nov 12 11:49:19 2007 +0000 @@ -332,7 +332,9 @@ __gnttab_map_grant_ref( if ( op->flags & GNTMAP_host_map ) { /* Could be an iomem page for setting up permission */ - if ( is_iomem_page(frame) ) + if ( rd->domain_id == 0 && + is_iomem_page(frame) && + iomem_access_permitted(rd, frame, frame) ) { is_iomem = 1; if ( iomem_permit_access(ld, frame, frame) ) @@ -527,7 +529,8 @@ __gnttab_unmap_common( op->flags)) < 0 ) goto unmap_out; } - else if ( is_iomem_page(op->frame) && + else if ( rd->domain_id == 0 && + is_iomem_page(op->frame) && iomem_access_permitted(ld, op->frame, op->frame) ) { if ( (rc = iomem_deny_access(ld, op->frame, op->frame)) < 0 ) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/11/07 09:35, "Kieran Mansley" <kmansley@solarflare.com> wrote:> 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.I didn''t realise this was how it worked. I think it''s a bad idea -- mapping the grantref should map the I/O page. The mapping domain''s io capabilities should not be affected. Apart from being the obvious semantics for map_grant, using the current scheme we cannot be sure when all mappings to the granted page have gone away. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 2007-11-13 at 04:48 +0000, Keir Fraser wrote:> On 12/11/07 09:35, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > > 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. > > I didn''t realise this was how it worked. I think it''s a bad idea -- mapping > the grantref should map the I/O page. The mapping domain''s io capabilities > should not be affected. Apart from being the obvious semantics for > map_grant,I agree that mapping the I/O page when you map the grant ref makes for a much better interface. I''d just obviously got the wrong end of the stick and thought that you wanted it the other way when we first discussed this, and it hadn''t occurred to me that doing the map without first getting the right permissions was sane. Attached is pair of patches against the current xen- unstable.hg/linux-2.6.18-xen.hg (i.e. instead of not on top of the other patch I posted in this thread) that should do the I/O page map when the grant is mapped. The I/O capabilities of the domain are no longer modified. It seems to work for me, is definitely an improvement in the API, and should fix the original bug that was posted. I added a GNTMAP_nocache flag to match the GNTMAP_readonly etc flags so that you can get the equivalent behaviour of ioremap_nocache() vs ioremap(). The grant is mapped in as normal; e.g.: struct gnttab_map_grant_ref op; gnttab_set_map_op(&op, (unsigned long)vaddr, GNTMAP_host_map | GNTMAP_nocache, gnt_ref, dev->otherend_id); BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1)); I''m slightly surprised that without the call to iomem_permit_access() I don''t trip over the iomem_access_permitted() check in xen/arch/x86/mm.c:get_page_from_l1e() when the grant is mapped. I hope this doesn''t indicate that I''ve missed something. Thanks Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 13/11/07 16:40, "Kieran Mansley" <kmansley@solarflare.com> wrote:> I''m slightly surprised that without the call to iomem_permit_access() I > don''t trip over the iomem_access_permitted() check in > xen/arch/x86/mm.c:get_page_from_l1e() when the grant is mapped. I hope > this doesn''t indicate that I''ve missed something.get_page_from_l1e() is not executed on the map_grant path, which explains this behaviour. As for GNTMAP_nocache, I think actually we should not trust the mapper to get the cache attributes correct (because sometimes if the attributes are wrong the system can be destabilised). Actually we now in xen-unstable have cache-attribute tracking for RAM pages. The cache attributes for the granted mapping should pick up PAT/PWT/PCD from the page''s count_info field. If dom0 has the page mapped with the right attributes this will then do the right thing immediately. If dom0 doesn''t have the page mapped then we''ll need to do a bit more work... -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 2007-11-13 at 16:45 +0000, Keir Fraser wrote:> As for GNTMAP_nocache, I think actually we should not trust the mapper to > get the cache attributes correct (because sometimes if the attributes are > wrong the system can be destabilised).Is this just (or mainly?) a concern for RAM pages? If not, you''ll have the same problem when using the domctl iomem_permission and ioremap_nocache() in the guest.> Actually we now in xen-unstable have > cache-attribute tracking for RAM pages. The cache attributes for the granted > mapping should pick up PAT/PWT/PCD from the page''s count_info field. If dom0 > has the page mapped with the right attributes this will then do the right > thing immediately. If dom0 doesn''t have the page mapped then we''ll need to > do a bit more work...dom0 does have the page mapped, but I think there is no page_info struct (and so no count_info) for some I/O memory pages. Perhaps a short-term fix would be to remove the GNTMAP_nocache option that I''ve added and instead just make all I/O memory pages that get mapped through this mechanism forced to _PAGE_PCD. That way the mapper doesn''t get to choose (and we loose the ability to do an ioremap() equivalent as opposed to an ioremap_nocache() equivalent, but that''s not a big problem right now). If that sounds agreeable I can spin another patch. Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
You make a good point. Let''s instead allow the granter to explicitly specify the cache attributes in the grant entry. We have space in the flags field. Let''s use bits 5,6,7 of that field. They can have the same format as the three contiguous bits we have added in page->count_info. Conveniently, all zeroes means map-WB-cacheable, which is the correct default. -- Keir On 13/11/07 17:06, "Kieran Mansley" <kmansley@solarflare.com> wrote:> On Tue, 2007-11-13 at 16:45 +0000, Keir Fraser wrote: >> As for GNTMAP_nocache, I think actually we should not trust the mapper to >> get the cache attributes correct (because sometimes if the attributes are >> wrong the system can be destabilised). > > Is this just (or mainly?) a concern for RAM pages? If not, you''ll have > the same problem when using the domctl iomem_permission and > ioremap_nocache() in the guest. > >> Actually we now in xen-unstable have >> cache-attribute tracking for RAM pages. The cache attributes for the granted >> mapping should pick up PAT/PWT/PCD from the page''s count_info field. If dom0 >> has the page mapped with the right attributes this will then do the right >> thing immediately. If dom0 doesn''t have the page mapped then we''ll need to >> do a bit more work... > > dom0 does have the page mapped, but I think there is no page_info struct > (and so no count_info) for some I/O memory pages. Perhaps a short-term > fix would be to remove the GNTMAP_nocache option that I''ve added and > instead just make all I/O memory pages that get mapped through this > mechanism forced to _PAGE_PCD. That way the mapper doesn''t get to > choose (and we loose the ability to do an ioremap() equivalent as > opposed to an ioremap_nocache() equivalent, but that''s not a big problem > right now). > > If that sounds agreeable I can spin another patch. > > Kieran >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
hi I have read the paper about parallex FS in xen , and I am interested in it ,but I notice that parallex FS which is in the tree for earlier version ,has been dropped out in the later version, what is the reason for dropping it out, in my opinion,it is an interesting and promising technech,is it ? or are there some disadvantages in it ,or are there some even more interesting substituent tech for it? I am confused about it ,could someone give me some exeplaination thanks in advance _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 2007-11-13 at 17:12 +0000, Keir Fraser wrote:> You make a good point. Let''s instead allow the granter to explicitly specify > the cache attributes in the grant entry. We have space in the flags field. > Let''s use bits 5,6,7 of that field. They can have the same format as the > three contiguous bits we have added in page->count_info. Conveniently, all > zeroes means map-WB-cacheable, which is the correct default.Attached is another spin of the patches to do this, based on a tree with 16067 reverted as requested.>From the guest API point of view, previously gnttab_grant_foreign_access() (and _ref()) took a "readonly" flag as an argument. This is now a more general flags field to allow the granter to specify the cache attributes. I also took the opportunity to remove the "readonly" flag from gnttab_remove_foreign_access() as it was unused and I couldn''t see how to make useful in light of this change. Within the hypervisor, the granter''s specified cache attributes are ignored at the point of mapping unless it is an I/O memory grant; for RAM grants the cache attributes should I think be picked up automatically using the cache-attribute tracking you mentioned, and this seemed safer than allowing the granter to override. In the case of an I/O memory map the granter-specified flags are passed into create_grant_host_mapping() to add to the page table entry''s flags. I/0 memory maps using the grant table are only allowed in the case where dom0 is doing the granting, the granter has iomem_access_permitted(), GNTMAP_host_map is specified and GNTMAP_device_map is not. Reference counting using put_page/get_page is only done for those I/0 memory pages that have mfn_valid() I''m pretty happy with this now. Signed-off-by Kieran Mansley <kmansley@solarflare.com> Thanks Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> I have read the paper about parallex FS in xen , and I am interested in > it ,but I notice that parallex FS which is in the tree for earlier > version ,has been dropped out in the later version, what is the reason > for dropping it out, in my opinion,it is an interesting and promising > technech,is it ? or are there some disadvantages in it ,or are there > some even more interesting substituent tech for it? > I am confused about it ,could someone give me some exeplainationI don''t think the parallax in the mainline tree was ever developed very heavily; it seems likely that the version written up in the paper was based on a modified source tree. But the code went unmaintained for some time and was presumably removed from the source tree due to it getting old and not working properly... I agree it seemed like a really interesting technology and it''s a shame not to see more done with it. I get the impression that most enterprises are using NAS/SAN solutions already, so perhaps there wasn''t sufficient demand for Parallax as a competing solution... Cheers, Mark -- Dave: Just a question. What use is a unicyle with no seat? And no pedals! Mark: To answer a question with a question: What use is a skateboard? Dave: Skateboards have wheels. Mark: My wheel has a wheel! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Actually, Parallax is currently under active development. Dutch Meyer recently presented a progress report on it at the Xen summit. I believe Dutch is planning a release of the updated Parallax source RSN, but I''ll leave it to him (copied) to commit to a date. ;) a. On Dec 1, 2007 9:18 PM, Mark Williamson <mark.williamson@cl.cam.ac.uk> wrote:> > I have read the paper about parallex FS in xen , and I am interested in > > it ,but I notice that parallex FS which is in the tree for earlier > > version ,has been dropped out in the later version, what is the reason > > for dropping it out, in my opinion,it is an interesting and promising > > technech,is it ? or are there some disadvantages in it ,or are there > > some even more interesting substituent tech for it? > > I am confused about it ,could someone give me some exeplaination > > I don''t think the parallax in the mainline tree was ever developed very > heavily; it seems likely that the version written up in the paper was based > on a modified source tree. > > But the code went unmaintained for some time and was presumably removed from > the source tree due to it getting old and not working properly... I agree it > seemed like a really interesting technology and it''s a shame not to see more > done with it. I get the impression that most enterprises are using NAS/SAN > solutions already, so perhaps there wasn''t sufficient demand for Parallax as > a competing solution... > > Cheers, > Mark > > -- > Dave: Just a question. What use is a unicyle with no seat? And no pedals! > Mark: To answer a question with a question: What use is a skateboard? > Dave: Skateboards have wheels. > Mark: My wheel has a wheel! > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> Actually, Parallax is currently under active development. Dutch Meyer > recently presented a progress report on it at the Xen summit. I > believe Dutch is planning a release of the updated Parallax source > RSN, but I''ll leave it to him (copied) to commit to a date. ;)Ah, that''s excellent news! I had the impression that you''d moved onto other things and was worried that it''d lost priority. It''ll be awesome to see a parallax release. I hope the summit proceedings get online soon so I can find out what''s going on in Xenland ;-) Cheers, Mark> a. > > On Dec 1, 2007 9:18 PM, Mark Williamson <mark.williamson@cl.cam.ac.uk>wrote:> > > I have read the paper about parallex FS in xen , and I am interested in > > > it ,but I notice that parallex FS which is in the tree for earlier > > > version ,has been dropped out in the later version, what is the reason > > > for dropping it out, in my opinion,it is an interesting and promising > > > technech,is it ? or are there some disadvantages in it ,or are there > > > some even more interesting substituent tech for it? > > > I am confused about it ,could someone give me some exeplaination > > > > I don''t think the parallax in the mainline tree was ever developed very > > heavily; it seems likely that the version written up in the paper was > > based on a modified source tree. > > > > But the code went unmaintained for some time and was presumably removed > > from the source tree due to it getting old and not working properly... I > > agree it seemed like a really interesting technology and it''s a shame not > > to see more done with it. I get the impression that most enterprises are > > using NAS/SAN solutions already, so perhaps there wasn''t sufficient > > demand for Parallax as a competing solution... > > > > Cheers, > > Mark > > > > -- > > Dave: Just a question. What use is a unicyle with no seat? And no > > pedals! Mark: To answer a question with a question: What use is a > > skateboard? Dave: Skateboards have wheels. > > Mark: My wheel has a wheel! > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel-- Dave: Just a question. What use is a unicyle with no seat? And no pedals! Mark: To answer a question with a question: What use is a skateboard? Dave: Skateboards have wheels. Mark: My wheel has a wheel! _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel