Re: [Xen-devel] [PATCH v4] gnttab: don't expose host physical address without need

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel