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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 09, 2025 at 03:57:24PM -0800, Stefano Stabellini wrote:
> 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 :-)

So unless the proposed adjustment is considered better for code
readability patch 1 can be dropped, and patch 2 could be applied after
the ECLAIR change is in effect?

How long will it take Nicola to get the ECLAIR change propagated into
the Gitlab runner?

Thanks, Roger.