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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?)

Jan