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