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
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
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:
Andrew Cooper
2013-Nov-21 15:07 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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
Tim Deegan
2013-Nov-21 15:13 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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; }
Keir Fraser
2013-Nov-21 15:19 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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>
Tim Deegan
2013-Nov-21 15:32 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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; }
Andrew Cooper
2013-Nov-21 18:56 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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
Jan Beulich
2013-Nov-22 11:50 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
>>> 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
Tim Deegan
2013-Nov-22 11:54 UTC
Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ?
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.
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;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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; > } > > >
Andrew Cooper
2013-Nov-22 14:27 UTC
Re: [PATCH v4 2/2] x86/hvm: clean up segment validation
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: > > >