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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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) )
> 
> 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

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