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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.13 at 19:56, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxx>
>>>>> Signed-off-by: Tim Deegan <tim@xxxxxxx>
>>>>>
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -2278,7 +2278,7 @@ static int hvm_load_segment_selector(
>>>>>              if ( !(desc.b & (1u<<11)) )
>>>>>                  goto unmap_and_fail;
>>>>>              /* Non-conforming segment: check DPL against RPL. */
>>>>> -            if ( ((desc.b & (6u<<9)) != 6) && (dpl != rpl) )
>>>>> +            if ( !(desc.b & (1u<<10)) && (dpl != rpl) )
>>>>>                  goto unmap_and_fail;
>>>>>              break;
>>>>>          case x86_seg_ss:
>>>>>
>>>> There is another example higher in the switch statement for the code
>>>> segment selector.
>>>>
>>>> Also, the commit should probably have CID 1055180 referenced ?
>>> Sure.  here's v2:
>> ...which was buggy: one path needs to handle data segments too.  v3:
>>
>> commit 8f8b746cfdcc11197c91efea2b4414045e846fa3
>> Author: Tim Deegan <tim@xxxxxxx>
>> Date:   Thu Nov 21 15:11:39 2013 +0000
>>
>>     x86/hvm: fix test for non-conforming segments.
>>     
>>     Also Coverity CID 1055180
>>     
>>     Reported-by: David Binderman <dcb314@xxxxxxxxxxx>
>>     Signed-off-by: Tim Deegan <tim@xxxxxxx>
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 3b353ec..d64f635 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2278,7 +2278,7 @@ static int hvm_load_segment_selector(
>>              if ( !(desc.b & (1u<<11)) )
>>                  goto unmap_and_fail;
>>              /* Non-conforming segment: check DPL against RPL. */
>> -            if ( ((desc.b & (6u<<9)) != 6) && (dpl != rpl) )
>> +            if ( !(desc.b & (1u<<10)) && (dpl != rpl) )
>>                  goto unmap_and_fail;
>>              break;
>>          case x86_seg_ss:
>> @@ -2298,7 +2298,8 @@ static int hvm_load_segment_selector(
>>              if ( (desc.b & (5u<<9)) == (4u<<9) )
>>                  goto unmap_and_fail;
>>              /* Non-conforming segment: check DPL against RPL and CPL. */
>> -            if ( ((desc.b & (6u<<9)) != 6) && ((dpl < cpl) || (dpl < rpl)) )
>> +            if ( ((desc.b & (3u<<10)) != (3u<<10))
>> +                 && ((dpl < cpl) || (dpl < rpl)) )
>>                  goto unmap_and_fail;
>>              break;
>>          }
> 
> 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

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