Re: [Xen-devel] [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?

[Tim Deegan <tim@xxxxxxx>]

At 16:13 +0100 on 21 Nov (1385046788), Tim Deegan wrote:
> At 15:07 +0000 on 21 Nov (1385042827), Andrew Cooper wrote:
> > On 21/11/13 15:03, Tim Deegan wrote:
> > > At 11:54 +0000 on 21 Nov (1385031246), Andrew Cooper wrote:
> > >> On 21/11/13 11:45, David Binderman wrote:
> > >>> Hello there,
> > >>>
> > >>> I just ran the source code of xen-4.3.1 through the static analyser 
> > >>> "cppcheck".
> > >>>
> > >>> It said
> > >>>
> > >>> 1.
> > >>>
> > >>> [hvm.c:2190]: (style) Expression '(X & 0xc00) != 0x6' is always true.
> > >>>
> > >>> Source code is
> > >>>
> > >>>             if ( ((desc.b & (6u<<9)) != 6) && (dpl != rpl) )
> > >>>                 goto unmap_and_fail;
> > >>>
> > >>> You might be better off with
> > >>>
> > >>>             if ( ((desc.b & (6u<<9))) && (dpl != rpl) )
> > >>>                 goto unmap_and_fail;
> > >>>
> > >>> 2.
> > >>>
> > >>> [hvm.c:2210]: (style) Expression '(X & 0xc00) != 0x6' is always true.
> > >>>
> > >>> Source code is
> > >>>
> > >>>             if ( ((desc.b & (6u<<9)) != 6) && ((dpl < cpl) || (dpl < 
> > >>> rpl)) )
> > >>>                 goto unmap_and_fail;
> > >> These have both been flagged up by our Coverity scanning, but I haven't
> > >> had enough time to pour over the manuals workout out the correct
> > >> expression should be.
> > >>
> > >> The prevailing style for all other checks in this area is "(X & (6u<<9))
> > >> != (6u<<9)" , which is rather different to the result you came up with.
> > >>
> > >> As this is the security checks for segment selectors in the emulation
> > >> code, leaving it in its current "too many operations are failed" is
> > >> safer than being uncertain with the fix and introducing a vulnerability.
> > > Looking at the manual and the comment, I think the right change is:
> > >
> > > x86/hvm: fix test for non-conforming segments.
> > >
> > > Reported-by: David Binderman <dcb314@xxxxxxxxxxx>
> > > Signed-off-by: Tim Deegan <tim@xxxxxxx>
> > >
> > > --- a/xen/arch/x86/hvm/hvm.c
> > > +++ b/xen/arch/x86/hvm/hvm.c
> > > @@ -2278,7 +2278,7 @@ static int hvm_load_segment_selector(
> > >              if ( !(desc.b & (1u<<11)) )
> > >                  goto unmap_and_fail;
> > >              /* Non-conforming segment: check DPL against RPL. */
> > > -            if ( ((desc.b & (6u<<9)) != 6) && (dpl != rpl) )
> > > +            if ( !(desc.b & (1u<<10)) && (dpl != rpl) )
> > >                  goto unmap_and_fail;
> > >              break;
> > >          case x86_seg_ss:
> > >
> > 
> > There is another example higher in the switch statement for the code
> > segment selector.
> > 
> > Also, the commit should probably have CID 1055180 referenced ?
> 
> Sure.  here's v2:

...which was buggy: one path needs to handle data segments too.  v3:

commit 8f8b746cfdcc11197c91efea2b4414045e846fa3
Author: Tim Deegan <tim@xxxxxxx>
Date:   Thu Nov 21 15:11:39 2013 +0000

    x86/hvm: fix test for non-conforming segments.
    
    Also Coverity CID 1055180
    
    Reported-by: David Binderman <dcb314@xxxxxxxxxxx>
    Signed-off-by: Tim Deegan <tim@xxxxxxx>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3b353ec..d64f635 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2278,7 +2278,7 @@ static int hvm_load_segment_selector(
             if ( !(desc.b & (1u<<11)) )
                 goto unmap_and_fail;
             /* Non-conforming segment: check DPL against RPL. */
-            if ( ((desc.b & (6u<<9)) != 6) && (dpl != rpl) )
+            if ( !(desc.b & (1u<<10)) && (dpl != rpl) )
                 goto unmap_and_fail;
             break;
         case x86_seg_ss:
@@ -2298,7 +2298,8 @@ static int hvm_load_segment_selector(
             if ( (desc.b & (5u<<9)) == (4u<<9) )
                 goto unmap_and_fail;
             /* Non-conforming segment: check DPL against RPL and CPL. */
-            if ( ((desc.b & (6u<<9)) != 6) && ((dpl < cpl) || (dpl < rpl)) )
+            if ( ((desc.b & (3u<<10)) != (3u<<10))
+                 && ((dpl < cpl) || (dpl < rpl)) )
                 goto unmap_and_fail;
             break;
         }

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