Re: [PATCH v2 1/2] x86/uaccess: rework user access speculative harden guards

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 9 Jan 2025, Nicola Vetrini wrote:
> On 2025-01-04 01:20, Stefano Stabellini wrote:
> > Hi Nicola, one question below
> > 
> > On Wed, 27 Nov 2024, Nicola Vetrini wrote:
> > > > #define AMD_OSVW_ERRATUM(osvw_id, ...)  osvw_id, __VA_ARGS__, 0
> > > >
> > > > where we're using the C99 form rather than the GNU extension, and where
> > > > hence __VA_ARGS__ would - by extrapolation of the Misra rule - need
> > > > parenthesizing, when it isn't and can't be.
> > > >
> > > > Isn't it rather the case that variable argument macros need a more
> > > general
> > > > deviation, if not an adjustment to the Misra rule? Extending the Cc list
> > > > some ...
> > 
> > Nicola, if you look at the original patch:
> > https://marc.info/?l=xen-devel&m=173261356716876
> > 
> > "The current guards to select whether user accesses should be speculative
> > hardened violate Misra rule 20.7, as the UA_KEEP() macro doesn't (and can't)
> > parenthesize the 'args' argument."
> > 
> > And the very first change in the patch is:
> > 
> > diff --git a/xen/arch/x86/include/asm/uaccess.h
> > b/xen/arch/x86/include/asm/uaccess.h
> > index 2d01669b96..6b8150ac22 100644
> > --- a/xen/arch/x86/include/asm/uaccess.h
> > +++ b/xen/arch/x86/include/asm/uaccess.h
> > @@ -24,9 +24,6 @@ unsigned int copy_from_unsafe_ll(void *to, const void
> > *from, unsigned int n);
> >  void noreturn __get_user_bad(void);
> >  void noreturn __put_user_bad(void);
> > 
> > -#define UA_KEEP(args...) args
> > -#define UA_DROP(args...)
> > -
> >  /**
> >   * get_guest: - Get a simple variable from guest space.
> >   * @x:   Variable to store result.
> > 
> > 
> > Do you think there is any way we could configure Eclair, with or without
> > a deviation, not to detect every use of UA_KEEP as violations?
> 
> I narrowed this violation down to a different treatment of the named variadic
> argument. Since the argument 'args' cannot be parenthesized as a regular
> argument could, the invocations of the 'UA_KEEP' cannot comply with the rule.
> Therefore, as an extension to the rule, ECLAIR currently ignores the use of
> '__VA_ARGS__' in a macro definition, but treats 'args...' as a regular macro
> parameter name, hence the violation.
> 
> To be clear, these two definitions have the same semantics, but one shows a
> violation and the other doesn't
> 
> #define UA_KEEP(args...) args
> #define UA_KEEP(...) __VA_ARGS__
> 
> I will update ECLAIR to treat the two forms as the same, so this patch can be
> dropped. If you think it's helpful I can send a patch spelling out this -
> arbitrary, but reasonable in my opinion - extension to the MISRA rule (which
> does not consider the implications related to the use of GNU exensions) so
> that contributors have a clear picture of the situation.

Thank you Nicola! Yes the patch would be appreciated :-)