[PATCH] xen/x86: Fix build with clang following c/s 4fa0105

[PATCH] xen/x86: Fix build with clang following c/s 4fa0105

[Andrew Cooper]

https://travis-ci.org/xen-project/xen/jobs/158494027#L2344

Clang complains:

  emulate.c:2016:14: error: comparison of unsigned enum expression < 0
  is always false [-Werror,-Wtautological-compare]
      if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
           ~~~ ^ ~

Clang is wrong to raise a warning like this.  The signed-ness of an enum is
implementation defined in C, and robust code must not assume the choices made
by the compiler.

In this case, dropping the < 0 check creates a latent bug which would result
in an array underflow when compiled with a compiler which chooses a signed
enum.

Work around the bug by explicitly pulling seg into an unsigned integer, and
only perform the upper bounds check.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/hvm/emulate.c      | 19 +++++++++++--------
 xen/arch/x86/mm/shadow/common.c |  9 +++++----
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index e3bfda5..cc25676 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1447,13 +1447,14 @@ static int hvmemul_write_segment(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    unsigned int idx = seg;
 
-    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
+    if ( idx >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
         return X86EMUL_UNHANDLEABLE;
 
-    hvmemul_ctxt->seg_reg[seg] = *reg;
-    __set_bit(seg, &hvmemul_ctxt->seg_reg_accessed);
-    __set_bit(seg, &hvmemul_ctxt->seg_reg_dirty);
+    hvmemul_ctxt->seg_reg[idx] = *reg;
+    __set_bit(idx, &hvmemul_ctxt->seg_reg_accessed);
+    __set_bit(idx, &hvmemul_ctxt->seg_reg_dirty);
 
     return X86EMUL_OKAY;
 }
@@ -2012,12 +2013,14 @@ struct segment_register *hvmemul_get_seg_reg(
     enum x86_segment seg,
     struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
-    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
+    unsigned int idx = seg;
+
+    if ( idx >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
         return ERR_PTR(-X86EMUL_UNHANDLEABLE);
 
-    if ( !__test_and_set_bit(seg, &hvmemul_ctxt->seg_reg_accessed) )
-        hvm_get_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
-    return &hvmemul_ctxt->seg_reg[seg];
+    if ( !__test_and_set_bit(idx, &hvmemul_ctxt->seg_reg_accessed) )
+        hvm_get_segment_register(current, idx, &hvmemul_ctxt->seg_reg[idx]);
+    return &hvmemul_ctxt->seg_reg[idx];
 }
 
 static const char *guest_x86_mode_to_str(int mode)
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 8d6661c..21607bf 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -130,14 +130,15 @@ __initcall(shadow_audit_key_init);
 static struct segment_register *hvm_get_seg_reg(
     enum x86_segment seg, struct sh_emulate_ctxt *sh_ctxt)
 {
+    unsigned int idx = seg;
     struct segment_register *seg_reg;
 
-    if ( seg < 0 || seg >= ARRAY_SIZE(sh_ctxt->seg_reg) )
+    if ( idx >= ARRAY_SIZE(sh_ctxt->seg_reg) )
         return ERR_PTR(-X86EMUL_UNHANDLEABLE);
 
-    seg_reg = &sh_ctxt->seg_reg[seg];
-    if ( !__test_and_set_bit(seg, &sh_ctxt->valid_seg_regs) )
-        hvm_get_segment_register(current, seg, seg_reg);
+    seg_reg = &sh_ctxt->seg_reg[idx];
+    if ( !__test_and_set_bit(idx, &sh_ctxt->valid_seg_regs) )
+        hvm_get_segment_register(current, idx, seg_reg);
     return seg_reg;
 }
 
-- 
2.1.4


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

Re: [PATCH] xen/x86: Fix build with clang following c/s 4fa0105

[Jan Beulich]

>>> On 08.09.16 at 20:21, <andrew.cooper3@citrix.com> wrote:
> https://travis-ci.org/xen-project/xen/jobs/158494027#L2344 
> 
> Clang complains:
> 
>   emulate.c:2016:14: error: comparison of unsigned enum expression < 0
>   is always false [-Werror,-Wtautological-compare]
>       if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
>            ~~~ ^ ~
> 
> Clang is wrong to raise a warning like this.  The signed-ness of an enum is
> implementation defined in C, and robust code must not assume the choices made
> by the compiler.

Indeed.

> In this case, dropping the < 0 check creates a latent bug which would result
> in an array underflow when compiled with a compiler which chooses a signed
> enum.
> 
> Work around the bug by explicitly pulling seg into an unsigned integer, and
> only perform the upper bounds check.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH] xen/x86: Fix build with clang following c/s 4fa0105

[George Dunlap]

On 08/09/16 19:21, Andrew Cooper wrote:
> https://travis-ci.org/xen-project/xen/jobs/158494027#L2344
> 
> Clang complains:
> 
>   emulate.c:2016:14: error: comparison of unsigned enum expression < 0
>   is always false [-Werror,-Wtautological-compare]
>       if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
>            ~~~ ^ ~
> 
> Clang is wrong to raise a warning like this.  The signed-ness of an enum is
> implementation defined in C, and robust code must not assume the choices made
> by the compiler.
> 
> In this case, dropping the < 0 check creates a latent bug which would result
> in an array underflow when compiled with a compiler which chooses a signed
> enum.
> 
> Work around the bug by explicitly pulling seg into an unsigned integer, and
> only perform the upper bounds check.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  xen/arch/x86/hvm/emulate.c      | 19 +++++++++++--------
>  xen/arch/x86/mm/shadow/common.c |  9 +++++----
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e3bfda5..cc25676 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1447,13 +1447,14 @@ static int hvmemul_write_segment(
>  {
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +    unsigned int idx = seg;
>  
> -    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
> +    if ( idx >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
>          return X86EMUL_UNHANDLEABLE;
>  
> -    hvmemul_ctxt->seg_reg[seg] = *reg;
> -    __set_bit(seg, &hvmemul_ctxt->seg_reg_accessed);
> -    __set_bit(seg, &hvmemul_ctxt->seg_reg_dirty);
> +    hvmemul_ctxt->seg_reg[idx] = *reg;
> +    __set_bit(idx, &hvmemul_ctxt->seg_reg_accessed);
> +    __set_bit(idx, &hvmemul_ctxt->seg_reg_dirty);
>  
>      return X86EMUL_OKAY;
>  }
> @@ -2012,12 +2013,14 @@ struct segment_register *hvmemul_get_seg_reg(
>      enum x86_segment seg,
>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
> -    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
> +    unsigned int idx = seg;
> +
> +    if ( idx >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
>          return ERR_PTR(-X86EMUL_UNHANDLEABLE);
>  
> -    if ( !__test_and_set_bit(seg, &hvmemul_ctxt->seg_reg_accessed) )
> -        hvm_get_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
> -    return &hvmemul_ctxt->seg_reg[seg];
> +    if ( !__test_and_set_bit(idx, &hvmemul_ctxt->seg_reg_accessed) )
> +        hvm_get_segment_register(current, idx, &hvmemul_ctxt->seg_reg[idx]);
> +    return &hvmemul_ctxt->seg_reg[idx];
>  }
>  
>  static const char *guest_x86_mode_to_str(int mode)
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 8d6661c..21607bf 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -130,14 +130,15 @@ __initcall(shadow_audit_key_init);
>  static struct segment_register *hvm_get_seg_reg(
>      enum x86_segment seg, struct sh_emulate_ctxt *sh_ctxt)
>  {
> +    unsigned int idx = seg;
>      struct segment_register *seg_reg;
>  
> -    if ( seg < 0 || seg >= ARRAY_SIZE(sh_ctxt->seg_reg) )
> +    if ( idx >= ARRAY_SIZE(sh_ctxt->seg_reg) )
>          return ERR_PTR(-X86EMUL_UNHANDLEABLE);
>  
> -    seg_reg = &sh_ctxt->seg_reg[seg];
> -    if ( !__test_and_set_bit(seg, &sh_ctxt->valid_seg_regs) )
> -        hvm_get_segment_register(current, seg, seg_reg);
> +    seg_reg = &sh_ctxt->seg_reg[idx];
> +    if ( !__test_and_set_bit(idx, &sh_ctxt->valid_seg_regs) )
> +        hvm_get_segment_register(current, idx, seg_reg);
>      return seg_reg;
>  }
>  
> 


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