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

[Oleksandr <olekstysh@xxxxxxxxx>]

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@xxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> > You can find the corresponding discussion at:
> > https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xxxxxxx/ 
> >
> > 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