[PATCH] xen: arm: Avoid reading beyond the last module

[PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().

Signed-off-by: Chris Brand <chris.brand@broadcom.com>
---
 xen/arch/arm/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    for ( i = 0; i <= mi->nr_mods; i++ )
+    for ( i = 0; i < mi->nr_mods; i++ )
     {
         paddr_t s = mi->module[i].start;
         paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
     paddr_t lowest = ~(paddr_t)0;
     int i;
 
-    for ( i = 0; i <= mi->nr_mods; i++ )
+    for ( i = 0; i < mi->nr_mods; i++ )
     {
         paddr_t mod_s = mi->module[i].start;
         paddr_t mod_e = mod_s + mi->module[i].size;
-- 
1.9.1

Re: [PATCH] xen: arm: Avoid reading beyond the last module

[Julien Grall]

Hi Chris,

On 17/07/15 21:48, Chris (Christopher) Brand wrote:
> nr_mods is set in add_boot_module() to the number of module
> array elements used. This function also ensures that nr_mods
> never exceeds MAX_MODULES (the size of the array). When looping
> through the array, the correct maximum index is "nr_mods-1",
> not "nr_mods". If the array is full, using the latter will in
> fact access beyond the end of the array.
> This was done correctly in boot_module_find_by_kind() and
> consider_modules() but incorrectly in discard_initial_modules()
> and next_module().
> 
> Signed-off-by: Chris Brand <chris.brand@broadcom.com>

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: Avoid reading beyond the last module

[Ian Campbell]

On Mon, 2015-07-20 at 13:32 +0100, Julien Grall wrote:
> Hi Chris,
> 
> On 17/07/15 21:48, Chris (Christopher) Brand wrote:
> > nr_mods is set in add_boot_module() to the number of module
> > array elements used. This function also ensures that nr_mods
> > never exceeds MAX_MODULES (the size of the array). When looping
> > through the array, the correct maximum index is "nr_mods-1",
> > not "nr_mods". If the array is full, using the latter will in
> > fact access beyond the end of the array.
> > This was done correctly in boot_module_find_by_kind() and
> > consider_modules() but incorrectly in discard_initial_modules()
> > and next_module().
> > 
> > Signed-off-by: Chris Brand <chris.brand@broadcom.com>
> 
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Acked + applied.

Care should be taken when backporting since I think this off-by-one was
the result of us previously not including Xen in nr_mods despite it
being in the array or something like that (i..e the off-by-one used to
be correct).

Ian.

Re: [PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

> It looks like the mail as been sent in HTML. Can you resend it in plain text?

Re-sent, hopefully correctly this time.

Chris

Re: [PATCH] xen: arm: Avoid reading beyond the last module

[Julien Grall]

Hi Christopher,

Thank you for the patch.

It looks like the mail as been sent in HTML. Can you resend it in plain
text?

You also need to cc the maintainers of the code you are modifying (I've
CCed them this time).

You can give a look to [1] to know how to send correctly the patch.


On 16/07/15 22:41, Chris (Christopher) Brand wrote:
> nr_mods is set in add_boot_module() to the number of module
> 
> array elements used. This function also ensures that nr_mods
> 
> never exceeds MAX_MODULES (the size of the array). When looping
> 
> through the array, the correct maximum index is "nr_mods-1",
> 
> not "nr_mods". If the array is full, using the latter will in
> 
> fact access beyond the end of the array.
> 
> This was done correctly in boot_module_find_by_kind() and
> 
> consider_modules() but incorrectly in discard_initial_modules()
> 
> and next_module().
> 
>  
> 
> Signed-off-by: Chris Brand <chris.brand@broadcom.com>


The patch looks good to me. I think it's a candidate to backport in Xen 4.5.

Regards,


[1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

-- 
Julien Grall

[PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

[-- Attachment #1.1: Type: text/plain, Size: 1458 bytes --]

nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().

Signed-off-by: Chris Brand <chris.brand@broadcom.com>
---
xen/arch/arm/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
     struct bootmodules *mi = &bootinfo.modules;
     int i;
-    for ( i = 0; i <= mi->nr_mods; i++ )
+    for ( i = 0; i < mi->nr_mods; i++ )
     {
         paddr_t s = mi->module[i].start;
         paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
     paddr_t lowest = ~(paddr_t)0;
     int i;
-    for ( i = 0; i <= mi->nr_mods; i++ )
+    for ( i = 0; i < mi->nr_mods; i++ )
     {
         paddr_t mod_s = mi->module[i].start;
         paddr_t mod_e = mod_s + mi->module[i].size;
--
1.9.1




[-- Attachment #1.2: Type: text/html, Size: 8513 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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