Re: [PATCH v5 08/11] x86/mm: add vmtrace_buf resource type

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/07/2020 19:55, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@xxxxxxx>
>
> Allow to map processor trace buffer using
> acquire_resource().
>
> Signed-off-by: Michal Leszczynski <michal.leszczynski@xxxxxxx>
> ---
>   xen/common/memory.c         | 28 ++++++++++++++++++++++++++++
>   xen/include/public/memory.h |  1 +
>   2 files changed, 29 insertions(+)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index eb42f883df..04f4e152c0 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,6 +1007,29 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>       return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>   }
>   
> +static int acquire_vmtrace_buf(struct domain *d, unsigned int id,
> +                               unsigned long frame,
Shouldn't this be uint64_t to avoid truncation?

> +                               unsigned int nr_frames,
> +                               xen_pfn_t mfn_list[])
> +{
> +    mfn_t mfn;
> +    unsigned int i;
> +    struct vcpu *v = domain_vcpu(d, id);
> +
> +    if ( !v || !v->vmtrace.pt_buf )
> +        return -EINVAL;
> +
> +    mfn = page_to_mfn(v->vmtrace.pt_buf);
> +
> +    if ( frame + nr_frames > (v->domain->vmtrace_pt_size >> PAGE_SHIFT) )
frame + nr_frames could possibly overflow a 64-bit value and therefore 
still pass the check.
So I would suggest to use:

(frame > (v->domain_vm_ptrace_pt_size >> PAGE_SHIFT)) ||
(nr_frames > ((v->domain_vm_ptrace_pt_size >> PAGE_SHIFT) - frame))

Cheers,

--
Julien Grall