[PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets() failure path

[PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets() failure path

[Andrew Cooper]

Coverity-ID: 1220092 "Unsigned compare against 0"
Coverity-ID: 1220093 "Out-of-bounds read"

Both of these are cased by the the while() loop in the fail path, which
results in an infinite loop and memory corruption from rangeset_destroy().

Move hvm_ioreq_server_free_rangesets() up and use it for cleanup on the
failure path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |   27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4f993f4..1f13329 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -824,6 +824,18 @@ static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
     }
 }
 
+static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
+                                            bool_t is_default)
+{
+    unsigned int i;
+
+    if ( is_default )
+        return;
+
+    for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
+        rangeset_destroy(s->range[i]);
+}
+
 static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s, 
                                             bool_t is_default)
 {
@@ -861,24 +873,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
     return 0;
 
  fail:
-    while ( --i >= 0 )
-        rangeset_destroy(s->range[i]);
+    hvm_ioreq_server_free_rangesets(s, 0);
 
     return rc;
 }
 
-static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s, 
-                                            bool_t is_default)
-{
-    unsigned int i;
-
-    if ( is_default )
-        return;
-
-    for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
-        rangeset_destroy(s->range[i]);
-}
-
 static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
                                     bool_t is_default)
 {
-- 
1.7.10.4

Re: [PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets() failure path

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 04 June 2014 12:01
> To: Xen-devel
> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich; Paul Durrant
> Subject: [PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets()
> failure path
> 
> Coverity-ID: 1220092 "Unsigned compare against 0"
> Coverity-ID: 1220093 "Out-of-bounds read"
> 
> Both of these are cased by the the while() loop in the fail path, which
> results in an infinite loop and memory corruption from rangeset_destroy().
> 
> Move hvm_ioreq_server_free_rangesets() up and use it for cleanup on the
> failure path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c |   27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 4f993f4..1f13329 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -824,6 +824,18 @@ static void hvm_ioreq_server_unmap_pages(struct
> hvm_ioreq_server *s,
>      }
>  }
> 
> +static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
> +                                            bool_t is_default)
> +{
> +    unsigned int i;
> +
> +    if ( is_default )
> +        return;
> +
> +    for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
> +        rangeset_destroy(s->range[i]);
> +}
> +
>  static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>                                              bool_t is_default)
>  {
> @@ -861,24 +873,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct
> hvm_ioreq_server *s,
>      return 0;
> 
>   fail:
> -    while ( --i >= 0 )
> -        rangeset_destroy(s->range[i]);
> +    hvm_ioreq_server_free_rangesets(s, 0);
> 
>      return rc;
>  }
> 
> -static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
> -                                            bool_t is_default)
> -{
> -    unsigned int i;
> -
> -    if ( is_default )
> -        return;
> -
> -    for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
> -        rangeset_destroy(s->range[i]);
> -}
> -
>  static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
>                                      bool_t is_default)
>  {
> --
> 1.7.10.4

Re: [PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets() failure path

[Jan Beulich]

>>> On 04.06.14 at 13:00, <andrew.cooper3@citrix.com> wrote:
> Coverity-ID: 1220092 "Unsigned compare against 0"
> Coverity-ID: 1220093 "Out-of-bounds read"
> 
> Both of these are cased by the the while() loop in the fail path, which
> results in an infinite loop and memory corruption from rangeset_destroy().
> 
> Move hvm_ioreq_server_free_rangesets() up and use it for cleanup on the
> failure path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Since you and Paul appear to agree to fix it that way, it'll go in as is,
but ...

> @@ -861,24 +873,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>      return 0;
>  
>   fail:
> -    while ( --i >= 0 )

... the one line equivalent fix would have been to use while ( i-- ).

Jan

Re: [PATCH] x86/hvm: Correct hvm_ioreq_server_alloc_rangesets() failure path

[Andrew Cooper]

On 04/06/14 13:22, Jan Beulich wrote:
>
>> @@ -861,24 +873,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>>      return 0;
>>  
>>   fail:
>> -    while ( --i >= 0 )
> ... the one line equivalent fix would have been to use while ( i-- ).
>
> Jan

I was considering the possible case where alloc/free_rangesets() gain
further moving parts, although I guess is probably unlikely.

~Andrew