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 15:07:07 +0000 Message-ID: <528E219B.6060801@citrix.com> References: <528DF45E.7050905@citrix.com> <20131121150355.GC89770@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VjVqc-00008k-L2 for xen-devel@lists.xenproject.org; Thu, 21 Nov 2013 15:07:14 +0000 In-Reply-To: <20131121150355.GC89770@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: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 ? ~Andrew