[v1] device-tree: fix infinite loop issue in 'assign_shared_memory()'

[PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Dmytro Prokopchuk1 2 months ago

Resolve infinite loop issue in the 'fail:' cleanup path of the function
'assign_shared_memory()'. The issue was caused by an 'unsigned long' type
for the loop counter 'i', which could underflow and wrap around, violating
termination conditions.
Change 'i' to a signed data type ('long') to ensure safe termination of
the 'while (--i >= 0)' loop.

This change adheres to MISRA Rule R14.3: "Controlling expressions shall
not be invariant."

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
 xen/common/device-tree/static-shmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/device-tree/static-shmem.c b/xen/common/device-tree/static-shmem.c
index 8023c0a484..b4c772466c 100644
--- a/xen/common/device-tree/static-shmem.c
+++ b/xen/common/device-tree/static-shmem.c
@@ -134,7 +134,8 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
 {
     mfn_t smfn;
     int ret = 0;
-    unsigned long nr_pages, nr_borrowers, i;
+    unsigned long nr_pages, nr_borrowers;
+    long i;
     struct page_info *page;
     paddr_t pbase, psize;
 
-- 
2.43.0

Re: [PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Jan Beulich 2 months ago

On 28.08.2025 10:17, Dmytro Prokopchuk1 wrote:
> Resolve infinite loop issue in the 'fail:' cleanup path of the function
> 'assign_shared_memory()'. The issue was caused by an 'unsigned long' type
> for the loop counter 'i', which could underflow and wrap around, violating
> termination conditions.
> Change 'i' to a signed data type ('long') to ensure safe termination of
> the 'while (--i >= 0)' loop.

If I was a maintainer of this code, I would strongly object to such a change.
A signed type variable used as (effectively) an array index is almost always
conceptually wrong. Plus i continues to be compared to nr_pages, which still
is of an unsigned type.

What imo wants changing instead is the use of the variable:

 fail:
    while ( i-- > 0 )
        put_page_nr(page + i, nr_borrowers);

or yet more simply

 fail:
    while ( i-- )
        put_page_nr(page + i, nr_borrowers);

See e.g. prepare_staticmem_pages() for a similar case.

Jan

Re: [PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Dmytro Prokopchuk1 2 months ago


On 8/28/25 11:42, Jan Beulich wrote:
> On 28.08.2025 10:17, Dmytro Prokopchuk1 wrote:
>> Resolve infinite loop issue in the 'fail:' cleanup path of the function
>> 'assign_shared_memory()'. The issue was caused by an 'unsigned long' type
>> for the loop counter 'i', which could underflow and wrap around, violating
>> termination conditions.
>> Change 'i' to a signed data type ('long') to ensure safe termination of
>> the 'while (--i >= 0)' loop.
> 
> If I was a maintainer of this code, I would strongly object to such a change.
> A signed type variable used as (effectively) an array index is almost always
> conceptually wrong. Plus i continues to be compared to nr_pages, which still
> is of an unsigned type.
> 
> What imo wants changing instead is the use of the variable:
> 
>   fail:
>      while ( i-- > 0 )
>          put_page_nr(page + i, nr_borrowers);
> 
> or yet more simply
> 
>   fail:
>      while ( i-- )
>          put_page_nr(page + i, nr_borrowers);
> 
> See e.g. prepare_staticmem_pages() for a similar case.
> 
> Jan

I had the such thought...
Thanks for advice. I will change it.

Dmytro.

Re: [PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Julien Grall 2 months ago


On 28/08/2025 09:42, Jan Beulich wrote:
> On 28.08.2025 10:17, Dmytro Prokopchuk1 wrote:
>> Resolve infinite loop issue in the 'fail:' cleanup path of the function
>> 'assign_shared_memory()'. The issue was caused by an 'unsigned long' type
>> for the loop counter 'i', which could underflow and wrap around, violating
>> termination conditions.
>> Change 'i' to a signed data type ('long') to ensure safe termination of
>> the 'while (--i >= 0)' loop.
> 
> If I was a maintainer of this code, I would strongly object to such a change.
> A signed type variable used as (effectively) an array index is almost always
> conceptually wrong. Plus i continues to be compared to nr_pages, which still
> is of an unsigned type.
> 
> What imo wants changing instead is the use of the variable:
> 
>   fail:
>      while ( i-- > 0 )
>          put_page_nr(page + i, nr_borrowers);
> 
> or yet more simply
> 
>   fail:
>      while ( i-- )
>          put_page_nr(page + i, nr_borrowers);
> 
> See e.g. prepare_staticmem_pages() for a similar case.

+1 with Jan's comment.

Cheers,

-- 
Julien Grall

Re: [PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Nicola Vetrini 2 months ago

On 2025-08-28 10:17, Dmytro Prokopchuk1 wrote:
> Resolve infinite loop issue in the 'fail:' cleanup path of the function
> 'assign_shared_memory()'. The issue was caused by an 'unsigned long' 
> type
> for the loop counter 'i', which could underflow and wrap around, 
> violating
> termination conditions.
> Change 'i' to a signed data type ('long') to ensure safe termination of
> the 'while (--i >= 0)' loop.
> 

Then this likely should have Fixes tag. The R14.3 violation was found 
after adding CONFIG_UNSUPPORTED=y to analyze.yaml?

> This change adheres to MISRA Rule R14.3: "Controlling expressions shall
> not be invariant."
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
>  xen/common/device-tree/static-shmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/device-tree/static-shmem.c 
> b/xen/common/device-tree/static-shmem.c
> index 8023c0a484..b4c772466c 100644
> --- a/xen/common/device-tree/static-shmem.c
> +++ b/xen/common/device-tree/static-shmem.c
> @@ -134,7 +134,8 @@ static int __init assign_shared_memory(struct 
> domain *d, paddr_t gbase,
>  {
>      mfn_t smfn;
>      int ret = 0;
> -    unsigned long nr_pages, nr_borrowers, i;
> +    unsigned long nr_pages, nr_borrowers;
> +    long i;
>      struct page_info *page;
>      paddr_t pbase, psize;

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

Re: [PATCH] device-tree: fix infinite loop issue in 'assign_shared_memory()'

Posted by Dmytro Prokopchuk1 2 months ago


On 8/28/25 11:31, Nicola Vetrini wrote:
> On 2025-08-28 10:17, Dmytro Prokopchuk1 wrote:
>> Resolve infinite loop issue in the 'fail:' cleanup path of the function
>> 'assign_shared_memory()'. The issue was caused by an 'unsigned long' type
>> for the loop counter 'i', which could underflow and wrap around, 
>> violating
>> termination conditions.
>> Change 'i' to a signed data type ('long') to ensure safe termination of
>> the 'while (--i >= 0)' loop.
>>
> 
> Then this likely should have Fixes tag. The R14.3 violation was found 
> after adding CONFIG_UNSUPPORTED=y to analyze.yaml?
Will add "Fixes".
Yes, with "CONFIG_UNSUPPORTED=y".
> 
>> This change adheres to MISRA Rule R14.3: "Controlling expressions shall
>> not be invariant."
>>
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>>  xen/common/device-tree/static-shmem.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/device-tree/static-shmem.c b/xen/common/ 
>> device-tree/static-shmem.c
>> index 8023c0a484..b4c772466c 100644
>> --- a/xen/common/device-tree/static-shmem.c
>> +++ b/xen/common/device-tree/static-shmem.c
>> @@ -134,7 +134,8 @@ static int __init assign_shared_memory(struct 
>> domain *d, paddr_t gbase,
>>  {
>>      mfn_t smfn;
>>      int ret = 0;
>> -    unsigned long nr_pages, nr_borrowers, i;
>> +    unsigned long nr_pages, nr_borrowers;
>> +    long i;
>>      struct page_info *page;
>>      paddr_t pbase, psize;
>