Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.17 at 18:26, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> @@ -4568,6 +4571,32 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>  
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> +                                      a.u.set_mem_access_multi.access_list,
> +                                      a.u.set_mem_access_multi.nr,
> +                                      a.u.set_mem_access_multi.opaque,
> +                                      0x3F,

Please add /* pretty arbitrary */ or something similar here.

> @@ -4586,6 +4615,80 @@ static int do_altp2m_op(
>      return rc;
>  }
>  
> +DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> +
> +static int compat_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    int rc = 0;
> +    struct compat_hvm_altp2m_op a;
> +    union
> +    {
> +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> +        struct xen_hvm_altp2m_op *altp2m_op;
> +    } nat;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return -EOPNOTSUPP;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.pad1 || a.pad2 ||
> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +        return -EINVAL;
> +
> +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +    switch ( a.cmd )
> +    {
> +        case HVMOP_altp2m_set_mem_access_multi:

Indentation.

> +            BUILD_BUG_ON(sizeof(struct xen_hvm_altp2m_set_mem_access_multi) <
> +                         sizeof(struct 
> compat_hvm_altp2m_set_mem_access_multi));

What good does this do?

> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +            
> XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
> +                    &a.u.set_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +            break;
> +        default:
> +            return do_altp2m_op(arg);
> +    }
> +
> +    /*
> +     * Manually fill the common part of the xen_hvm_altp2m_op structure 
> because
> +     * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the
> +     * translation of all fields from compat_hvm_altp2m_op to 
> xen_hvm_altp2m_op.
> +     */
> +    nat.altp2m_op->version  = a.version;
> +    nat.altp2m_op->cmd      = a.cmd;
> +    nat.altp2m_op->domain   = a.domain;
> +    nat.altp2m_op->pad1     = a.pad1;
> +    nat.altp2m_op->pad2     = a.pad2;

There are _still_ no size checks here.

> +    rc = do_altp2m_op(nat.hnd);
> +
> +    switch ( a.cmd )
> +    {
> +        case HVMOP_altp2m_set_mem_access_multi:

Indentation.

> +            if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0 )

Please also check rc here to avoid needlessly copying back. In
fact _only_ checking rc ought to be fine.

> +            {
> +                a.u.set_mem_access_multi.opaque =
> +                    nat.altp2m_op->u.set_mem_access_multi.opaque;

You also need a size check here.

Jan


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