Re: [PATCH V6 2/2] xen/arm: Harden the P2M code in p2m_remove_mapping()

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH V6 2/2] xen/arm: Harden the P2M code in p2m_remove_mapping()
Date: Fri, 24 Jun 2022 18:31:48 +0300	[thread overview]
Message-ID: <94afe35c-627c-8aba-37ce-1d017a2e4e3c@gmail.com> (raw)
In-Reply-To: <42b0d343-a491-877c-3b5c-d9c95872774c@xen.org>

On 23.06.22 21:08, Julien Grall wrote:
> Hi Oleksandr,

Hello Julien

>
> On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Borrow the x86's check from p2m_remove_page() which was added
>> by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e
>> "x86/p2m: don't assert that the passed in MFN matches for a remove"
>> and adjust it to the Arm code base.
>>
>> Basically, this check is strictly needed for the xenheap pages only
>> since there are several non-protected read accesses to our simplified
>> xenheap based M2P approach on Arm (most calls to page_get_xenheap_gfn()
>> are not protected by the P2M lock).
>
> To me, this read as you introduced a bug in patch #1 and now you are 
> fixing it. So this patch should have been first.

Sounds like yes, I agree. But, in that case I propose to rewrite this 
text like the following:

Basically, this check will be strictly needed for the xenheap pages only 
*and* only after applying subsequent
commit which will introduce xenheap based M2P approach on Arm. But, it 
will be a good opportunity
to harden the P2M code for *every* RAM pages since it is possible to 
remove any GFN - MFN mapping
currently on Arm (even with the wrong helpers).

And ...

>
>>
>> But, it will be a good opportunity to harden the P2M code for *every*
>> RAM pages since it is possible to remove any GFN - MFN mapping
>> currently on Arm (even with the wrong helpers).
>
>> This can result in
>> a few issues when mapping is overridden silently (in particular when
>> building dom0).
>
> Hmmm... AFAIU, in such situation p2m_remove_mapping() wouldn't be 
> called. Instead, we would call the mapping helper twice and the 
> override would still happen.

    ... drop this one.

>
>
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> You can find the corresponding discussion at:
>> https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xen.org/ 
>>
>>
>> Changes V5 -> V6:
>>   - new patch
>> ---
>>   xen/arch/arm/p2m.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f87b48e..635e474 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1311,11 +1311,32 @@ static inline int p2m_remove_mapping(struct 
>> domain *d,
>>                                        mfn_t mfn)
>>   {
>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    unsigned long i;
>>       int rc;
>>         p2m_write_lock(p2m);
>> +    for ( i = 0; i < nr; )
> One bit I really hate in the x86 code is the lack of in-code 
> documentation. It makes really difficult to understand the logic.
>
> I know this code was taken from x86, but I would like to avoid making 
> same mistake (this code is definitely not trivial). So can we document 
> the logic?

ok, I propose the following text right after acquiring the p2m lock:

  /*
   * Before removing the GFN - MFN mapping for any RAM pages make sure
   * that there is no difference between what is already mapped and what
   * is requested to be unmapped. If passed mapping doesn't match
   * the existing one bail out early.
   */

Could you please clarify, do you agree with both?

>
> The code itself looks good to me.

Thanks!

>
>> +    {
>> +        unsigned int cur_order;
>> +        p2m_type_t t;
>> +        mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i), 
>> &t, NULL,
>> +                                         &cur_order, NULL);
>> +
>> +        if ( p2m_is_any_ram(t) &&
>> +             (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), 
>> mfn_return)) )
>> +        {
>> +            rc = -EILSEQ;
>> +            goto out;
>> +        }
>> +
>> +        i += (1UL << cur_order) -
>> +             ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
>> +    }
>> +
>>       rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
>>                          p2m_invalid, p2m_access_rwx);
>> +
>> +out:
>>       p2m_write_unlock(p2m);
>>         return rc;
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko