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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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