* 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
* [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 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
* 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.