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

[Xen-devel] Ping: [PATCH] x86/memshr: properly check grant references



>>> On 14.11.16 at 11:34, <JBeulich@xxxxxxxx> wrote:
> They need to be range checked against the current table limit in any
> event.
> 
> Reported-by: Huawei PSIRT <psirt@xxxxxxxxxx>
> 
> Move the code to where it belongs, eliminating a number of duplicate
> definitions. Add locking. Produce proper error codes, and consume them
> instead of making one up. Check grant type. Convert parameter types at
> once.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Tamas? (The minor fix needed to address Andrew's reply doesn't seem
to warrant sending out a v2.)

> ---
> Note that likely there's more work needed subsequently: The grant isn't
> being marked in use for the duration of the use of the GFN. But I'll
> leave that to someone actually knowing how to properly to test this.
> 
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -753,75 +753,25 @@ int mem_sharing_debug_gfn(struct domain
>      return num_refs;
>  }
>  
> -#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
> -#define shared_entry_v1(t, e) \
> -    ((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
> -#define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
> -#define shared_entry_v2(t, e) \
> -    ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
> -#define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
> -#define status_entry(t, e) \
> -    ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
> -
> -static grant_entry_header_t *
> -shared_entry_header(struct grant_table *t, grant_ref_t ref)
> -{
> -    ASSERT (t->gt_version != 0);
> -    if ( t->gt_version == 1 )
> -        return (grant_entry_header_t*)&shared_entry_v1(t, ref);
> -    else
> -        return &shared_entry_v2(t, ref).hdr;
> -}
> -
> -static int mem_sharing_gref_to_gfn(struct domain *d, 
> -                                   grant_ref_t ref, 
> -                                   unsigned long *gfn)
> -{
> -    if ( d->grant_table->gt_version < 1 )
> -        return -1;
> -
> -    if ( d->grant_table->gt_version == 1 ) 
> -    {
> -        grant_entry_v1_t *sha1;
> -        sha1 = &shared_entry_v1(d->grant_table, ref);
> -        *gfn = sha1->frame;
> -    } 
> -    else 
> -    {
> -        grant_entry_v2_t *sha2;
> -        sha2 = &shared_entry_v2(d->grant_table, ref);
> -        *gfn = sha2->full_page.frame;
> -    }
> - 
> -    return 0;
> -}
> -
> -
>  int mem_sharing_debug_gref(struct domain *d, grant_ref_t ref)
>  {
> -    grant_entry_header_t *shah;
> +    int rc;
>      uint16_t status;
> -    unsigned long gfn;
> +    gfn_t gfn;
>  
> -    if ( d->grant_table->gt_version < 1 )
> +    rc = mem_sharing_gref_to_gfn(d->grant_table, ref, &gfn, &status);
> +    if ( rc )
>      {
> -        MEM_SHARING_DEBUG( 
> -                "Asked to debug [dom=%d,gref=%d], but not yet inited.\n",
> -                d->domain_id, ref);
> -        return -EINVAL;
> -    }
> -    (void)mem_sharing_gref_to_gfn(d, ref, &gfn); 
> -    shah = shared_entry_header(d->grant_table, ref);
> -    if ( d->grant_table->gt_version == 1 ) 
> -        status = shah->flags;
> -    else 
> -        status = status_entry(d->grant_table, ref);
> +        MEM_SHARING_DEBUG("Asked to debug [dom=%d,gref=%u]: error %d.\n",
> +                          d->domain_id, ref, rc);
> +        return rc;
> +    }
>      
>      MEM_SHARING_DEBUG(
>              "==> Grant [dom=%d,ref=%d], status=%x. ", 
>              d->domain_id, ref, status);
>  
> -    return mem_sharing_debug_gfn(d, gfn); 
> +    return mem_sharing_debug_gfn(d, gfn_x(gfn));
>  }
>  
>  int mem_sharing_nominate_page(struct domain *d,
> @@ -1422,23 +1372,24 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
>          case XENMEM_sharing_op_nominate_gref:
>          {
>              grant_ref_t gref = mso.u.nominate.u.grant_ref;
> -            unsigned long gfn;
> +            gfn_t gfn;
>              shr_handle_t handle;
>  
>              rc = -EINVAL;
>              if ( !mem_sharing_enabled(d) )
>                  goto out;
> -            if ( mem_sharing_gref_to_gfn(d, gref, &gfn) < 0 )
> +            rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &gfn, NULL);
> +            if ( rc < 0 )
>                  goto out;
>  
> -            rc = mem_sharing_nominate_page(d, gfn, 3, &handle);
> +            rc = mem_sharing_nominate_page(d, gfn_x(gfn), 3, &handle);
>              mso.u.nominate.handle = handle;
>          }
>          break;
>  
>          case XENMEM_sharing_op_share:
>          {
> -            unsigned long sgfn, cgfn;
> +            gfn_t sgfn, cgfn;
>              struct domain *cd;
>              shr_handle_t sh, ch;
>  
> @@ -1470,35 +1421,38 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
>                  grant_ref_t gref = (grant_ref_t) 
>                                      (XENMEM_SHARING_OP_FIELD_GET_GREF(
>                                          mso.u.share.source_gfn));
> -                if ( mem_sharing_gref_to_gfn(d, gref, &sgfn) < 0 )
> +                rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &sgfn,
> +                                             NULL);
> +                if ( rc < 0 )
>                  {
>                      rcu_unlock_domain(cd);
> -                    rc = -EINVAL;
>                      goto out;
>                  }
> -            } else {
> -                sgfn  = mso.u.share.source_gfn;
>              }
> +            else
> +                sgfn = _gfn(mso.u.share.source_gfn);
>  
>              if ( XENMEM_SHARING_OP_FIELD_IS_GREF(mso.u.share.client_gfn) )
>              {
>                  grant_ref_t gref = (grant_ref_t) 
>                                      (XENMEM_SHARING_OP_FIELD_GET_GREF(
>                                          mso.u.share.client_gfn));
> -                if ( mem_sharing_gref_to_gfn(cd, gref, &cgfn) < 0 )
> +                rc = mem_sharing_gref_to_gfn(cd->grant_table, gref, &cgfn,
> +                                             NULL);
> +                if ( rc < 0 )
>                  {
>                      rcu_unlock_domain(cd);
> -                    rc = -EINVAL;
>                      goto out;
>                  }
> -            } else {
> -                cgfn  = mso.u.share.client_gfn;
>              }
> +            else
> +                cgfn = _gfn(mso.u.share.client_gfn);
>  
>              sh = mso.u.share.source_handle;
>              ch = mso.u.share.client_handle;
>  
> -            rc = mem_sharing_share_pages(d, sgfn, sh, cd, cgfn, ch); 
> +            rc = mem_sharing_share_pages(d, gfn_x(sgfn), sh,
> +                                         cd, gfn_x(cgfn), ch);
>  
>              rcu_unlock_domain(cd);
>          }
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3438,6 +3438,53 @@ void grant_table_init_vcpu(struct vcpu *
>      v->maptrack_tail = MAPTRACK_TAIL;
>  }
>  
> +#ifdef CONFIG_HAS_MEM_SHARING
> +int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> +                            gfn_t *gfn, uint16_t *status)
> +{
> +    int rc = 0;
> +    uint16_t flags = 0;
> +
> +    grant_read_lock(gt);
> +
> +    if ( gt->gt_version < 1 )
> +        rc = -EINVAL;
> +    else if ( ref >= nr_grant_entries(gt) )
> +        rc = -ENOENT;
> +    else if ( gt->gt_version == 1 )
> +    {
> +        const grant_entry_v1_t *sha1 = &shared_entry_v1(gt, ref);
> +
> +        flags = sha1->flags;
> +        *gfn = _gfn(sha1->frame);
> +    }
> +    else
> +    {
> +        const grant_entry_v2_t *sha2 = &shared_entry_v2(gt, ref);
> +
> +        flags = sha2->hdr.flags;
> +        if ( flags & GTF_sub_page )
> +           *gfn = _gfn(sha2->sub_page.frame);
> +        else
> +           *gfn = _gfn(sha2->full_page.frame);
> +    }
> +
> +    if ( (flags & GTF_type_mask) != GTF_permit_access )
> +        rc = -ENXIO;
> +    else if ( !rc && status )
> +    {
> +        if ( gt->gt_version == 1 )
> +            *status = flags;
> +        else
> +            *status = status_entry(gt, ref);
> +    }
> +
> +    grant_read_unlock(gt);
> +
> +    return rc;
> +}
> +#endif
> +
>  static void gnttab_usage_print(struct domain *rd)
>  {
>      int first = 1;
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -149,4 +149,7 @@ static inline unsigned int grant_to_stat
>          GRANT_STATUS_PER_PAGE;
>  }
>  
> +int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> +                            gfn_t *gfn, uint16_t *status);
> +
>  #endif /* __XEN_GRANT_TABLE_H__ */



_______________________________________________
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®.