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

Re: [Xen-devel] [PATCH v2 21/25] arm/altp2m: Add HVMOP_altp2m_change_gfn.



Hi Julien,


On 08/04/2016 04:04 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> This commit adds the functionality to change mfn mappings for specified
>> gfn's in altp2m views. This mechanism can be used within the context of
>> VMI, e.g., to establish stealthy debugging.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>>  xen/arch/arm/altp2m.c        | 116
>> +++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/hvm.c           |   7 ++-
>>  xen/arch/arm/p2m.c           |  14 ++++++
>>  xen/include/asm-arm/altp2m.h |   6 +++
>>  xen/include/asm-arm/p2m.h    |   4 ++
>>  5 files changed, 146 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index 78fc1d5..db86c14 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -294,6 +294,122 @@ out:
>>      altp2m_unlock(d);
>>  }
>>
>> +int altp2m_change_gfn(struct domain *d,
>> +                      unsigned int idx,
>> +                      gfn_t old_gfn,
>> +                      gfn_t new_gfn)
>> +{
>> +    struct p2m_domain *hp2m, *ap2m;
>> +    paddr_t old_gpa = pfn_to_paddr(gfn_x(old_gfn));
>> +    mfn_t mfn;
>> +    xenmem_access_t xma;
>> +    p2m_type_t p2mt;
>> +    unsigned int level;
>> +    int rc = -EINVAL;
>> +
>> +    static const p2m_access_t memaccess[] = {
>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>> +        ACCESS(n),
>> +        ACCESS(r),
>> +        ACCESS(w),
>> +        ACCESS(rw),
>> +        ACCESS(x),
>> +        ACCESS(rx),
>> +        ACCESS(wx),
>> +        ACCESS(rwx),
>> +        ACCESS(rx2rw),
>> +        ACCESS(n2rwx),
>> +#undef ACCESS
>> +    };
>> +
>> +    if ( idx >= MAX_ALTP2M || d->arch.altp2m_vttbr[idx] ==
>> INVALID_VTTBR )
>
> The second check is not safe. Another operation could destroy the
> altp2m at the same time, but because of memory ordering this thread
> may still see altp2m_vttbr as valid.
>

Ok, I will move the alp2m_lock further up right before the check. Thank you.

>> +        return rc;
>> +
>> +    hp2m = p2m_get_hostp2m(d);
>> +    ap2m = d->arch.altp2m_p2m[idx];
>> +
>> +    altp2m_lock(d);
>> +
>> +    /*
>> +     * Flip mem_access_enabled to true when a permission is set, as
>> to prevent
>> +     * allocating or inserting super-pages.
>> +     */
>> +    ap2m->mem_access_enabled = true;
>
> Can you give more details about why you need this?
>

Similar to altp2m_set_mem_access, if we remap a page that is part of a
super page in the hostp2m, we first map the superpage in form of 512
pages into the ap2m and then change only one page. So, we set
mem_access_enabled to true to shatter the superpage on the ap2m side.

>> +
>> +    mfn = p2m_lookup_attr(ap2m, old_gfn, &p2mt, &level, NULL, NULL);
>> +
>> +    /* Check whether the page needs to be reset. */
>> +    if ( gfn_eq(new_gfn, INVALID_GFN) )
>> +    {
>> +        /* If mfn is mapped by old_gpa, remove old_gpa from the
>> altp2m table. */
>> +        if ( !mfn_eq(mfn, INVALID_MFN) )
>> +        {
>> +            rc = remove_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)), level);
>
> remove_altp2m_entry should take a gfn and mfn in parameter and not an
> address. The latter is a call for misusage of the API.
>

Ok. This will also remove the need for level_sizes/level_masks in the
associated function.

>> +            if ( rc )
>> +            {
>> +                rc = -EINVAL;
>> +                goto out;
>> +            }
>> +        }
>> +
>> +        rc = 0;
>> +        goto out;
>> +    }
>> +
>> +    /* Check host p2m if no valid entry in altp2m present. */
>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>> +    {
>> +        mfn = p2m_lookup_attr(hp2m, old_gfn, &p2mt, &level, NULL,
>> &xma);
>> +        if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>
> Please add a comment to explain why the second check.
>

Ok, I will. It has the same reason as in patch #19: It is not sufficient
so simply check for invalid MFN's as the type might be invalid. Also,
the x86 implementation did not allow to remap a gfn to a shared page.

>> +        {
>> +            rc = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        /* If this is a superpage, copy that first. */
>> +        if ( level != 3 )
>> +        {
>> +            rc = modify_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)),
>> +                                     level, p2mt, memaccess[xma]);
>> +            if ( rc )
>> +            {
>> +                rc = -EINVAL;
>> +                goto out;
>> +            }
>> +        }
>> +    }
>> +
>> +    mfn = p2m_lookup_attr(ap2m, new_gfn, &p2mt, &level, NULL, &xma);
>> +
>> +    /* If new_gfn is not part of altp2m, get the mapping information
>> from hp2m */
>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>> +        mfn = p2m_lookup_attr(hp2m, new_gfn, &p2mt, &level, NULL,
>> &xma);
>> +
>> +    if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>
> Please add a comment to explain why the second check.
>

Same reason as above.

>> +    {
>> +        rc = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    /* Set mem access attributes - currently supporting only one
>> (4K) page. */
>> +    level = 3;
>> +    rc = modify_altp2m_entry(d, ap2m, old_gpa,
>> pfn_to_paddr(mfn_x(mfn)),
>
> modify_altp2m_entry should take a gfn and mfn in parameter and not an
> address. The latter is a call for misusage of the API.
>

Ok.

>> +                             level, p2mt, memaccess[xma]);
>> +    if ( rc )
>> +    {
>> +        rc = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    rc = 0;
>> +
>> +out:
>> +    altp2m_unlock(d);
>> +
>> +    return rc;
>> +}
>> +
>> +
>>  static void altp2m_vcpu_reset(struct vcpu *v)
>>  {
>>      struct altp2mvcpu *av = &vcpu_altp2m(v);
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 00a244a..38b32de 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -142,7 +142,12 @@ static int
>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>
>>      case HVMOP_altp2m_change_gfn:
>> -        rc = -EOPNOTSUPP;
>> +        if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>> +            rc = -EINVAL;
>> +        else
>> +            rc = altp2m_change_gfn(d, a.u.change_gfn.view,
>> +                                   _gfn(a.u.change_gfn.old_gfn),
>> +                                   _gfn(a.u.change_gfn.new_gfn));
>>          break;
>>      }
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index bee8be7..2f4751b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1321,6 +1321,20 @@ void guest_physmap_remove_page(struct domain *d,
>>      p2m_remove_mapping(d, p2m_get_hostp2m(d), gfn, (1 <<
>> page_order), mfn);
>>  }
>>
>> +int remove_altp2m_entry(struct domain *d, struct p2m_domain *ap2m,
>> +                        paddr_t gpa, paddr_t maddr, unsigned int level)
>
> The interface should take mfn_t and gfn_t in parameter and not address.
>

Ok.

>> +{
>> +    paddr_t size = level_sizes[level];
>> +    paddr_t mask = level_masks[level];
>> +    gfn_t gfn = _gfn(paddr_to_pfn(gpa & mask));
>> +    mfn_t mfn = _mfn(paddr_to_pfn(maddr & mask));
>> +    unsigned long nr = paddr_to_pfn(size);
>> +
>> +    ASSERT(p2m_is_altp2m(ap2m));
>> +
>> +    return p2m_remove_mapping(d, ap2m, gfn, nr, mfn);
>> +}
>> +
>>  int modify_altp2m_entry(struct domain *d, struct p2m_domain *ap2m,
>>                          paddr_t gpa, paddr_t maddr, unsigned int level,
>>                          p2m_type_t t, p2m_access_t a)
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index 8bfbc6a..64fbff7 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -99,4 +99,10 @@ void altp2m_propagate_change(struct domain *d,
>>                               p2m_type_t p2mt,
>>                               p2m_access_t p2ma);
>>
>> +/* Change a gfn->mfn mapping */
>> +int altp2m_change_gfn(struct domain *d,
>> +                      unsigned int idx,
>> +                      gfn_t old_gfn,
>> +                      gfn_t new_gfn);
>> +
>>  #endif /* __ASM_ARM_ALTP2M_H */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 16e33ca..8433d66 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -182,6 +182,10 @@ mfn_t p2m_lookup_attr(struct p2m_domain *p2m,
>> gfn_t gfn, p2m_type_t *t,
>>                        unsigned int *level, unsigned int *mattr,
>>                        xenmem_access_t *xma);
>>
>> +/* Remove an altp2m view's entry. */
>> +int remove_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>> +                        paddr_t gpa, paddr_t maddr, unsigned int
>> level);
>> +
>>  /* Modify an altp2m view's entry or its attributes. */
>>  int modify_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>>                          paddr_t gpa, paddr_t maddr, unsigned int level,
>>
>

Best regards,
~Sergej


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

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