[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas


  • To: Julien Grall <julien@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Tue, 17 Jan 2023 22:20:48 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=t4EILZT52ZKSfqjd+zyNYWSgc1N/Ke0NH5zkAWZgzo0=; b=NIOLTeDKIejYBhGhnTCY4TlC+k/XlnztYtUL7QfMTt9kgpB197p8qArwQZnfkF6HFSHs+Tpk/rssn/GLweWUEPZ+2TwxylpWn1O39e3bzR22TVFgYjtKieUhP7vzwRShNuYiVp8j02g2Rexh1uvO0uNSyEY3S9Yff8vTlroFSPfj7v57lrUmPPbbK3ErjYAomiCa2uvD2FvttavnYVcRtR5vHTnITXwn16W5cXrISPk7LPDNteNOu7kLJjQFgqPS2eUrd70zFx/V7kh9B/Icrktl90NTwNnoyRekCnky+4Muk++JCdNwuqDPJAfYZF43xk96yGypFnlpEFG66IjYtQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jCvanKGFTPEVIHcQw+DVEPSyhsi6rUVxricNj7iwTwVoUtqfShL1qERZodR3PbQOrjSrQhTgLvhIgz0oZ+PiF1Py+0Oa+qLcUujlG/e6npMSbtk4EQjfa7BPX/oRoi7WzmbARWEh8nY1LtljzZLeUJwEZzBl9H0vRUWmbzYsyj9GNuT0QXDENh0g0VWwHtAeAzqmy7qb4K03MI0Ibt5lVTUqIfmj7vdGgxKRmEyr6Hn5Z7wxnP5l4+VqfO2cYYzZoeQbxEQBrL7MHELxatt63veHciQA/Ju6J9MEvo4d+rg9Tqy+x8zJD/+ODtEJbY/OOHExdLJ5XNWiCQxxr8un/w==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 17 Jan 2023 22:21:19 +0000
  • Ironport-data: A9a23:9/VhV6+UBL3CPxrD4rkkDrUDSn+TJUtcMsCJ2f8bNWPcYEJGY0x3n WAfCDzVMv3bajOjKox3O9++8BhX6MfQxoMwHQdsqis8E34SpcT7XtnIdU2Y0wF+jCHgZBk+s 5hBMImowOQcFCK0SsKFa+C5xZVE/fjUAOG6UKucYHsZqTZMEE8JkQhkl/MynrlmiN24BxLlk d7pqojUNUTNNwRcawr40Ire7kIw1BjOkGlA5AdmPKkU5AW2e0Q9V/rzG4ngdxMUfaEMdgKKb 76r5K20+Grf4yAsBruN+losWhRXKlJ6FVHmZkt+A8BOsDAbzsAB+v9T2M4nQVVWk120c+VZk 72hg3ASpTABZcUgkMxFO/VR/roX0aduoNcrKlDn2SCfItGvn9IBDJyCAWlvVbD09NqbDkllq NMqCmEGRCmBmqWL56zkcdZHtsA8eZyD0IM34hmMzBn/JNN/G9XpZfWP4tVVmjAtmspJAPDSI dIDbiZiZwjBZBsJPUoLDJU5n6GjgXyXnz9w8QrJ4/ZopTWOilUui9ABM/KMEjCObexTklyVu STt+GPhDwtBHNee1SCE4jSngeqncSbTCdhPTuHkpq8CbFu7mn0UGD0pb16BoqO3qmKedcACF Ew6w397xUQ13AnxJjXnZDW6vXqFsxg0S9dWVeog52mlyKDZ/gKYDWgsVSNaZZots8pebSwn0 BqFks3kARRrsaaJUjSN+7GMtzSwNCMJa2gYakcsVhAZ6tPupIUyiBPnTdt5FqOxyNrvFlnY3 DSivCU4wbIJgqY2O76T+FnGh3emoMjPRwtsvAHPBDv6tUV+eZKvYJGu5R7D9/FcIY2FT16H+ n8Zh8yZ6+NIBpaI/MCQfNgw8HiSz67tGFXhbZRHRvHNKxzFF6afQL1t
  • Ironport-hdrordr: A9a23:R/VzV62jYzO2bIahbCQ5UwqjBLwkLtp133Aq2lEZdPU1SKClfq WV98jzuiWatN98Yh8dcLK7WJVoMEm8yXcd2+B4V9qftWLdyQiVxe9ZnO7f6gylNyri9vNMkY dMGpIObOEY1GIK7/rH3A==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHY4457jyaOcvAnhkOdTNwMDM8hXq5O0KiAgFTsIQA=
  • Thread-topic: [PATCH RFC 07/10] domain: map/unmap GADDR based shared guest areas

On 24/11/2022 9:29 pm, Julien Grall wrote:
> Hi Jan,
>
> I am still digesting this series and replying with some initial comments.
>
> On 19/10/2022 09:43, Jan Beulich wrote:
>> The registration by virtual/linear address has downsides: At least on
>> x86 the access is expensive for HVM/PVH domains. Furthermore for 64-bit
>> PV domains the areas are inaccessible (and hence cannot be updated by
>> Xen) when in guest-user mode.
>>
>> In preparation of the introduction of new vCPU operations allowing to
>> register the respective areas (one of the two is x86-specific) by
>> guest-physical address, flesh out the map/unmap functions.
>>
>> Noteworthy differences from map_vcpu_info():
>> - areas can be registered more than once (and de-registered),
>> - remote vCPU-s are paused rather than checked for being down (which in
>>    principle can change right after the check),
>> - the domain lock is taken for a much smaller region.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> RFC: By using global domain page mappings the demand on the underlying
>>       VA range may increase significantly. I did consider to use per-
>>       domain mappings instead, but they exist for x86 only. Of course we
>>       could have arch_{,un}map_guest_area() aliasing global domain page
>>       mapping functions on Arm and using per-domain mappings on x86. Yet
>>       then again map_vcpu_info() doesn't do so either (albeit that's
>>       likely to be converted subsequently to use map_vcpu_area()
>> anyway).
>>
>> RFC: In map_guest_area() I'm not checking the P2M type, instead - just
>>       like map_vcpu_info() - solely relying on the type ref acquisition.
>>       Checking for p2m_ram_rw alone would be wrong, as at least
>>       p2m_ram_logdirty ought to also be okay to use here (and in similar
>>       cases, e.g. in Argo's find_ring_mfn()). p2m_is_pageable() could be
>>       used here (like altp2m_vcpu_enable_ve() does) as well as in
>>       map_vcpu_info(), yet then again the P2M type is stale by the time
>>       it is being looked at anyway without the P2M lock held.
>>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1563,7 +1563,82 @@ int map_guest_area(struct vcpu *v, paddr
>>                      struct guest_area *area,
>>                      void (*populate)(void *dst, struct vcpu *v))
>>   {
>> -    return -EOPNOTSUPP;
>> +    struct domain *currd = v->domain;
>> +    void *map = NULL;
>> +    struct page_info *pg = NULL;
>> +    int rc = 0;
>> +
>> +    if ( gaddr )
>
> 0 is technically a valid (guest) physical address on Arm.

It is on x86 too, but that's not why 0 is generally considered an
invalid address.

See the multitude of XSAs, and near-XSAs which have been caused by bad
logic in Xen caused by trying to make a variable held in struct
vcpu/domain have a default value other than 0.

It's not impossible to write such code safely, and in this case I expect
it can be done by the NULLness (or not) of the mapping pointer, rather
than by stashing the gaddr, but history has proved repeatedly that this
is a very fertile source of security bugs.

~Andrew

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.