| Message ID | ad758354-b8e7-f5ef-b3cf-94a6a0d92bd4@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] gnttab: don't expose host physical address without need | expand |
On 05/12/2019 15:34, Jan Beulich wrote: > Translated domains shouldn't see host physical addresses. While the > address is also not supposed to be handed back even to non-translated > domains when GNTMAP_device_map is not set (as explicitly stated by a > comment in the public header), PV kernels (Linux at least) assume the > field to get populated nevertheless. This really means that the public header needs correcting. The field may not have intended to escape out of Xen, but it is defacto part of the ABI now. > (Similarly mapkind() should check only GNTMAP_device_map.) Is this comment stale, or have I misunderstood some of the reasoning? > > Along these lines split the paging mode related check near the top of > map_grant_ref() to handle the "external" and "translated" cases > separately (GNTMAP_device_map use getting tied to being non-translated > rather than non-external). > > Still along these lines in the unmapping case there's no point checking > ->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field > isn't going to be consumed). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v4: Re-base over dropped patches. > v3: New. > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -967,10 +967,16 @@ map_grant_ref( > } > > if ( unlikely(paging_mode_external(ld) && > - (op->flags & (GNTMAP_device_map|GNTMAP_application_map| > - GNTMAP_contains_pte))) ) > + (op->flags & (GNTMAP_application_map|GNTMAP_contains_pte))) ) > { > - gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n"); > + gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n"); > + op->status = GNTST_general_error; > + return; > + } > + > + if ( paging_mode_translate(ld) && unlikely(op->flags & GNTMAP_device_map) ) > + { > + gdprintk(XENLOG_INFO, "No device mapping in translated domain\n"); > op->status = GNTST_general_error; > return; > } > @@ -1213,7 +1219,8 @@ map_grant_ref( > if ( need_iommu ) > double_gt_unlock(lgt, rgt); > > - op->dev_bus_addr = mfn_to_maddr(mfn); > + op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr > + : mfn_to_maddr(mfn); > op->handle = handle; > op->status = GNTST_okay; > > @@ -1394,7 +1401,7 @@ unmap_common( > > op->mfn = act->mfn; > > - if ( op->dev_bus_addr && > + if ( op->dev_bus_addr && (flags & GNTMAP_device_map) && Drop the first clause entirely? act->mfn will never be 0 so can subsume the check with one fewer branch. ~Andrew > unlikely(op->dev_bus_addr != mfn_to_maddr(act->mfn)) ) > PIN_FAIL(act_release_out, GNTST_general_error, > "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
On 05/12/2019 15:47, Andrew Cooper wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -1394,7 +1401,7 @@ unmap_common( >> >> op->mfn = act->mfn; >> >> - if ( op->dev_bus_addr && >> + if ( op->dev_bus_addr && (flags & GNTMAP_device_map) && > Drop the first clause entirely? act->mfn will never be 0 so can subsume > the check with one fewer branch. Never mind. I've just remembered why 0 is special, and the check needs to stay. ~Andrew
On 05.12.2019 16:47, Andrew Cooper wrote: > On 05/12/2019 15:34, Jan Beulich wrote: >> Translated domains shouldn't see host physical addresses. While the >> address is also not supposed to be handed back even to non-translated >> domains when GNTMAP_device_map is not set (as explicitly stated by a >> comment in the public header), PV kernels (Linux at least) assume the >> field to get populated nevertheless. > > This really means that the public header needs correcting. The field > may not have intended to escape out of Xen, but it is defacto part of > the ABI now. Well, that's one of two possible routes. The other is to have, like you did suggest earlier on, a mode in which we behave more strictly, and current Linux then wouldn't work on such a Xen until fixed. >> (Similarly mapkind() should check only GNTMAP_device_map.) > > Is this comment stale, or have I misunderstood some of the reasoning? It's certainly not stale. mapkind() is used to determine whether IOMMU mapping adjustments are needed. With this, it should in principle only consider whether the current operation would possibly alter IOMMU mapping needs. What needs doing should, according to my interpretation of the originally intended design, only depend on current and prior requests with GNTMAP_device_map set. Jan
On 05/12/2019 16:57, Jan Beulich wrote: > On 05.12.2019 16:47, Andrew Cooper wrote: >> On 05/12/2019 15:34, Jan Beulich wrote: >>> Translated domains shouldn't see host physical addresses. While the >>> address is also not supposed to be handed back even to non-translated >>> domains when GNTMAP_device_map is not set (as explicitly stated by a >>> comment in the public header), PV kernels (Linux at least) assume the >>> field to get populated nevertheless. >> This really means that the public header needs correcting. The field >> may not have intended to escape out of Xen, but it is defacto part of >> the ABI now. > Well, that's one of two possible routes. The other is to have, like > you did suggest earlier on, a mode in which we behave more strictly, > and current Linux then wouldn't work on such a Xen until fixed. I'm sorely tempted to introduce a "fully strict mode" right now, behind a command line option, which chops off all the bits which shouldn't be usable in their current form. However, nothing, not even dom0, will boot successfully, so it probably isn't a great idea right now. Also, we'd have an easier time starting from nothing and whitelisting ok things in, than attempt to locate everything that we should blacklist in the current configuration. > >>> (Similarly mapkind() should check only GNTMAP_device_map.) >> Is this comment stale, or have I misunderstood some of the reasoning? > It's certainly not stale. mapkind() is used to determine whether > IOMMU mapping adjustments are needed. With this, it should in > principle only consider whether the current operation would > possibly alter IOMMU mapping needs. What needs doing should, > according to my interpretation of the originally intended design, > only depend on current and prior requests with GNTMAP_device_map > set. That's all reasonable, but there are no edits to mapkind(), so I'm confused as to why this is present in the commit message. ~Andrew
On 06.12.2019 20:48, Andrew Cooper wrote: > On 05/12/2019 16:57, Jan Beulich wrote: >> On 05.12.2019 16:47, Andrew Cooper wrote: >>> On 05/12/2019 15:34, Jan Beulich wrote: >>>> Translated domains shouldn't see host physical addresses. While the >>>> address is also not supposed to be handed back even to non-translated >>>> domains when GNTMAP_device_map is not set (as explicitly stated by a >>>> comment in the public header), PV kernels (Linux at least) assume the >>>> field to get populated nevertheless. >>> This really means that the public header needs correcting. The field >>> may not have intended to escape out of Xen, but it is defacto part of >>> the ABI now. >> Well, that's one of two possible routes. The other is to have, like >> you did suggest earlier on, a mode in which we behave more strictly, >> and current Linux then wouldn't work on such a Xen until fixed. > > I'm sorely tempted to introduce a "fully strict mode" right now, behind > a command line option, which chops off all the bits which shouldn't be > usable in their current form. > > However, nothing, not even dom0, will boot successfully, so it probably > isn't a great idea right now. Also, we'd have an easier time starting > from nothing and whitelisting ok things in, than attempt to locate > everything that we should blacklist in the current configuration. Okay, I'll add a remark to the public header comment then for now. >>>> (Similarly mapkind() should check only GNTMAP_device_map.) >>> Is this comment stale, or have I misunderstood some of the reasoning? >> It's certainly not stale. mapkind() is used to determine whether >> IOMMU mapping adjustments are needed. With this, it should in >> principle only consider whether the current operation would >> possibly alter IOMMU mapping needs. What needs doing should, >> according to my interpretation of the originally intended design, >> only depend on current and prior requests with GNTMAP_device_map >> set. > > That's all reasonable, but there are no edits to mapkind(), so I'm > confused as to why this is present in the commit message. Well, it's the same underlying problem - improper separation of host_map from device_map. I've added this just to emphasize that this likely wasn't an oversight, but intentional (yet wrong, at least afaict). Jan
Hi, On 05/12/2019 15:34, Jan Beulich wrote: > Translated domains shouldn't see host physical addresses. While the > address is also not supposed to be handed back even to non-translated > domains when GNTMAP_device_map is not set (as explicitly stated by a > comment in the public header), PV kernels (Linux at least) assume the > field to get populated nevertheless. (Similarly mapkind() should check > only GNTMAP_device_map.) > > Along these lines split the paging mode related check near the top of > map_grant_ref() to handle the "external" and "translated" cases > separately (GNTMAP_device_map use getting tied to being non-translated > rather than non-external). > > Still along these lines in the unmapping case there's no point checking > ->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field > isn't going to be consumed). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v4: Re-base over dropped patches. > v3: New. > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -967,10 +967,16 @@ map_grant_ref( > } > > if ( unlikely(paging_mode_external(ld) && > - (op->flags & (GNTMAP_device_map|GNTMAP_application_map| > - GNTMAP_contains_pte))) ) > + (op->flags & (GNTMAP_application_map|GNTMAP_contains_pte))) ) > { > - gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n"); > + gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n"); > + op->status = GNTST_general_error; > + return; > + } > + > + if ( paging_mode_translate(ld) && unlikely(op->flags & GNTMAP_device_map) ) There is at least one instance in Linux where GNTMAP_device_map may be given regardless the type of the guest. See dmabuf_exp_from_refs() in drivers/xen/gntdev-dmabuf.c. How are you going to deal with them? > + { > + gdprintk(XENLOG_INFO, "No device mapping in translated domain\n"); > op->status = GNTST_general_error; > return; > } > @@ -1213,7 +1219,8 @@ map_grant_ref( > if ( need_iommu ) > double_gt_unlock(lgt, rgt); > > - op->dev_bus_addr = mfn_to_maddr(mfn); > + op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr > + : mfn_to_maddr(mfn); The "host_addr" is pretty confusing. I first thought it would be a "Host Physical Address" but it seems to be a "Guest Physical address" If so, this is going to break Linux Dom0 on Arm where it is expected to return the machine physical address to have a DMA fully working. I don't abide with the current behavior on Arm, but I don't think we should break them when there are no replacement for it. So it would be better if we look at a different approach (i.e a new flag or strict mode) in order to avoid breakage. Cheers,
On 16.12.2019 15:19, Julien Grall wrote: > On 05/12/2019 15:34, Jan Beulich wrote: >> Translated domains shouldn't see host physical addresses. While the >> address is also not supposed to be handed back even to non-translated >> domains when GNTMAP_device_map is not set (as explicitly stated by a >> comment in the public header), PV kernels (Linux at least) assume the >> field to get populated nevertheless. (Similarly mapkind() should check >> only GNTMAP_device_map.) >> >> Along these lines split the paging mode related check near the top of >> map_grant_ref() to handle the "external" and "translated" cases >> separately (GNTMAP_device_map use getting tied to being non-translated >> rather than non-external). >> >> Still along these lines in the unmapping case there's no point checking >> ->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field >> isn't going to be consumed). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v4: Re-base over dropped patches. >> v3: New. >> >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -967,10 +967,16 @@ map_grant_ref( >> } >> >> if ( unlikely(paging_mode_external(ld) && >> - (op->flags & (GNTMAP_device_map|GNTMAP_application_map| >> - GNTMAP_contains_pte))) ) >> + (op->flags & (GNTMAP_application_map|GNTMAP_contains_pte))) ) >> { >> - gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n"); >> + gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n"); >> + op->status = GNTST_general_error; >> + return; >> + } >> + >> + if ( paging_mode_translate(ld) && unlikely(op->flags & GNTMAP_device_map) ) > > There is at least one instance in Linux where GNTMAP_device_map may be > given regardless the type of the guest. See dmabuf_exp_from_refs() in > drivers/xen/gntdev-dmabuf.c. > > How are you going to deal with them? I didn't spot that case, nor ... >> @@ -1213,7 +1219,8 @@ map_grant_ref( >> if ( need_iommu ) >> double_gt_unlock(lgt, rgt); >> >> - op->dev_bus_addr = mfn_to_maddr(mfn); >> + op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr >> + : mfn_to_maddr(mfn); > > The "host_addr" is pretty confusing. I first thought it would be a "Host > Physical Address" but it seems to be a "Guest Physical address" > > If so, this is going to break Linux Dom0 on Arm where it is expected to > return the machine physical address to have a DMA fully working. ... this. > I don't abide with the current behavior on Arm, but I don't think we > should break them when there are no replacement for it. I agree; I didn't mean to break anything here. The state of things then means that I need to withdraw this patch, ... > So it would be better if we look at a different approach (i.e a new flag > or strict mode) in order to avoid breakage. ... without any replacement (at least for the time being). Jan
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -967,10 +967,16 @@ map_grant_ref( } if ( unlikely(paging_mode_external(ld) && - (op->flags & (GNTMAP_device_map|GNTMAP_application_map| - GNTMAP_contains_pte))) ) + (op->flags & (GNTMAP_application_map|GNTMAP_contains_pte))) ) { - gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n"); + gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n"); + op->status = GNTST_general_error; + return; + } + + if ( paging_mode_translate(ld) && unlikely(op->flags & GNTMAP_device_map) ) + { + gdprintk(XENLOG_INFO, "No device mapping in translated domain\n"); op->status = GNTST_general_error; return; } @@ -1213,7 +1219,8 @@ map_grant_ref( if ( need_iommu ) double_gt_unlock(lgt, rgt); - op->dev_bus_addr = mfn_to_maddr(mfn); + op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr + : mfn_to_maddr(mfn); op->handle = handle; op->status = GNTST_okay; @@ -1394,7 +1401,7 @@ unmap_common( op->mfn = act->mfn; - if ( op->dev_bus_addr && + if ( op->dev_bus_addr && (flags & GNTMAP_device_map) && unlikely(op->dev_bus_addr != mfn_to_maddr(act->mfn)) ) PIN_FAIL(act_release_out, GNTST_general_error, "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
Translated domains shouldn't see host physical addresses. While the address is also not supposed to be handed back even to non-translated domains when GNTMAP_device_map is not set (as explicitly stated by a comment in the public header), PV kernels (Linux at least) assume the field to get populated nevertheless. (Similarly mapkind() should check only GNTMAP_device_map.) Along these lines split the paging mode related check near the top of map_grant_ref() to handle the "external" and "translated" cases separately (GNTMAP_device_map use getting tied to being non-translated rather than non-external). Still along these lines in the unmapping case there's no point checking ->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field isn't going to be consumed). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v4: Re-base over dropped patches. v3: New.