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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


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