[PATCH] fix potential pa_range_info out of bound access

[PATCH] fix potential pa_range_info out of bound access

[Stefano Stabellini]

pa_range_info has only 8 elements and is accessed using pa_range as
index. pa_range is initialized to 16, potentially causing out of bound
access errors. Fix the issue by initializing pa_range to the effective
number of pa_range_info elements.

CID 1381865

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e4991df..245fcd1 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
     };
 
     unsigned int cpu;
-    unsigned int pa_range = 0x10; /* Larger than any possible value */
+    unsigned int pa_range = sizeof(pa_range_info) / sizeof(pa_range_info[0]);
 
     for_each_online_cpu ( cpu )
     {

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] fix potential pa_range_info out of bound access

[Stefano Stabellini]

On Thu, 8 Dec 2016, Stefano Stabellini wrote:
> pa_range_info has only 8 elements and is accessed using pa_range as
> index. pa_range is initialized to 16, potentially causing out of bound
> access errors. Fix the issue by initializing pa_range to the effective
> number of pa_range_info elements.
> 
> CID 1381865
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e4991df..245fcd1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
>      };
>  
>      unsigned int cpu;
> -    unsigned int pa_range = 0x10; /* Larger than any possible value */
> +    unsigned int pa_range = sizeof(pa_range_info) / sizeof(pa_range_info[0]);
>  
>      for_each_online_cpu ( cpu )
>      {

this is wrong, it should be sizeof(pa_range_info) / sizeof(pa_range_info[0]) - 1:

---
pa_range_info has only 8 elements and is accessed using pa_range as
index. pa_range is initialized to 16, potentially causing out of bound
access errors. Fix the issue by initializing pa_range to the effective
number of pa_range_info elements minus 1.

Coverity-ID: 1381865

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e4991df..14901b0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
     };
 
     unsigned int cpu;
-    unsigned int pa_range = 0x10; /* Larger than any possible value */
+    unsigned int pa_range = ARRAY_SIZE(pa_range_info) - 1;
 
     for_each_online_cpu ( cpu )
     {

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] fix potential pa_range_info out of bound access

[Julien Grall]

Hi Stefano,

On 09/12/16 01:40, Stefano Stabellini wrote:
> On Thu, 8 Dec 2016, Stefano Stabellini wrote:
>> pa_range_info has only 8 elements and is accessed using pa_range as
>> index. pa_range is initialized to 16, potentially causing out of bound
>> access errors. Fix the issue by initializing pa_range to the effective
>> number of pa_range_info elements.
>>
>> CID 1381865
>>
>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index e4991df..245fcd1 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
>>      };
>>
>>      unsigned int cpu;
>> -    unsigned int pa_range = 0x10; /* Larger than any possible value */
>> +    unsigned int pa_range = sizeof(pa_range_info) / sizeof(pa_range_info[0]);
>>
>>      for_each_online_cpu ( cpu )
>>      {
>
> this is wrong, it should be sizeof(pa_range_info) / sizeof(pa_range_info[0]) - 1:
>
> ---
> pa_range_info has only 8 elements and is accessed using pa_range as
> index. pa_range is initialized to 16, potentially causing out of bound
> access errors. Fix the issue by initializing pa_range to the effective
> number of pa_range_info elements minus 1.
>
> Coverity-ID: 1381865
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e4991df..14901b0 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
>      };
>
>      unsigned int cpu;
> -    unsigned int pa_range = 0x10; /* Larger than any possible value */
> +    unsigned int pa_range = ARRAY_SIZE(pa_range_info) - 1;

The previous value was confusing and I think this one is even more.

But this is not really the problem, it is because the boundary check the 
later on is wrong:

if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )

It will only check whether bit 3 is not set. But we want to check that 
pa_range is the range of the array. I.e

pa_range < ARRAY_SIZE(pa_range_info)

If you still want to change the pa_range initial value, then I would 
prefer to see the boot CPU one (i.e boot_cpu_data.mm64.pa_range).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] fix potential pa_range_info out of bound access

[Stefano Stabellini]

On Fri, 9 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/12/16 01:40, Stefano Stabellini wrote:
> > On Thu, 8 Dec 2016, Stefano Stabellini wrote:
> > > pa_range_info has only 8 elements and is accessed using pa_range as
> > > index. pa_range is initialized to 16, potentially causing out of bound
> > > access errors. Fix the issue by initializing pa_range to the effective
> > > number of pa_range_info elements.
> > > 
> > > CID 1381865
> > > 
> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > 
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index e4991df..245fcd1 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
> > >      };
> > > 
> > >      unsigned int cpu;
> > > -    unsigned int pa_range = 0x10; /* Larger than any possible value */
> > > +    unsigned int pa_range = sizeof(pa_range_info) /
> > > sizeof(pa_range_info[0]);
> > > 
> > >      for_each_online_cpu ( cpu )
> > >      {
> > 
> > this is wrong, it should be sizeof(pa_range_info) / sizeof(pa_range_info[0])
> > - 1:
> > 
> > ---
> > pa_range_info has only 8 elements and is accessed using pa_range as
> > index. pa_range is initialized to 16, potentially causing out of bound
> > access errors. Fix the issue by initializing pa_range to the effective
> > number of pa_range_info elements minus 1.
> > 
> > Coverity-ID: 1381865
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index e4991df..14901b0 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1629,7 +1629,7 @@ void __init setup_virt_paging(void)
> >      };
> > 
> >      unsigned int cpu;
> > -    unsigned int pa_range = 0x10; /* Larger than any possible value */
> > +    unsigned int pa_range = ARRAY_SIZE(pa_range_info) - 1;
> 
> The previous value was confusing and I think this one is even more.
> 
> But this is not really the problem, it is because the boundary check the later
> on is wrong:
> 
> if ( pa_range&0x8 || !pa_range_info[pa_range].pabits )
> 
> It will only check whether bit 3 is not set. But we want to check that
> pa_range is the range of the array. I.e
> 
> pa_range < ARRAY_SIZE(pa_range_info)

You are right, that is better and I don't think it requires changing the
initial value. Andrew suggested something similar on IRC too.


> If you still want to change the pa_range initial value, then I would prefer to
> see the boot CPU one (i.e boot_cpu_data.mm64.pa_range).


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel