[PATCH] xen: arm: correct off-by-one error in consider_modules

[PATCH] xen: arm: correct off-by-one error in consider_modules

[Ian Campbell]

By iterating up to <= mi->nr_mods we are running off the end of the boot
modules, but more importantly it causes us to then skip the first FDT reserved
region, meaning we might clobber it.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
---
For 4.5: I think this bug fix should go in, it fixes a real issue and is low
risk.

I'll also add to my list of things to consider for backport to 4.4.
---
 xen/arch/arm/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3991d64..f49569d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -286,7 +286,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
         return 0;
 
     /* First check the boot modules */
-    for ( i = first_mod; i <= mi->nr_mods; i++ )
+    for ( i = first_mod; i < mi->nr_mods; i++ )
     {
         paddr_t mod_s = mi->module[i].start;
         paddr_t mod_e = mod_s + mi->module[i].size;
-- 
2.1.3

Re: [PATCH] xen: arm: correct off-by-one error in consider_modules

[Julien Grall]

Hi Ian,

On 21/12/2014 12:18, Ian Campbell wrote:
> By iterating up to <= mi->nr_mods we are running off the end of the boot
> modules, but more importantly it causes us to then skip the first FDT reserved
> region, meaning we might clobber it.

Oops. Good catch!

OOI, how did you find it?

> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>

Reviewed-by: Julien Grall <julien.grall@linaro.org>

> ---
> For 4.5: I think this bug fix should go in, it fixes a real issue and is low
> risk.

Agreed.

> I'll also add to my list of things to consider for backport to 4.4.

Ditto.

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: correct off-by-one error in consider_modules

[Ian Campbell]

On Mon, 2014-12-22 at 11:54 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 21/12/2014 12:18, Ian Campbell wrote:
> > By iterating up to <= mi->nr_mods we are running off the end of the boot
> > modules, but more importantly it causes us to then skip the first FDT reserved
> > region, meaning we might clobber it.
> 
> Oops. Good catch!
> 
> OOI, how did you find it?

U-boot on Jetson was locating the PSCI handling code at the top of RAM
and reserving it in the DT, but Xen was still relocating itself over it,
causing secondary CPU bringup to hang the board.

(This is an experimental u-boot, so it doesn't actually enforce the
security of the PSCI region yet, if it did I'd have expected a fault on
relocation instead of a hang on CPU up).

Ian.

Re: [PATCH] xen: arm: correct off-by-one error in consider_modules

[Konrad Rzeszutek Wilk]

On Mon, Dec 22, 2014 at 11:54:01AM +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 21/12/2014 12:18, Ian Campbell wrote:
> >By iterating up to <= mi->nr_mods we are running off the end of the boot
> >modules, but more importantly it causes us to then skip the first FDT reserved
> >region, meaning we might clobber it.
> 
> Oops. Good catch!
> 
> OOI, how did you find it?
> 
> >Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> 
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> 
> >---
> >For 4.5: I think this bug fix should go in, it fixes a real issue and is low
> >risk.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilK@oracle.com>
> 
> Agreed.
> 
> >I'll also add to my list of things to consider for backport to 4.4.
> 
> Ditto.
> 
> Regards,
> 
> -- 
> Julien Grall

Re: [PATCH] xen: arm: correct off-by-one error in consider_modules

[Ian Campbell]

On Mon, 2015-01-05 at 11:17 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 22, 2014 at 11:54:01AM +0100, Julien Grall wrote:
> > Hi Ian,
> > 
> > On 21/12/2014 12:18, Ian Campbell wrote:
> > >By iterating up to <= mi->nr_mods we are running off the end of the boot
> > >modules, but more importantly it causes us to then skip the first FDT reserved
> > >region, meaning we might clobber it.
> > 
> > Oops. Good catch!
> > 
> > OOI, how did you find it?
> > 
> > >Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> > 
> > Reviewed-by: Julien Grall <julien.grall@linaro.org>
> > 
> > >---
> > >For 4.5: I think this bug fix should go in, it fixes a real issue and is low
> > >risk.
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilK@oracle.com>

Applied, thanks.

Re: [PATCH] xen: arm: correct off-by-one error in consider_modules

[Ian Campbell]

On Sun, 2014-12-21 at 11:18 +0000, Ian Campbell wrote:
> By iterating up to <= mi->nr_mods we are running off the end of the boot
> modules, but more importantly it causes us to then skip the first FDT reserved
> region, meaning we might clobber it.
> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> ---
> For 4.5: I think this bug fix should go in, it fixes a real issue and is low
> risk.
> 
> I'll also add to my list of things to consider for backport to 4.4.

In 4.4 nr_mods was, confusingly, the index of the highest mod present,
not the nr. This changed in e73b7d6619c8 "xen: arm: store per-boot
module type instead of relying on index", which is how this off by one
came to be.

e73b7d6619c8 is not in 4.4, consequently I don't think it would be
correct to backport this either.

Ian.

> ---
>  xen/arch/arm/setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 3991d64..f49569d 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -286,7 +286,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>          return 0;
>  
>      /* First check the boot modules */
> -    for ( i = first_mod; i <= mi->nr_mods; i++ )
> +    for ( i = first_mod; i < mi->nr_mods; i++ )
>      {
>          paddr_t mod_s = mi->module[i].start;
>          paddr_t mod_e = mod_s + mi->module[i].size;