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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/01/2021 07:37, Jan Beulich wrote:
> 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.

I slipped this buggy version in to prove a point.

You're now 3 or 4 attempts into "simplifying" my original version, and
have on at least 2 attempts made your R-b conditional on a buggy version.

This form is clearly too complicated to reason about correctly, and it
is definitely more complicated than I am happy taking.


I am either going to go with my original version, which is trivially and
obviously correct, or I'm considering reducing frame to 32 bits at the
top level to fix this width nonsense throughout Xen.

~Andrew