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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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! Also thanks for
2f531c122e95.