From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] Re: xen-4.3.1:hvm.c: 2 * possible bad if tests ? Date: Thu, 21 Nov 2013 18:56:19 +0000 Message-ID: <528E5753.5020104@citrix.com> References: <528DF45E.7050905@citrix.com> <20131121150355.GC89770@deinos.phlegethon.org> <528E219B.6060801@citrix.com> <20131121151308.GD89770@deinos.phlegethon.org> <20131121153231.GE89770@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VjZQN-0000pS-Rc for xen-devel@lists.xenproject.org; Thu, 21 Nov 2013 18:56:24 +0000 In-Reply-To: <20131121153231.GE89770@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: "xen-devel@lists.xenproject.org" , keir@xen.org, David Binderman , jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org 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 >>>> Signed-off-by: Tim Deegan >>>> >>>> --- 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 > 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 > Signed-off-by: Tim Deegan > > 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