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

[Tim Deegan <tim@xxxxxxx>]

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:


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