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.
Jan
