* xen-4.3.1:hvm.c: 2 * possible bad if tests ? @ 2013-11-21 11:45 David Binderman 2013-11-21 11:54 ` Andrew Cooper 0 siblings, 1 reply; 14+ messages in thread From: David Binderman @ 2013-11-21 11:45 UTC (permalink / raw) To: xen-devel@lists.xenproject.org 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; Duplicate. Regards David Binderman ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 11:45 xen-4.3.1:hvm.c: 2 * possible bad if tests ? David Binderman @ 2013-11-21 11:54 ` Andrew Cooper 2013-11-21 15:03 ` [PATCH] " Tim Deegan 0 siblings, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2013-11-21 11:54 UTC (permalink / raw) To: David Binderman; +Cc: xen-devel@lists.xenproject.org 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. ~Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 11:54 ` Andrew Cooper @ 2013-11-21 15:03 ` Tim Deegan 2013-11-21 15:07 ` Andrew Cooper 0 siblings, 1 reply; 14+ messages in thread From: Tim Deegan @ 2013-11-21 15:03 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, keir, David Binderman, jbeulich 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@hotmail.com> Signed-off-by: Tim Deegan <tim@xen.org> --- 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: ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 15:03 ` [PATCH] " Tim Deegan @ 2013-11-21 15:07 ` Andrew Cooper 2013-11-21 15:13 ` Tim Deegan 0 siblings, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2013-11-21 15:07 UTC (permalink / raw) To: Tim Deegan Cc: xen-devel@lists.xenproject.org, keir, David Binderman, jbeulich 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@hotmail.com> > Signed-off-by: Tim Deegan <tim@xen.org> > > --- 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 ? ~Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 15:07 ` Andrew Cooper @ 2013-11-21 15:13 ` Tim Deegan 2013-11-21 15:19 ` Keir Fraser 2013-11-21 15:32 ` Tim Deegan 0 siblings, 2 replies; 14+ messages in thread From: Tim Deegan @ 2013-11-21 15:13 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, keir, jbeulich, David Binderman 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@hotmail.com> > > Signed-off-by: Tim Deegan <tim@xen.org> > > > > --- 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: commit 22c2226e799787ec444ab480db95369d18972cd8 Author: Tim Deegan <tim@xen.org> 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@hotmail.com> Signed-off-by: Tim Deegan <tim@xen.org> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 3b353ec..bbeef53 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,7 @@ 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 & (1u<<10)) && ((dpl < cpl) || (dpl < rpl)) ) goto unmap_and_fail; break; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 15:13 ` Tim Deegan @ 2013-11-21 15:19 ` Keir Fraser 2013-11-21 15:32 ` Tim Deegan 1 sibling, 0 replies; 14+ messages in thread From: Keir Fraser @ 2013-11-21 15:19 UTC (permalink / raw) To: Tim Deegan, Andrew Cooper Cc: xen-devel@lists.xenproject.org, keir, jbeulich, David Binderman On 21/11/2013 07:13, "Tim Deegan" <tim@xen.org> wrote: > commit 22c2226e799787ec444ab480db95369d18972cd8 > Author: Tim Deegan <tim@xen.org> > 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@hotmail.com> > Signed-off-by: Tim Deegan <tim@xen.org> Acked-by: Keir Fraser <keir@xen.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 15:13 ` Tim Deegan 2013-11-21 15:19 ` Keir Fraser @ 2013-11-21 15:32 ` Tim Deegan 2013-11-21 18:56 ` Andrew Cooper 1 sibling, 1 reply; 14+ messages in thread From: Tim Deegan @ 2013-11-21 15:32 UTC (permalink / raw) To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, keir, David Binderman, jbeulich 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@hotmail.com> > > > Signed-off-by: Tim Deegan <tim@xen.org> > > > > > > --- 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@xen.org> 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@hotmail.com> Signed-off-by: Tim Deegan <tim@xen.org> 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; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 15:32 ` Tim Deegan @ 2013-11-21 18:56 ` Andrew Cooper 2013-11-22 11:50 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2013-11-21 18:56 UTC (permalink / raw) To: Tim Deegan Cc: xen-devel@lists.xenproject.org, keir, David Binderman, jbeulich On 21/11/13 15:32, Tim Deegan wrote: > 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@hotmail.com> >>>> Signed-off-by: Tim Deegan <tim@xen.org> >>>> >>>> --- 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@xen.org> > 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@hotmail.com> > Signed-off-by: Tim Deegan <tim@xen.org> > > 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; > } Can you fix the comment to /* Data or non-conforming segment: check DPL against RPL and CPL. */ to match the new logic? ~Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-21 18:56 ` Andrew Cooper @ 2013-11-22 11:50 ` Jan Beulich 2013-11-22 11:54 ` Tim Deegan 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2013-11-22 11:50 UTC (permalink / raw) To: Andrew Cooper, Tim Deegan Cc: xen-devel@lists.xenproject.org, keir, David Binderman >>> On 21.11.13 at 19:56, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 21/11/13 15:32, Tim Deegan wrote: >> 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@hotmail.com> >>>>> Signed-off-by: Tim Deegan <tim@xen.org> >>>>> >>>>> --- 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@xen.org> >> 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@hotmail.com> >> Signed-off-by: Tim Deegan <tim@xen.org> >> >> 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; >> } > > Can you fix the comment to /* Data or non-conforming segment: check DPL > against RPL and CPL. */ to match the new logic? And ideally use _SEGMENT_* instead of raw numbers... Jan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? 2013-11-22 11:50 ` Jan Beulich @ 2013-11-22 11:54 ` Tim Deegan 2013-11-22 14:20 ` [PATCH v4 1/2] x86/hvm: fix segment validation Jan Beulich 2013-11-22 14:21 ` [PATCH v4 2/2] x86/hvm: clean up " Jan Beulich 0 siblings, 2 replies; 14+ messages in thread From: Tim Deegan @ 2013-11-22 11:54 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, keir, David Binderman, xen-devel@lists.xenproject.org At 11:50 +0000 on 22 Nov (1385117445), Jan Beulich wrote: > >>> On 21.11.13 at 19:56, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 21/11/13 15:32, Tim Deegan wrote: > >> 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@hotmail.com> > >>>>> Signed-off-by: Tim Deegan <tim@xen.org> > >>>>> > >>>>> --- 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@xen.org> > >> 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@hotmail.com> > >> Signed-off-by: Tim Deegan <tim@xen.org> > >> > >> 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; > >> } > > > > Can you fix the comment to /* Data or non-conforming segment: check DPL > > against RPL and CPL. */ to match the new logic? Yes. > And ideally use _SEGMENT_* instead of raw numbers... Eh, OK. It'll be next week, then, with a followup to convert the surrounding code too. Tim. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 1/2] x86/hvm: fix segment validation 2013-11-22 11:54 ` Tim Deegan @ 2013-11-22 14:20 ` Jan Beulich 2013-11-22 14:25 ` Andrew Cooper 2013-11-22 14:21 ` [PATCH v4 2/2] x86/hvm: clean up " Jan Beulich 1 sibling, 1 reply; 14+ messages in thread From: Jan Beulich @ 2013-11-22 14:20 UTC (permalink / raw) To: xen-devel@lists.xenproject.org Cc: Andrew Cooper, keir, David Binderman, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 1459 bytes --] Also Coverity CID 1055180. Reported-by: David Binderman <dcb314@hotmail.com> Signed-off-by: Tim Deegan <tim@xen.org> Use _SEGMENT_* instead of plain numbers and adjust a comment. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2280,7 +2280,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 & _SEGMENT_EC) && (dpl != rpl) ) goto unmap_and_fail; break; case x86_seg_ss: @@ -2299,8 +2299,13 @@ static int hvm_load_segment_selector( /* Readable code or data segment? */ 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)) ) + /* + * Data or non-conforming code segment: + * check DPL against RPL and CPL. + */ + if ( ((desc.b & (_SEGMENT_EC|_SEGMENT_CODE)) != + (_SEGMENT_EC|_SEGMENT_CODE)) + && ((dpl < cpl) || (dpl < rpl)) ) goto unmap_and_fail; break; } [-- Attachment #2: x86-HVM-segment-validation.patch --] [-- Type: text/plain, Size: 1492 bytes --] x86/hvm: fix segment validation Also Coverity CID 1055180. Reported-by: David Binderman <dcb314@hotmail.com> Signed-off-by: Tim Deegan <tim@xen.org> Use _SEGMENT_* instead of plain numbers and adjust a comment. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2280,7 +2280,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 & _SEGMENT_EC) && (dpl != rpl) ) goto unmap_and_fail; break; case x86_seg_ss: @@ -2299,8 +2299,13 @@ static int hvm_load_segment_selector( /* Readable code or data segment? */ 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)) ) + /* + * Data or non-conforming code segment: + * check DPL against RPL and CPL. + */ + if ( ((desc.b & (_SEGMENT_EC|_SEGMENT_CODE)) != + (_SEGMENT_EC|_SEGMENT_CODE)) + && ((dpl < cpl) || (dpl < rpl)) ) goto unmap_and_fail; break; } [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] x86/hvm: fix segment validation 2013-11-22 14:20 ` [PATCH v4 1/2] x86/hvm: fix segment validation Jan Beulich @ 2013-11-22 14:25 ` Andrew Cooper 0 siblings, 0 replies; 14+ messages in thread From: Andrew Cooper @ 2013-11-22 14:25 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel@lists.xenproject.org, keir, David Binderman, Tim Deegan On 22/11/13 14:20, Jan Beulich wrote: > Also Coverity CID 1055180. > > Reported-by: David Binderman <dcb314@hotmail.com> > Signed-off-by: Tim Deegan <tim@xen.org> > > Use _SEGMENT_* instead of plain numbers and adjust a comment. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2280,7 +2280,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 & _SEGMENT_EC) && (dpl != rpl) ) > goto unmap_and_fail; > break; > case x86_seg_ss: > @@ -2299,8 +2299,13 @@ static int hvm_load_segment_selector( > /* Readable code or data segment? */ > 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)) ) > + /* > + * Data or non-conforming code segment: > + * check DPL against RPL and CPL. > + */ > + if ( ((desc.b & (_SEGMENT_EC|_SEGMENT_CODE)) != > + (_SEGMENT_EC|_SEGMENT_CODE)) > + && ((dpl < cpl) || (dpl < rpl)) ) > goto unmap_and_fail; > break; > } > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] x86/hvm: clean up segment validation 2013-11-22 11:54 ` Tim Deegan 2013-11-22 14:20 ` [PATCH v4 1/2] x86/hvm: fix segment validation Jan Beulich @ 2013-11-22 14:21 ` Jan Beulich 2013-11-22 14:27 ` Andrew Cooper 1 sibling, 1 reply; 14+ messages in thread From: Jan Beulich @ 2013-11-22 14:21 UTC (permalink / raw) To: xen-devel@lists.xenproject.org Cc: Andrew Cooper, keir, David Binderman, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 1859 bytes --] Use _SEGMENT_* instead of plain numbers where feasible. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2259,7 +2259,7 @@ static int hvm_load_segment_selector( desc = *pdesc; /* Segment present in memory? */ - if ( !(desc.b & (1u<<15)) ) + if ( !(desc.b & _SEGMENT_P) ) { fault_type = TRAP_no_segment; goto unmap_and_fail; @@ -2277,7 +2277,7 @@ static int hvm_load_segment_selector( { case x86_seg_cs: /* Code segment? */ - if ( !(desc.b & (1u<<11)) ) + if ( !(desc.b & _SEGMENT_CODE) ) goto unmap_and_fail; /* Non-conforming segment: check DPL against RPL. */ if ( !(desc.b & _SEGMENT_EC) && (dpl != rpl) ) @@ -2285,19 +2285,19 @@ static int hvm_load_segment_selector( break; case x86_seg_ss: /* Writable data segment? */ - if ( (desc.b & (5u<<9)) != (1u<<9) ) + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) != _SEGMENT_WR ) goto unmap_and_fail; if ( (dpl != cpl) || (dpl != rpl) ) goto unmap_and_fail; break; case x86_seg_ldtr: /* LDT system segment? */ - if ( (desc.b & (15u<<8)) != (2u<<8) ) + if ( (desc.b & _SEGMENT_TYPE) != (2u<<8) ) goto unmap_and_fail; goto skip_accessed_flag; default: /* Readable code or data segment? */ - if ( (desc.b & (5u<<9)) == (4u<<9) ) + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) == _SEGMENT_CODE ) goto unmap_and_fail; /* * Data or non-conforming code segment: [-- Attachment #2: x86-HVM-segment-validation-constants.patch --] [-- Type: text/plain, Size: 1897 bytes --] x86/hvm: clean up segment validation Use _SEGMENT_* instead of plain numbers where feasible. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2259,7 +2259,7 @@ static int hvm_load_segment_selector( desc = *pdesc; /* Segment present in memory? */ - if ( !(desc.b & (1u<<15)) ) + if ( !(desc.b & _SEGMENT_P) ) { fault_type = TRAP_no_segment; goto unmap_and_fail; @@ -2277,7 +2277,7 @@ static int hvm_load_segment_selector( { case x86_seg_cs: /* Code segment? */ - if ( !(desc.b & (1u<<11)) ) + if ( !(desc.b & _SEGMENT_CODE) ) goto unmap_and_fail; /* Non-conforming segment: check DPL against RPL. */ if ( !(desc.b & _SEGMENT_EC) && (dpl != rpl) ) @@ -2285,19 +2285,19 @@ static int hvm_load_segment_selector( break; case x86_seg_ss: /* Writable data segment? */ - if ( (desc.b & (5u<<9)) != (1u<<9) ) + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) != _SEGMENT_WR ) goto unmap_and_fail; if ( (dpl != cpl) || (dpl != rpl) ) goto unmap_and_fail; break; case x86_seg_ldtr: /* LDT system segment? */ - if ( (desc.b & (15u<<8)) != (2u<<8) ) + if ( (desc.b & _SEGMENT_TYPE) != (2u<<8) ) goto unmap_and_fail; goto skip_accessed_flag; default: /* Readable code or data segment? */ - if ( (desc.b & (5u<<9)) == (4u<<9) ) + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) == _SEGMENT_CODE ) goto unmap_and_fail; /* * Data or non-conforming code segment: [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] x86/hvm: clean up segment validation 2013-11-22 14:21 ` [PATCH v4 2/2] x86/hvm: clean up " Jan Beulich @ 2013-11-22 14:27 ` Andrew Cooper 0 siblings, 0 replies; 14+ messages in thread From: Andrew Cooper @ 2013-11-22 14:27 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel@lists.xenproject.org, keir, David Binderman, Tim Deegan On 22/11/13 14:21, Jan Beulich wrote: > Use _SEGMENT_* instead of plain numbers where feasible. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2259,7 +2259,7 @@ static int hvm_load_segment_selector( > desc = *pdesc; > > /* Segment present in memory? */ > - if ( !(desc.b & (1u<<15)) ) > + if ( !(desc.b & _SEGMENT_P) ) > { > fault_type = TRAP_no_segment; > goto unmap_and_fail; > @@ -2277,7 +2277,7 @@ static int hvm_load_segment_selector( > { > case x86_seg_cs: > /* Code segment? */ > - if ( !(desc.b & (1u<<11)) ) > + if ( !(desc.b & _SEGMENT_CODE) ) > goto unmap_and_fail; > /* Non-conforming segment: check DPL against RPL. */ > if ( !(desc.b & _SEGMENT_EC) && (dpl != rpl) ) > @@ -2285,19 +2285,19 @@ static int hvm_load_segment_selector( > break; > case x86_seg_ss: > /* Writable data segment? */ > - if ( (desc.b & (5u<<9)) != (1u<<9) ) > + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) != _SEGMENT_WR ) > goto unmap_and_fail; > if ( (dpl != cpl) || (dpl != rpl) ) > goto unmap_and_fail; > break; > case x86_seg_ldtr: > /* LDT system segment? */ > - if ( (desc.b & (15u<<8)) != (2u<<8) ) > + if ( (desc.b & _SEGMENT_TYPE) != (2u<<8) ) > goto unmap_and_fail; > goto skip_accessed_flag; > default: > /* Readable code or data segment? */ > - if ( (desc.b & (5u<<9)) == (4u<<9) ) > + if ( (desc.b & (_SEGMENT_CODE|_SEGMENT_WR)) == _SEGMENT_CODE ) > goto unmap_and_fail; > /* > * Data or non-conforming code segment: > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-11-22 14:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-21 11:45 xen-4.3.1:hvm.c: 2 * possible bad if tests ? David Binderman 2013-11-21 11:54 ` Andrew Cooper 2013-11-21 15:03 ` [PATCH] " Tim Deegan 2013-11-21 15:07 ` Andrew Cooper 2013-11-21 15:13 ` Tim Deegan 2013-11-21 15:19 ` Keir Fraser 2013-11-21 15:32 ` Tim Deegan 2013-11-21 18:56 ` Andrew Cooper 2013-11-22 11:50 ` Jan Beulich 2013-11-22 11:54 ` Tim Deegan 2013-11-22 14:20 ` [PATCH v4 1/2] x86/hvm: fix segment validation Jan Beulich 2013-11-22 14:25 ` Andrew Cooper 2013-11-22 14:21 ` [PATCH v4 2/2] x86/hvm: clean up " Jan Beulich 2013-11-22 14:27 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.