Re: [PATCH v7 04/10] xen/memory: Add a vmtrace_buf resource type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 17:31, Jan Beulich wrote:
> On 21.01.2021 22:27, Andrew Cooper wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1068,11 +1068,35 @@ static unsigned int resource_max_frames(const struct 
>> domain *d,
>>      case XENMEM_resource_grant_table:
>>          return gnttab_resource_max_frames(d, id);
>>  
>> +    case XENMEM_resource_vmtrace_buf:
>> +        return d->vmtrace_frames;
>> +
>>      default:
>>          return arch_resource_max_frames(d, type, id);
>>      }
>>  }
>>  
>> +static int acquire_vmtrace_buf(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    const struct vcpu *v = domain_vcpu(d, id);
>> +    unsigned int i;
>> +    mfn_t mfn;
>> +
>> +    if ( !v || !v->vmtrace.buf ||
>> +         nr_frames > d->vmtrace_frames ||
>> +         (frame + nr_frames) > d->vmtrace_frames )
>> +        return -EINVAL;
> 
> 
> I think that for this to guard against overflow, the first nr_frames
> needs to be replaced by frame (as having the wider type), or else a
> very large value of frame coming in will not yield the intended
> -EINVAL.

Actually, besides this then wanting to be >= instead of >, this
wouldn't take care of the 32-bit case (or more generally the
sizeof(long) == sizeof(int) one). So I think you want

    if ( !v || !v->vmtrace.buf ||
         (frame + nr_frames) < frame ||
         (frame + nr_frames) > d->vmtrace_frames )
        return -EINVAL;

> If you agree, with this changed,
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

This holds.

Jan