[PATCH v2 0/2] x86/misra: fix remaining violation of rule 20.7

[PATCH v2 0/2] x86/misra: fix remaining violation of rule 20.7

[Roger Pau Monne]

Hello,

This series fixes the remaining violation of rule 20.7, and marks the
rule a blocking for x86 also on the Eclair scan.

An example gitlab job with the rule enabled can be seen at:

https://gitlab.com/xen-project/people/royger/xen/-/jobs/8470641011

Thanks, Roger.

Roger Pau Monne (2):
  x86/uaccess: rework user access speculative harden guards
  automation/eclair: make Misra rule 20.7 blocking for x86 also

 automation/eclair_analysis/ECLAIR/tagging.ecl |  3 +-
 xen/arch/x86/include/asm/uaccess.h            | 45 +++++++++----------
 xen/arch/x86/usercopy.c                       | 26 +++++------
 3 files changed, 37 insertions(+), 37 deletions(-)

-- 
2.46.0

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

[Roger Pau Monne]

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.

Change the logic so the guard is implemented inside the assembly block using
the .if assembly directive.  This results in some register spilling if the
guest_access_mask_ptr assembly macro is removed from the final assembly.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes xince v1:
 - Drop unneeded __maybe_unused.
 - Add comment in GUARD definitions.
 - Mention register spilling in the comment message.
---
 xen/arch/x86/include/asm/uaccess.h | 45 +++++++++++++++---------------
 xen/arch/x86/usercopy.c            | 26 ++++++++---------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
index 2d01669b9610..6b8150ac221a 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.
@@ -104,7 +101,7 @@ void noreturn __put_user_bad(void);
 #define put_unsafe(x, ptr)						\
 ({									\
 	int err_; 							\
-	put_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\
+	put_unsafe_size(x, ptr, sizeof(*(ptr)), 0, err_, -EFAULT);      \
 	err_;								\
 })
 
@@ -126,7 +123,7 @@ void noreturn __put_user_bad(void);
 #define get_unsafe(x, ptr)						\
 ({									\
 	int err_; 							\
-	get_unsafe_size(x, ptr, sizeof(*(ptr)), UA_DROP, err_, -EFAULT);\
+	get_unsafe_size(x, ptr, sizeof(*(ptr)), 0, err_, -EFAULT);	\
 	err_;								\
 })
 
@@ -155,9 +152,9 @@ struct __large_struct { unsigned long buf[100]; };
  */
 #define put_unsafe_asm(x, addr, GUARD, err, itype, rtype, ltype, errret) \
 	__asm__ __volatile__(						\
-		GUARD(							\
+		".if " STR(GUARD) "\n"					\
 		"	guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \
-		)							\
+		".endif\n"						\
 		"1:	mov"itype" %"rtype"[val], (%[ptr])\n"		\
 		"2:\n"							\
 		".section .fixup,\"ax\"\n"				\
@@ -165,16 +162,16 @@ struct __large_struct { unsigned long buf[100]; };
 		"	jmp 2b\n"					\
 		".previous\n"						\
 		_ASM_EXTABLE(1b, 3b)					\
-		: [ret] "+r" (err), [ptr] "=&r" (dummy_)		\
-		  GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_))	\
+		: [ret] "+r" (err), [ptr] "=&r" (dummy_),		\
+		  [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)		\
 		: [val] ltype (x), "m" (__m(addr)),			\
 		  "[ptr]" (addr), [errno] "i" (errret))
 
 #define get_unsafe_asm(x, addr, GUARD, err, rtype, ltype, errret)	\
 	__asm__ __volatile__(						\
-		GUARD(							\
+		".if " STR(GUARD) "\n"					\
 		"	guest_access_mask_ptr %[ptr], %[scr1], %[scr2]\n" \
-		)							\
+		".endif\n"						\
 		"1:	mov (%[ptr]), %"rtype"[val]\n"			\
 		"2:\n"							\
 		".section .fixup,\"ax\"\n"				\
@@ -184,14 +181,15 @@ struct __large_struct { unsigned long buf[100]; };
 		".previous\n"						\
 		_ASM_EXTABLE(1b, 3b)					\
 		: [ret] "+r" (err), [val] ltype (x),			\
-		  [ptr] "=&r" (dummy_)					\
-		  GUARD(, [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_))	\
+		  [ptr] "=&r" (dummy_),					\
+		  [scr1] "=&r" (dummy_), [scr2] "=&r" (dummy_)		\
 		: "m" (__m(addr)), "[ptr]" (addr),			\
 		  [errno] "i" (errret))
 
 #define put_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
     retval = 0;                                                            \
+    BUILD_BUG_ON((grd) != 0 && (grd) != 1);                                \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
@@ -214,11 +212,12 @@ do {                                                                       \
 } while ( false )
 
 #define put_guest_size(x, ptr, size, retval, errret) \
-    put_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
+    put_unsafe_size(x, ptr, size, 1, retval, errret)
 
 #define get_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
     retval = 0;                                                            \
+    BUILD_BUG_ON((grd) != 0 && (grd) != 1);                                \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
@@ -233,7 +232,7 @@ do {                                                                       \
 } while ( false )
 
 #define get_guest_size(x, ptr, size, retval, errret)                       \
-    get_unsafe_size(x, ptr, size, UA_KEEP, retval, errret)
+    get_unsafe_size(x, ptr, size, 1, retval, errret)
 
 /**
  * __copy_to_guest_pv: - Copy a block of data into guest space, with less
@@ -333,16 +332,16 @@ copy_to_unsafe(void __user *to, const void *from, unsigned int n)
 
         switch (n) {
         case 1:
-            put_unsafe_size(*(const uint8_t *)from, to, 1, UA_DROP, ret, 1);
+            put_unsafe_size(*(const uint8_t *)from, to, 1, 0, ret, 1);
             return ret;
         case 2:
-            put_unsafe_size(*(const uint16_t *)from, to, 2, UA_DROP, ret, 2);
+            put_unsafe_size(*(const uint16_t *)from, to, 2, 0, ret, 2);
             return ret;
         case 4:
-            put_unsafe_size(*(const uint32_t *)from, to, 4, UA_DROP, ret, 4);
+            put_unsafe_size(*(const uint32_t *)from, to, 4, 0, ret, 4);
             return ret;
         case 8:
-            put_unsafe_size(*(const uint64_t *)from, to, 8, UA_DROP, ret, 8);
+            put_unsafe_size(*(const uint64_t *)from, to, 8, 0, ret, 8);
             return ret;
         }
     }
@@ -374,16 +373,16 @@ copy_from_unsafe(void *to, const void __user *from, unsigned int n)
         switch ( n )
         {
         case 1:
-            get_unsafe_size(*(uint8_t *)to, from, 1, UA_DROP, ret, 1);
+            get_unsafe_size(*(uint8_t *)to, from, 1, 0, ret, 1);
             return ret;
         case 2:
-            get_unsafe_size(*(uint16_t *)to, from, 2, UA_DROP, ret, 2);
+            get_unsafe_size(*(uint16_t *)to, from, 2, 0, ret, 2);
             return ret;
         case 4:
-            get_unsafe_size(*(uint32_t *)to, from, 4, UA_DROP, ret, 4);
+            get_unsafe_size(*(uint32_t *)to, from, 4, 0, ret, 4);
             return ret;
         case 8:
-            get_unsafe_size(*(uint64_t *)to, from, 8, UA_DROP, ret, 8);
+            get_unsafe_size(*(uint64_t *)to, from, 8, 0, ret, 8);
             return ret;
         }
     }
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index 7ab2009efe4c..286080f68f7e 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -11,23 +11,23 @@
 #include <asm/uaccess.h>
 
 #ifndef GUARD
-# define GUARD UA_KEEP
+# define GUARD 1 /* Keep speculative harden guards. */
 #endif
 
 unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
 {
-    GUARD(unsigned dummy);
+    unsigned dummy;
 
     stac();
     asm volatile (
-        GUARD(
+        ".if " STR(GUARD) "\n"
         "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
-        )
+        ".endif\n"
         "1:  rep movsb\n"
         "2:\n"
         _ASM_EXTABLE(1b, 2b)
-        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
-          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
+        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
+          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
         :: "memory" );
     clac();
 
@@ -40,9 +40,9 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
 
     stac();
     asm volatile (
-        GUARD(
+        ".if " STR(GUARD) "\n"
         "    guest_access_mask_ptr %[from], %q[scratch1], %q[scratch2]\n"
-        )
+        ".endif\n"
         "1:  rep movsb\n"
         "2:\n"
         ".section .fixup,\"ax\"\n"
@@ -56,15 +56,15 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
         ".previous\n"
         _ASM_EXTABLE(1b, 6b)
         : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
-          [aux] "=&r" (dummy)
-          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
+          [aux] "=&r" (dummy),
+          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
         :: "memory" );
     clac();
 
     return n;
 }
 
-#if GUARD(1) + 0
+#if GUARD
 
 /**
  * copy_to_guest_pv: - Copy a block of data into PV guest space.
@@ -140,14 +140,14 @@ unsigned int copy_from_guest_pv(void *to, const void __user *from,
 }
 
 # undef GUARD
-# define GUARD UA_DROP
+# define GUARD 0 /* Drop speculative harden guards. */
 # define copy_to_guest_ll copy_to_unsafe_ll
 # define copy_from_guest_ll copy_from_unsafe_ll
 # undef __user
 # define __user
 # include __FILE__
 
-#endif /* GUARD(1) */
+#endif /* GUARD */
 
 /*
  * Local variables:
-- 
2.46.0

[PATCH v2 2/2] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Roger Pau Monne]

There are no violations left, make the rule globally blocking for both x86 and
ARM.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
index 755ea3271fc9..cb4e233e838d 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -80,6 +80,7 @@ MC3R1.R20.2||
 MC3R1.R20.3||
 MC3R1.R20.4||
 MC3R1.R20.6||
+MC3R1.R20.7||
 MC3R1.R20.9||
 MC3R1.R20.11||
 MC3R1.R20.12||
@@ -116,7 +117,7 @@ if(string_equal(target,"x86_64"),
 )
 
 if(string_equal(target,"arm64"),
-    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
+    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
 )
 
 -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
-- 
2.46.0

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

[Jan Beulich]

On 26.11.2024 10:35, Roger Pau Monne wrote:
> 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.

For my own education: This definitely isn't the only place where we use a
macro with variable arguments, and where the use of the respective macro
parameter can't be parenthesized. Given patch 2, why is e.g.

#define emulate_fpu_insn_stub(bytes...)                                 \
do {                                                                    \
    unsigned int nr_ = sizeof((uint8_t[]){ bytes });                    \
    memcpy(get_stub(stub), ((uint8_t[]){ bytes, 0xc3 }), nr_ + 1);      \
    invoke_stub("", "", "=m" (dummy) : "i" (0));                        \
    put_stub(stub);                                                     \
} while (0)

not an issue? The first use of "bytes" is in figure braces, so probably
fine. Yet the second use is followed by a comma, so unlikely to be okay.

Somewhat similarly for

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

Jan

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

[Nicola Vetrini]

On 2024-11-26 10:58, Jan Beulich wrote:
> On 26.11.2024 10:35, Roger Pau Monne wrote:
>> 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.
> 
> For my own education: This definitely isn't the only place where we use 
> a
> macro with variable arguments, and where the use of the respective 
> macro
> parameter can't be parenthesized. Given patch 2, why is e.g.
> 
> #define emulate_fpu_insn_stub(bytes...)                                 
> \
> do {                                                                    
> \
>     unsigned int nr_ = sizeof((uint8_t[]){ bytes });                    
> \
>     memcpy(get_stub(stub), ((uint8_t[]){ bytes, 0xc3 }), nr_ + 1);      
> \
>     invoke_stub("", "", "=m" (dummy) : "i" (0));                        
> \
>     put_stub(stub);                                                     
> \
> } while (0)
> 
> not an issue? The first use of "bytes" is in figure braces, so probably
> fine. Yet the second use is followed by a comma, so unlikely to be 
> okay.
> 

I didn't look at it in detail, but if bytes is expanded inside an 
initialization list, the configuration allows it iirc. I'll need to 
double check, though.

> Somewhat similarly for
> 
> #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 ...
> 

> Jan

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Stefano Stabellini]

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?

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

[Nicola Vetrini]

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.

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Stefano Stabellini]

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 :-)

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

[Roger Pau Monné]

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.

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

[Nicola Vetrini]

On 2025-01-10 09:29, Roger Pau Monné wrote:
> 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?
> 

Yes, exactly

> How long will it take Nicola to get the ECLAIR change propagated into
> the Gitlab runner?
> 
> Thanks, Roger.

We're still fixing the false positive upstream, but it shouldn't take 
too long so I think next week I should be able to refresh the runner.

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Nicola Vetrini]

On 2025-01-10 09:56, Nicola Vetrini wrote:
> On 2025-01-10 09:29, Roger Pau Monné wrote:
>> 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

>>> >
>>> > 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?
>> 
> 
> Yes, exactly
> 
>> How long will it take Nicola to get the ECLAIR change propagated into
>> the Gitlab runner?
>> 
>> Thanks, Roger.
> 
> We're still fixing the false positive upstream, but it shouldn't take 
> too long so I think next week I should be able to refresh the runner.

Hi Roger,

the runner is updated so, assuming no new violation of Rule 20.7 
appeared in the meantime, the rule should now be clean. In the next few 
days I'll prepare a patch to the docs to document the behaviour.

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [PATCH v2 2/2] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Roger Pau Monné]

Hello Oleksii,

This is in principle ready to go in now (I'm currently running a
private Eclair scan to ensure the patch is still OK against current
staging).  I would like to ask for a release Ack.

Thanks, Roger.

On Tue, Nov 26, 2024 at 10:35:08AM +0100, Roger Pau Monne wrote:
> There are no violations left, make the rule globally blocking for both x86 and
> ARM.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index 755ea3271fc9..cb4e233e838d 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -80,6 +80,7 @@ MC3R1.R20.2||
>  MC3R1.R20.3||
>  MC3R1.R20.4||
>  MC3R1.R20.6||
> +MC3R1.R20.7||
>  MC3R1.R20.9||
>  MC3R1.R20.11||
>  MC3R1.R20.12||
> @@ -116,7 +117,7 @@ if(string_equal(target,"x86_64"),
>  )
>  
>  if(string_equal(target,"arm64"),
> -    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
> +    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
>  )
>  
>  -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
> -- 
> 2.46.0
> 

Re: [PATCH v2 2/2] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Nicola Vetrini]

On 2025-01-14 12:22, Roger Pau Monné wrote:
> Hello Oleksii,
> 
> This is in principle ready to go in now (I'm currently running a
> private Eclair scan to ensure the patch is still OK against current
> staging).  I would like to ask for a release Ack.
> 

One nit below, which I overlooked initially

> Thanks, Roger.
> 
> On Tue, Nov 26, 2024 at 10:35:08AM +0100, Roger Pau Monne wrote:
>> There are no violations left, make the rule globally blocking for both 
>> x86 and
>> ARM.
>> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>>  automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl 
>> b/automation/eclair_analysis/ECLAIR/tagging.ecl
>> index 755ea3271fc9..cb4e233e838d 100644
>> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
>> @@ -80,6 +80,7 @@ MC3R1.R20.2||
>>  MC3R1.R20.3||
>>  MC3R1.R20.4||
>>  MC3R1.R20.6||
>> +MC3R1.R20.7||
>>  MC3R1.R20.9||
>>  MC3R1.R20.11||
>>  MC3R1.R20.12||
>> @@ -116,7 +117,7 @@ if(string_equal(target,"x86_64"),
>>  )

this hunk will not apply because it uses MC3R1, rather than MC3R2. 
Should be an easy fix.

>> 
>>  if(string_equal(target,"arm64"),
>> -    
>> service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
>> +    
>> service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
>>  )

here as well

>> 
>>  
>> -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
>> --
>> 2.46.0
>> 

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [PATCH v2 2/2] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Roger Pau Monné]

On Tue, Jan 14, 2025 at 12:24:30PM +0100, Nicola Vetrini wrote:
> On 2025-01-14 12:22, Roger Pau Monné wrote:
> > Hello Oleksii,
> > 
> > This is in principle ready to go in now (I'm currently running a
> > private Eclair scan to ensure the patch is still OK against current
> > staging).  I would like to ask for a release Ack.
> > 
> 
> One nit below, which I overlooked initially
> 
> > Thanks, Roger.
> > 
> > On Tue, Nov 26, 2024 at 10:35:08AM +0100, Roger Pau Monne wrote:
> > > There are no violations left, make the rule globally blocking for
> > > both x86 and
> > > ARM.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > ---
> > >  automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > b/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > index 755ea3271fc9..cb4e233e838d 100644
> > > --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > @@ -80,6 +80,7 @@ MC3R1.R20.2||
> > >  MC3R1.R20.3||
> > >  MC3R1.R20.4||
> > >  MC3R1.R20.6||
> > > +MC3R1.R20.7||
> > >  MC3R1.R20.9||
> > >  MC3R1.R20.11||
> > >  MC3R1.R20.12||
> > > @@ -116,7 +117,7 @@ if(string_equal(target,"x86_64"),
> > >  )
> 
> this hunk will not apply because it uses MC3R1, rather than MC3R2. Should be
> an easy fix.
> 
> > > 
> > >  if(string_equal(target,"arm64"),
> > > -    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
> > > +    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
> > >  )
> 
> here as well

Yeah indeed, I had to rebase the patch:

https://gitlab.com/xen-project/people/royger/xen/-/commit/538439d59dc338ee3861bf1bc056783671ba1fc2

Let's see if Eclair is happy with it, currently running a pipeline.

Thanks, Roger.

Re: [PATCH v2 2/2] automation/eclair: make Misra rule 20.7 blocking for x86 also

[Oleksii Kurochko]

Hi Roger,

On 1/14/25 12:22 PM, Roger Pau Monné wrote:
> Hello Oleksii,
>
> This is in principle ready to go in now (I'm currently running a
> private Eclair scan to ensure the patch is still OK against current
> staging).  I would like to ask for a release Ack.

R-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii


>
> Thanks, Roger.
>
> On Tue, Nov 26, 2024 at 10:35:08AM +0100, Roger Pau Monne wrote:
>> There are no violations left, make the rule globally blocking for both x86 and
>> ARM.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>>   automation/eclair_analysis/ECLAIR/tagging.ecl | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
>> index 755ea3271fc9..cb4e233e838d 100644
>> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
>> @@ -80,6 +80,7 @@ MC3R1.R20.2||
>>   MC3R1.R20.3||
>>   MC3R1.R20.4||
>>   MC3R1.R20.6||
>> +MC3R1.R20.7||
>>   MC3R1.R20.9||
>>   MC3R1.R20.11||
>>   MC3R1.R20.12||
>> @@ -116,7 +117,7 @@ if(string_equal(target,"x86_64"),
>>   )
>>   
>>   if(string_equal(target,"arm64"),
>> -    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6||MC3R1.R20.7"})
>> +    service_selector({"additional_clean_guidelines","MC3R1.R2.1||MC3R1.R5.3||MC3.R11.2||MC3R1.R16.6"})
>>   )
>>   
>>   -reports+={clean:added,"service(clean_guidelines_common||additional_clean_guidelines)"}
>> -- 
>> 2.46.0
>>