Re: [PATCH 1/6] xen/trace: Don't over-read trace objects

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/09/2021 09:00, Jan Beulich wrote:
> On 17.09.2021 15:26, Andrew Cooper wrote:
>> On 17/09/2021 13:58, Jan Beulich wrote:
>>> On 17.09.2021 10:45, Andrew Cooper wrote:
>>>> --- a/xen/common/trace.c
>>>> +++ b/xen/common/trace.c
>>>> @@ -686,22 +686,21 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra,
>>>>      unsigned long flags;
>>>>      u32 bytes_to_tail, bytes_to_wrap;
>>>>      unsigned int rec_size, total_size;
>>>> -    unsigned int extra_word;
>>>>      bool_t started_below_highwater;
>>>>  
>>>>      if( !tb_init_done )
>>>>          return;
>>>>  
>>>> -    /* Convert byte count into word count, rounding up */
>>>> -    extra_word = (extra / sizeof(u32));
>>>> -    if ( (extra % sizeof(u32)) != 0 )
>>>> -        extra_word++;
>>>> -    
>>>> -    ASSERT(extra_word <= TRACE_EXTRA_MAX);
>>>> -    extra_word = min_t(int, extra_word, TRACE_EXTRA_MAX);
>>>> -
>>>> -    /* Round size up to nearest word */
>>>> -    extra = extra_word * sizeof(u32);
>>>> +    /*
>>>> +     * Trace records require extra data which is an exact multiple of
>>>> +     * uint32_t.  Reject out-of-spec records.  Any failure here is an error in
>>>> +     * the caller.
>>>> +     */
>>> Hmm, is "require" accurate?
>> In terms of "what will go wrong if this condition is violated", yes.
>>
>>>  They may very well come without extra data
>>> afaics.
>> 0 is fine, and used by plenty of records, and also permitted by the
>> filtering logic.
> I was about to say that the two parts of your reply contradict one
> another, when I finally realized that it looks like the first sentence
> in the comment can be read two ways: "Trace records require extra data"
> then going on to describe properties, or "Trace records require extra
> data to be an exact multiple of uint32_t." Obviously this is to me as a
> non-native speaker. But maybe you could still reword this to be
> unambiguous? (I'm not going to exclude that the lack of a comma, which
> I did silently add while reading, makes a difference here: Does "Trace
> records require extra data, which is an exact multiple of uint32_t" end
> up altering the meaning?)

Yes.  The requirement is for "extra data which is an exact multiple of
uint32_t", not "extra data".

The comma massively changes the meaning.

I'll see about tweaking the wording.

~Andrew