Re: [PATCH v1 2/3] compat: address violations of MISRA C Rule 19.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2025 01:21, Stefano Stabellini wrote:
> On Mon, 28 Apr 2025, Jan Beulich wrote:
>> On 26.04.2025 01:42, victorm.lira@xxxxxxx wrote:
>>> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>
>>> Rule 19.1 states: "An object shall not be assigned or copied
>>> to an overlapping object". Since the "call" and "compat_call" are
>>> fields of the same union, reading from one member and writing to
>>> the other violates the rule, while not causing Undefined Behavior
>>> due to their relative sizes. However, a dummy variable is used to
>>> address the violation and prevent the future possibility of
>>> incurring in UB.
>>
>> If there is such a concern, ...
>>
>>> --- a/xen/common/compat/multicall.c
>>> +++ b/xen/common/compat/multicall.c
>>> @@ -15,8 +15,13 @@ typedef int ret_t;
>>>  static inline void xlat_multicall_entry(struct mc_state *mcs)
>>>  {
>>>      int i;
>>> +    xen_ulong_t arg;
>>> +
>>>      for (i=0; i<6; i++)
>>> -        mcs->compat_call.args[i] = mcs->call.args[i];
>>> +    {
>>> +        arg = mcs->call.args[i];
>>> +        mcs->compat_call.args[i] = arg;
>>> +    }
>>>  }
>>
>> ... wouldn't it be of concern as well that the alternating parts of
>> the union are still accessed in a flip-flop manner? IOW we continue to
>> rely on the relative placement properties of the individual array
>> elements. To eliminate such a concern, I think the resulting code would
>> also want to be correct if iteration was swapped to work downwards.
>>
>> Also the scope of the temporary could certainly be the loop body rather
>> than the entire function.
> 
> Wouldn't be safer to do this then?
> 
> static inline void xlat_multicall_entry(struct mc_state *mcs)
> {
>     int i;
>     xen_ulong_t args[6];
> 
>     for ( i = 0; i < 6; i++ )
>     {
>         args[i] = mcs->call.args[i];
>     }
>     for ( i = 0; i < 6; i++ )
>     {
>         mcs->compat_call.args[i] = args[i];
>     }
> }
> 
> If you have any specific suggestions I think C code would be easier to
> understand than English.

Kind of the above, yes, with the further remark below also taken care of.
So ...

>> I also don't think it needs to be xen_ulong_t,
>> but maybe using unsigned int instead wouldn't make a difference in
>> generated code.
> 
> Keeping the same type as mcs->call.args[i] would seem more obviously
> correct? Not to mention that unsigned long is what we defined as
> register type? If we really want to avoid xen_ulong_t, then it should
> be uintptr_t?
> 
> We should stick to one type to be used as register type. On ARM, we
> defined register_t.

... with both taken into account e.g.:

    typeof(mcs->compat_call.args[0]) args[ARRAY_SIZE(mcs->call.args)];

    for ( i = 0; i < ARRAY_SIZE(args); i++ )
        args[i] = mcs->call.args[i];

    memcpy(mcs->compat_call.args, args, sizeof(args));

Of course there are variations possible. There also may want to be a
BUILD_BUG_ON() to "document" both array sizes match, even if the compat
form is auto-generated from the native one.

Tangential: As of 2f531c122e95 ("x86: limit number of hypercall parameters
to 5") it's kind of bogus that we process 6 array elements here. This even
extends to an assertion in hypercall_xlat_continuation() and to some of
the handling in hypercall_create_continuation(). I guess I will want to
make a patch there, which of course I could make cover the Misra aspect
here as well.

Jan