[XEN PATCH 0/3] xen: address violations of MISRA C Rule 13.6

[XEN PATCH 0/3] xen: address violations of MISRA C Rule 13.6

[Federico Serafini]

Address remaining violations of Rule 13.6 and tag it as clean.

Federico Serafini (3):
  EFI: address violations of MISRA C Rule 13.6
  xen/gnttab: address violations of MISRA C Rule 13.6
  automation/eclair: tag Rule 13.6 as clean

 automation/eclair_analysis/ECLAIR/tagging.ecl |  1 +
 xen/common/compat/grant_table.c               | 15 +++++++++------
 xen/common/efi/runtime.c                      | 12 ++++++++++--
 3 files changed, 20 insertions(+), 8 deletions(-)

-- 
2.34.1

[XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Federico Serafini]

Refactor the code to improve readability and address violations of
MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
not contain any expression which has potential side effect").

No functional change.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/common/efi/runtime.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index d03e5c90ce..acf08dcaa3 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
         info->cfg.addr = __pa(efi_ct);
         info->cfg.nent = efi_num_ct;
         break;
+
     case XEN_FW_EFI_VENDOR:
+    {
+        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
+            guest_handle_cast(info->vendor.name, CHAR16);
+
         if ( !efi_fw_vendor )
             return -EOPNOTSUPP;
+
         info->vendor.revision = efi_fw_revision;
         n = info->vendor.bufsz / sizeof(*efi_fw_vendor);
-        if ( !guest_handle_okay(guest_handle_cast(info->vendor.name,
-                                                  CHAR16), n) )
+        if ( !guest_handle_okay(vendor_name, n) )
             return -EFAULT;
+
         for ( i = 0; i < n; ++i )
         {
             if ( __copy_to_guest_offset(info->vendor.name, i,
@@ -267,6 +273,8 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
                 break;
         }
         break;
+    }
+
     case XEN_FW_EFI_MEM_INFO:
         for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
         {
-- 
2.34.1

[XEN PATCH 2/3] xen/gnttab: address violations of MISRA C Rule 13.6

[Federico Serafini]

Refactor the code to improve readability and address violations of
MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
not contain any expression which has potential side effect").

No functional change.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/common/compat/grant_table.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 5ad0debf96..4342e573c5 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -78,12 +78,15 @@ int compat_grant_table_op(
         cmd_op = cmd;
     switch ( cmd_op )
     {
-#define CASE(name) \
-    case GNTTABOP_##name: \
-        if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
-                                                           gnttab_##name##_compat_t), \
-                                         count)) ) \
-            rc = -EFAULT; \
+#define CASE(name)                                                      \
+        case GNTTABOP_ ## name:                                         \
+        {                                                               \
+            XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h =    \
+                guest_handle_cast(uop, gnttab_ ## name ## _compat_t);   \
+                                                                        \
+            if ( unlikely(!guest_handle_okay(h, count)) )               \
+                rc = -EFAULT;                                           \
+        }                                                               \
         break
 
 #ifndef CHECK_gnttab_map_grant_ref
-- 
2.34.1

[XEN PATCH 3/3] automation/eclair: tag Rule 13.6 as clean

[Federico Serafini]

Update ECLAIR configuration to consider Rule 13.6 as clean:
introducing violations of this rule will cause a failure of the CI pipeline.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
index b8448938e6..76032b1fe1 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -60,6 +60,7 @@ MC3R1.R11.6||
 MC3R1.R11.7||
 MC3R1.R11.9||
 MC3R1.R12.5||
+MC3R1.R13.6||
 MC3R1.R14.1||
 MC3R1.R14.3||
 MC3R1.R14.4||
-- 
2.34.1

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Jan Beulich]

On 10.09.2024 21:06, Federico Serafini wrote:
> Refactor the code to improve readability

I question this aspect. I'm not the maintainer of this code anymore, so
my view probably doesn't matter much here.

> and address violations of
> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
> not contain any expression which has potential side effect").

Where's the potential side effect? Since you move ...

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>          info->cfg.addr = __pa(efi_ct);
>          info->cfg.nent = efi_num_ct;
>          break;
> +
>      case XEN_FW_EFI_VENDOR:
> +    {
> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
> +            guest_handle_cast(info->vendor.name, CHAR16);

.. this out, it must be the one. I've looked at it, yet I can't spot
anything:

#define guest_handle_cast(hnd, type) ({         \
    type *_x = (hnd).p;                         \
    (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
})

As a rule of thumb, when things aren't obvious, please call out the
specific aspect / property in descriptions of such patches.

Jan

Re: [XEN PATCH 2/3] xen/gnttab: address violations of MISRA C Rule 13.6

[Jan Beulich]

On 10.09.2024 21:06, Federico Serafini wrote:
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -78,12 +78,15 @@ int compat_grant_table_op(
>          cmd_op = cmd;
>      switch ( cmd_op )
>      {
> -#define CASE(name) \
> -    case GNTTABOP_##name: \
> -        if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
> -                                                           gnttab_##name##_compat_t), \
> -                                         count)) ) \
> -            rc = -EFAULT; \
> +#define CASE(name)                                                      \
> +        case GNTTABOP_ ## name:                                         \

Why the re-indentation? The earlier way was pretty intentional, to match
what a non-macroized case label would look like in this switch.

> +        {                                                               \
> +            XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h =    \
> +                guest_handle_cast(uop, gnttab_ ## name ## _compat_t);   \
> +                                                                        \
> +            if ( unlikely(!guest_handle_okay(h, count)) )               \
> +                rc = -EFAULT;                                           \

Same question as for the earlier patch - where's the potential side
effect?

Jan

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
> On 10.09.2024 21:06, Federico Serafini wrote:
> > Refactor the code to improve readability
> 
> I question this aspect. I'm not the maintainer of this code anymore, so
> my view probably doesn't matter much here.
> 
> > and address violations of
> > MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
> > not contain any expression which has potential side effect").
> 
> Where's the potential side effect? Since you move ...
> 
> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> >          info->cfg.addr = __pa(efi_ct);
> >          info->cfg.nent = efi_num_ct;
> >          break;
> > +
> >      case XEN_FW_EFI_VENDOR:
> > +    {
> > +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
> > +            guest_handle_cast(info->vendor.name, CHAR16);
> 
> .. this out, it must be the one. I've looked at it, yet I can't spot
> anything:
> 
> #define guest_handle_cast(hnd, type) ({         \
>     type *_x = (hnd).p;                         \
>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
> })
> 
> As a rule of thumb, when things aren't obvious, please call out the
> specific aspect / property in descriptions of such patches.

I guess it's because guest_handle_cast() is a macro, yet it's lowercase
so looks like a function? Wasn't there some other MISRA rule about
lowercase/uppercase for macro names?

And yes, I don't really see why this would violate the side effect rule
either.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Jan Beulich]

On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>> On 10.09.2024 21:06, Federico Serafini wrote:
>>> Refactor the code to improve readability
>>
>> I question this aspect. I'm not the maintainer of this code anymore, so
>> my view probably doesn't matter much here.
>>
>>> and address violations of
>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>> not contain any expression which has potential side effect").
>>
>> Where's the potential side effect? Since you move ...
>>
>>> --- a/xen/common/efi/runtime.c
>>> +++ b/xen/common/efi/runtime.c
>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>>          info->cfg.addr = __pa(efi_ct);
>>>          info->cfg.nent = efi_num_ct;
>>>          break;
>>> +
>>>      case XEN_FW_EFI_VENDOR:
>>> +    {
>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>
>> .. this out, it must be the one. I've looked at it, yet I can't spot
>> anything:
>>
>> #define guest_handle_cast(hnd, type) ({         \
>>     type *_x = (hnd).p;                         \
>>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>> })
>>
>> As a rule of thumb, when things aren't obvious, please call out the
>> specific aspect / property in descriptions of such patches.
> 
> I guess it's because guest_handle_cast() is a macro, yet it's lowercase
> so looks like a function?

If Eclair didn't look at the macro-expanded code, it wouldn't even see
the sizeof(). Hence I don't expect the thing to be mistaken for a function
call.

> Wasn't there some other MISRA rule about lowercase/uppercase for macro names?

I can't recall having run into one, but I also haven't memorized them all.

Jan

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Andrew Cooper]

On 11/09/2024 3:10 pm, Jan Beulich wrote:
> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>> Refactor the code to improve readability
>>> I question this aspect. I'm not the maintainer of this code anymore, so
>>> my view probably doesn't matter much here.
>>>
>>>> and address violations of
>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>> not contain any expression which has potential side effect").
>>> Where's the potential side effect? Since you move ...
>>>
>>>> --- a/xen/common/efi/runtime.c
>>>> +++ b/xen/common/efi/runtime.c
>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>>>          info->cfg.addr = __pa(efi_ct);
>>>>          info->cfg.nent = efi_num_ct;
>>>>          break;
>>>> +
>>>>      case XEN_FW_EFI_VENDOR:
>>>> +    {
>>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>> anything:
>>>
>>> #define guest_handle_cast(hnd, type) ({         \
>>>     type *_x = (hnd).p;                         \
>>>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>>> })
>>>
>>> As a rule of thumb, when things aren't obvious, please call out the
>>> specific aspect / property in descriptions of such patches.
>> I guess it's because guest_handle_cast() is a macro, yet it's lowercase
>> so looks like a function?
> If Eclair didn't look at the macro-expanded code, it wouldn't even see
> the sizeof(). Hence I don't expect the thing to be mistaken for a function
> call.

The complaint is a sizeof in guest_handle_okay() being given ({ ... })
to interpret.

({}) can have arbitrary side effects in it, hence the violation.

~Andrew

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Nicola Vetrini]

On 2024-09-11 16:10, Jan Beulich wrote:
> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>> Refactor the code to improve readability
>>> 
>>> I question this aspect. I'm not the maintainer of this code anymore, 
>>> so
>>> my view probably doesn't matter much here.
>>> 
>>>> and address violations of
>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>> not contain any expression which has potential side effect").
>>> 
>>> Where's the potential side effect? Since you move ...
>>> 
>>>> --- a/xen/common/efi/runtime.c
>>>> +++ b/xen/common/efi/runtime.c
>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union 
>>>> xenpf_efi_info *info)
>>>>          info->cfg.addr = __pa(efi_ct);
>>>>          info->cfg.nent = efi_num_ct;
>>>>          break;
>>>> +
>>>>      case XEN_FW_EFI_VENDOR:
>>>> +    {
>>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>> 
>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>> anything:
>>> 
>>> #define guest_handle_cast(hnd, type) ({         \
>>>     type *_x = (hnd).p;                         \
>>>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>>> })
>>> 
>>> As a rule of thumb, when things aren't obvious, please call out the
>>> specific aspect / property in descriptions of such patches.
>> 
>> I guess it's because guest_handle_cast() is a macro, yet it's 
>> lowercase
>> so looks like a function?
> 
> If Eclair didn't look at the macro-expanded code, it wouldn't even see
> the sizeof(). Hence I don't expect the thing to be mistaken for a 
> function
> call.
> 

Looking at the fully preprocessed code [1], there is an assignment to 
CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering 
the violation when used in such a way to be inside the sizeof().

if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode 
& ((1 << 4) << 10))) || (
__builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = 
(__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; 
}))._)))),1) && ((unsigned long)((unsigned long)((void *)(
full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
*)(full_ptr_t)(info->vendor.name).c; (
__compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * 
(sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; 
(__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
* (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
*)(full_ptr_t)(info->vendor.name).c; (
__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < 
((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
) )

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/latest/PROJECT.ecd;/by_service/MC3R1.R13.6.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":false,"selector":{"enabled":true,"negated":true,"kind":0,"domain":"message","inputs":[{"enabled":true,"text":"^.*xen/common/efi/runtime\\.c:258\\.15-258\\.31: 
`sizeof' expression trait"}]}}}

>> Wasn't there some other MISRA rule about lowercase/uppercase for macro 
>> names?
> 

There isn't one imposing this restriction (at least in MISRA C:2012, I 
haven't checked earlier editions).

> I can't recall having run into one, but I also haven't memorized them 
> all.
> 
> Jan

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

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Jan Beulich]

On 11.09.2024 16:27, Andrew Cooper wrote:
> On 11/09/2024 3:10 pm, Jan Beulich wrote:
>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>>> Refactor the code to improve readability
>>>> I question this aspect. I'm not the maintainer of this code anymore, so
>>>> my view probably doesn't matter much here.
>>>>
>>>>> and address violations of
>>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>>> not contain any expression which has potential side effect").
>>>> Where's the potential side effect? Since you move ...
>>>>
>>>>> --- a/xen/common/efi/runtime.c
>>>>> +++ b/xen/common/efi/runtime.c
>>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>>>>          info->cfg.addr = __pa(efi_ct);
>>>>>          info->cfg.nent = efi_num_ct;
>>>>>          break;
>>>>> +
>>>>>      case XEN_FW_EFI_VENDOR:
>>>>> +    {
>>>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>>> anything:
>>>>
>>>> #define guest_handle_cast(hnd, type) ({         \
>>>>     type *_x = (hnd).p;                         \
>>>>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>>>> })
>>>>
>>>> As a rule of thumb, when things aren't obvious, please call out the
>>>> specific aspect / property in descriptions of such patches.
>>> I guess it's because guest_handle_cast() is a macro, yet it's lowercase
>>> so looks like a function?
>> If Eclair didn't look at the macro-expanded code, it wouldn't even see
>> the sizeof(). Hence I don't expect the thing to be mistaken for a function
>> call.
> 
> The complaint is a sizeof in guest_handle_okay() being given ({ ... })
> to interpret.
> 
> ({}) can have arbitrary side effects in it, hence the violation.

I sincerely hope the tool actually looks inside the ({}).

Jan

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Jan Beulich]

On 11.09.2024 16:27, Nicola Vetrini wrote:
> On 2024-09-11 16:10, Jan Beulich wrote:
>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>>> Refactor the code to improve readability
>>>>
>>>> I question this aspect. I'm not the maintainer of this code anymore, 
>>>> so
>>>> my view probably doesn't matter much here.
>>>>
>>>>> and address violations of
>>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>>> not contain any expression which has potential side effect").
>>>>
>>>> Where's the potential side effect? Since you move ...
>>>>
>>>>> --- a/xen/common/efi/runtime.c
>>>>> +++ b/xen/common/efi/runtime.c
>>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union 
>>>>> xenpf_efi_info *info)
>>>>>          info->cfg.addr = __pa(efi_ct);
>>>>>          info->cfg.nent = efi_num_ct;
>>>>>          break;
>>>>> +
>>>>>      case XEN_FW_EFI_VENDOR:
>>>>> +    {
>>>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>>>
>>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>>> anything:
>>>>
>>>> #define guest_handle_cast(hnd, type) ({         \
>>>>     type *_x = (hnd).p;                         \
>>>>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>>>> })
>>>>
>>>> As a rule of thumb, when things aren't obvious, please call out the
>>>> specific aspect / property in descriptions of such patches.
>>>
>>> I guess it's because guest_handle_cast() is a macro, yet it's 
>>> lowercase
>>> so looks like a function?
>>
>> If Eclair didn't look at the macro-expanded code, it wouldn't even see
>> the sizeof(). Hence I don't expect the thing to be mistaken for a 
>> function
>> call.
>>
> 
> Looking at the fully preprocessed code [1], there is an assignment to 
> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering 
> the violation when used in such a way to be inside the sizeof().

I can see a number of initializers, but no assignment.

Jan

> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode 
> & ((1 << 4) << 10))) || (
> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = 
> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; 
> }))._)))),1) && ((unsigned long)((unsigned long)((void *)(
> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * 
> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; 
> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < 
> ((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
> ) )

Re: [XEN PATCH 3/3] automation/eclair: tag Rule 13.6 as clean

[Stefano Stabellini]

On Tue, 10 Sep 2024, Federico Serafini wrote:
> Update ECLAIR configuration to consider Rule 13.6 as clean:
> introducing violations of this rule will cause a failure of the CI pipeline.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>


With of course the understanding that this patch can only be committed
after the last two 13.6 fixes are committed:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index b8448938e6..76032b1fe1 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -60,6 +60,7 @@ MC3R1.R11.6||
>  MC3R1.R11.7||
>  MC3R1.R11.9||
>  MC3R1.R12.5||
> +MC3R1.R13.6||
>  MC3R1.R14.1||
>  MC3R1.R14.3||
>  MC3R1.R14.4||
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/3] xen/gnttab: address violations of MISRA C Rule 13.6

[Stefano Stabellini]

On Wed, 11 Sep 2024, Jan Beulich wrote:
> On 10.09.2024 21:06, Federico Serafini wrote:
> > --- a/xen/common/compat/grant_table.c
> > +++ b/xen/common/compat/grant_table.c
> > @@ -78,12 +78,15 @@ int compat_grant_table_op(
> >          cmd_op = cmd;
> >      switch ( cmd_op )
> >      {
> > -#define CASE(name) \
> > -    case GNTTABOP_##name: \
> > -        if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
> > -                                                           gnttab_##name##_compat_t), \
> > -                                         count)) ) \
> > -            rc = -EFAULT; \
> > +#define CASE(name)                                                      \
> > +        case GNTTABOP_ ## name:                                         \
> 
> Why the re-indentation? The earlier way was pretty intentional, to match
> what a non-macroized case label would look like in this switch.
> 
> > +        {                                                               \
> > +            XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h =    \
> > +                guest_handle_cast(uop, gnttab_ ## name ## _compat_t);   \
> > +                                                                        \
> > +            if ( unlikely(!guest_handle_okay(h, count)) )               \
> > +                rc = -EFAULT;                                           \
> 
> Same question as for the earlier patch - where's the potential side
> effect?

Leaving aside the re-indentation / readability changes, I think the fix
is to move the call to guest_handle_cast out of guest_handle_okay.

Since guest_handle_okay is implemented by calling sizeof on *h.p, I am
guessing that passing guest_handle_cast() as h causes problems. I am not
sure if there is a side effect, I cannot spot one, but I can see that
the nested macros could cause issues to a static analyzer.

Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Federico Serafini]

On 11/09/24 16:57, Jan Beulich wrote:
> On 11.09.2024 16:27, Nicola Vetrini wrote:
>> On 2024-09-11 16:10, Jan Beulich wrote:
>>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>>>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>>>> Refactor the code to improve readability
>>>>>
>>>>> I question this aspect. I'm not the maintainer of this code anymore,
>>>>> so
>>>>> my view probably doesn't matter much here.
>>>>>
>>>>>> and address violations of
>>>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>>>> not contain any expression which has potential side effect").
>>>>>
>>>>> Where's the potential side effect? Since you move ...
>>>>>
>>>>>> --- a/xen/common/efi/runtime.c
>>>>>> +++ b/xen/common/efi/runtime.c
>>>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union
>>>>>> xenpf_efi_info *info)
>>>>>>           info->cfg.addr = __pa(efi_ct);
>>>>>>           info->cfg.nent = efi_num_ct;
>>>>>>           break;
>>>>>> +
>>>>>>       case XEN_FW_EFI_VENDOR:
>>>>>> +    {
>>>>>> +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>>>> +            guest_handle_cast(info->vendor.name, CHAR16);
>>>>>
>>>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>>>> anything:
>>>>>
>>>>> #define guest_handle_cast(hnd, type) ({         \
>>>>>      type *_x = (hnd).p;                         \
>>>>>      (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>>>>> })
>>>>>
>>>>> As a rule of thumb, when things aren't obvious, please call out the
>>>>> specific aspect / property in descriptions of such patches.
>>>>
>>>> I guess it's because guest_handle_cast() is a macro, yet it's
>>>> lowercase
>>>> so looks like a function?
>>>
>>> If Eclair didn't look at the macro-expanded code, it wouldn't even see
>>> the sizeof(). Hence I don't expect the thing to be mistaken for a
>>> function
>>> call.
>>>
>>
>> Looking at the fully preprocessed code [1], there is an assignment to
>> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering
>> the violation when used in such a way to be inside the sizeof().
> 
> I can see a number of initializers, but no assignment.

+ Stefano in CC.

MISRA considers the initialization (even of a local variable)
a side effect. This is the reason of the violations.

I will send a V2 with a better description.

> 
>> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode
>> & ((1 << 4) << 10))) || (
>> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x =
>> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
>> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x };
>> }))._)))),1) && ((unsigned long)((unsigned long)((void *)(
>> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
>> *)(full_ptr_t)(info->vendor.name).c; (
>> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) *
>> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
>> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c;
>> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
>> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
>> *)(full_ptr_t)(info->vendor.name).c; (
>> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) <
>> ((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
>> ) )
> 

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)