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.l...@amd.com wrote:
>>>>> From: Nicola Vetrini <nicola.vetr...@bugseng.com>
>>>>>
>>>>> 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
