On 08/25/2017 10:46 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Roger Pau Monne
>> Sent: 25 August 2017 10:32
>> To: Paul Durrant <paul.durr...@citrix.com>
>> Cc: xen-de...@lists.xenproject.org; Stefano Stabellini
>> <sstabell...@kernel.org>; Wei Liu <wei.l...@citrix.com>; George Dunlap
>> <george.dun...@citrix.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
>> (Xen.org) <t...@xen.org>; Jan Beulich <jbeul...@suse.com>
>> Subject: Re: [Xen-devel] [PATCH v2 REPOST 12/12] x86/hvm/ioreq: add a
>> new mappable resource type...
>>
>> On Tue, Aug 22, 2017 at 03:51:06PM +0100, Paul Durrant wrote:
>>> ... XENMEM_resource_ioreq_server
>>>
>>> This patch adds support for a new resource type that can be mapped using
>>> the XENMEM_acquire_resource memory op.
>>>
>>> If an emulator makes use of this resource type then, instead of mapping
>>> gfns, the IOREQ server will allocate pages from the heap. These pages
>>> will never be present in the P2M of the guest at any point and so are
>>> not vulnerable to any direct attack by the guest. They are only ever
>>> accessible by Xen and any domain that has mapping privilege over the
>>> guest (which may or may not be limited to the domain running the
>> emulator).
>>>
>>> NOTE: Use of the new resource type is not compatible with use of
>>> XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns
>> flag is
>>> set.
>>>
>>> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
>>> ---
>>> Cc: Jan Beulich <jbeul...@suse.com>
>>> Cc: Andrew Cooper <andrew.coop...@citrix.com>
>>> Cc: George Dunlap <george.dun...@eu.citrix.com>
>>> Cc: Ian Jackson <ian.jack...@eu.citrix.com>
>>> Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
>>> Cc: Stefano Stabellini <sstabell...@kernel.org>
>>> Cc: Tim Deegan <t...@xen.org>
>>> Cc: Wei Liu <wei.l...@citrix.com>
>>> ---
>>> xen/arch/x86/hvm/ioreq.c | 136
>> ++++++++++++++++++++++++++++++++++++++++
>>> xen/arch/x86/mm.c | 27 ++++++++
>>> xen/include/asm-x86/hvm/ioreq.h | 2 +
>>> xen/include/public/hvm/dm_op.h | 4 ++
>>> xen/include/public/memory.h | 3 +
>>> 5 files changed, 172 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>>> index 795c198f95..9e6838dab6 100644
>>> --- a/xen/arch/x86/hvm/ioreq.c
>>> +++ b/xen/arch/x86/hvm/ioreq.c
>>> @@ -231,6 +231,15 @@ static int hvm_map_ioreq_gfn(struct
>> hvm_ioreq_server *s, bool buf)
>>> struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
>>> int rc;
>>>
>>> + if ( iorp->page )
>>> + {
>>> + /* Make sure the page has not been allocated */
>>> + if ( gfn_eq(iorp->gfn, INVALID_GFN) )
>>> + return -EPERM;
>>> +
>>> + return 0;
>>
>> EEXIST? (See comment below, which I think also applies here).
>>
>>> + }
>>> +
>>> if ( d->is_dying )
>>> return -EINVAL;
>>>
>>> @@ -253,6 +262,60 @@ static int hvm_map_ioreq_gfn(struct
>> hvm_ioreq_server *s, bool buf)
>>> return rc;
>>> }
>>>
>>> +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
>>> +{
>>> + struct domain *currd = current->domain;
>>> + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
>>> +
>>> + if ( iorp->page )
>>> + {
>>> + /* Make sure the page has not been mapped */
>>> + if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
>>> + return -EPERM;
>>> +
>>> + return 0;
>>
>> Shouldn't this return EEXIST? Page has already been allocated by a
>> previous call AFAICT, and it seems like a possible error/misbehavior
>> to try to do it twice.
>>
>
> The checks are there to prevent a caller from trying to mix the legacy and
> new methods of mapping ioreq server pages so EPERM (i.e. 'operation not
> permitted') seems like the correct error. I agree that it's not obvious, at
> this inner level, that I do think this is right. I'm open to debate about
> this though.
-EBUSY then?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
