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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2025 00:02, Stefano Stabellini wrote:
> On Tue, 29 Apr 2025, Jan Beulich wrote:
>> 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.
> 
> Please do, that would be much appreciated. Thank you!

I already did ["{hyper,multi}call: further limit arguments to just 5"]; all I
need there is an Arm side ack.

> Also thanks for 2f531c122e95.

I fear I don't understand this part. But it probably also doesn't matter much.

Jan