Re: [PATCH] misra: address MISRA C Rule 18.3 compliance

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-07-23 12:12, Dmytro Prokopchuk1 wrote:
> Rule 18.3: "The relational operators >, >=, < and <=
> shall not be applied to objects of pointer type
> except where they point into the same object".
>
> Update relational comparison to cast `text_start`
> (void pointer) to `unsigned long`. This ensures the
> comparison occurs between two values of the same integral type.
>
> Update relational comparison between `lock1` and `lock2` to cast
> pointers to `uintptr_t`. This ensures MISRA C compliance and avoids
> undefined behavior when comparing pointer values directly.
>
> Update for-loop condition to cast pointers `p` and `params->checksum`
> to `uintptr_t` for the relational `<` operator. This resolves a MISRA C 
> 18.3
> violation by ensuring that relational operations are not performed 
> directly
> on pointers of different objects (which is undefined behavior).
>
> Add Rule 18.3 deviations.
>
> Add Rule 18.3 to the monitored set.
>
> No functional changes to program behavior.
>
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
> ---
> This patch eliminates MISRA C Rule 18.3 violations for both ARM and 
> X86.
> Test CI: 
> https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1943306650
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 8 ++++++++
>  automation/eclair_analysis/ECLAIR/monitored.ecl  | 1 +
>  xen/arch/x86/efi/efi-boot.h                      | 3 ++-
>  xen/common/sched/core.c                          | 2 +-
>  xen/common/virtual_region.c                      | 4 ++--
>  5 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index 483507e7b9..d89834a49b 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not 
> point into, or just beyond, the
>  -config=MC3A2.R18.2,reports+={safe, 
> "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>  -doc_end
>
> +-doc_begin="Consider relational pointer comparisons in kernel-related 
> sections as safe and compliant."
This would need some evidence that they don't compare pointers to 
different arrays, wouldn't it?
> +-config=MC3R1.R18.3,reports+={safe, 
> "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
> +-doc_end
> +
> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and 
> ASSERT macros, treating them as safe for debugging and validation."
> +-config=MC3R1.R18.3,reports+={safe, 
> "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
> +-doc_end
> +
same here: no matter that they are used for debugging, if you mark 
something as "safe", then the argument needs to be there (in the .rst 
file, but still).
>  -doc_begin="Flexible array members are deliberately used and XEN 
> developers are aware of the dangers related to them:
>  unexpected result when the structure is given as argument to a 
> sizeof() operator and the truncation in assignment between structures."
>  -config=MC3A2.R18.7,reports+={deliberate, "any()"}
> diff --git a/automation/eclair_analysis/ECLAIR/monitored.ecl 
> b/automation/eclair_analysis/ECLAIR/monitored.ecl
> index 00bff9edbe..b8fb297e73 100644
> --- a/automation/eclair_analysis/ECLAIR/monitored.ecl
> +++ b/automation/eclair_analysis/ECLAIR/monitored.ecl
> @@ -68,6 +68,7 @@
>  -enable=MC3A2.R17.6
>  -enable=MC3A2.R18.1
>  -enable=MC3A2.R18.2
> +-enable=MC3A2.R18.3
>  -enable=MC3A2.R18.6
>  -enable=MC3A2.R18.8
>  -enable=MC3A2.R19.1
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 0194720003..170c569eb4 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>                  params->device_path_info_length =
>                      sizeof(struct edd_device_params) -
>                      offsetof(struct edd_device_params, key);
> -                for ( p = (const u8 *)&params->key; p < 
> &params->checksum; ++p )
> +                for ( p = (const u8 *)&params->key;
> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p 
> )
>                      params->checksum -= *p;
>                  break;
>              case MEDIA_DEVICE_PATH:
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index ea95dea65a..c3c101c142 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>      {
>          *flags = _spin_lock_irqsave(lock1);
>      }
> -    else if ( lock1 < lock2 )
> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
>      {
>          *flags = _spin_lock_irqsave(lock1);
>          _spin_lock(lock2);
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index 1dc2e9f592..515751b6c3 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -83,8 +83,8 @@ const struct virtual_region 
> *find_text_region(unsigned long addr)
>      rcu_read_lock(&rcu_virtual_region_lock);
>      list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>      {
> -        if ( (void *)addr >= iter->text_start &&
> -             (void *)addr <  iter->text_end )
> +        if ( addr >= (unsigned long)iter->text_start &&
> +             addr <  (unsigned long)iter->text_end )
>          {
>              region = iter;
>              break;
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253