[v4] gnttab: don't expose host physical address without need
diff mbox series

Message ID ad758354-b8e7-f5ef-b3cf-94a6a0d92bd4@suse.com
State New
Headers show
Series
  • [v4] gnttab: don't expose host physical address without need
Related show

Commit Message

Jan Beulich Dec. 5, 2019, 3:34 p.m. UTC
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.

Comments

Andrew Cooper Dec. 5, 2019, 3:47 p.m. UTC | #1
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",
Andrew Cooper Dec. 5, 2019, 3:53 p.m. UTC | #2
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
Jan Beulich Dec. 5, 2019, 4:57 p.m. UTC | #3
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
Andrew Cooper Dec. 6, 2019, 7:48 p.m. UTC | #4
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
Jan Beulich Dec. 9, 2019, 11:51 a.m. UTC | #5
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
Julien Grall Dec. 16, 2019, 2:19 p.m. UTC | #6
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,
Jan Beulich Dec. 16, 2019, 3:59 p.m. UTC | #7
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

Patch
diff mbox series

--- 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",