From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59118) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDYM4-000643-J9 for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:56:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDYLz-00010O-IE for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:56:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48194) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDYLz-00010E-AB for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:56:19 -0500 From: Markus Armbruster References: <1420790680-3266-1-git-send-email-blaschka@linux.vnet.ibm.com> <1420790680-3266-3-git-send-email-blaschka@linux.vnet.ibm.com> <87h9vln5tm.fsf@blackfin.pond.sub.org> <20150120110348.66afc423.cornelia.huck@de.ibm.com> <87ppa9iqco.fsf@blackfin.pond.sub.org> Date: Tue, 20 Jan 2015 13:56:09 +0100 In-Reply-To: <87ppa9iqco.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Tue, 20 Jan 2015 13:33:27 +0100") Message-ID: <87bnlthaqe.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: borntraeger@de.ibm.com, Frank Blaschka , Frank Blaschka , qemu-devel@nongnu.org Markus Armbruster writes: > Cornelia Huck writes: > >> On Tue, 20 Jan 2015 10:45:41 +0100 >> Markus Armbruster wrote: >> >>> This patch makes Coverity unhappy: >>> >>> *** CID 1264326: Unintended sign extension (SIGN_EXTENSION) >>> /hw/s390x/s390-pci-inst.c: 787 in stpcifc_service_call() >>> 781 stq_p(&fib.pal, pbdev->pal); >>> 782 stq_p(&fib.iota, pbdev->g_iota); >>> 783 stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr); >>> 784 stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr); >>> 785 stq_p(&fib.fmb_addr, pbdev->fmb_addr); >>> 786 >>> >>> CID 1264326: Unintended sign extension (SIGN_EXTENSION) >>> >>> Suspicious implicit sign extension: "pbdev->isc" with type >>> >>> "unsigned char" (8 bits, unsigned) is promoted in "(pbdev->isc << >>> >>> 28) | (pbdev->noi << 16)" to type "int" (32 bits, signed), then >>> >>> sign-extended to type "unsigned long" (64 bits, unsigned). If >>> >>> "(pbdev->isc << 28) | (pbdev->noi << 16)" is greater than >>> >>> 0x7FFFFFFF, the upper bits of the result will all be 1. >>> 787 data = (pbdev->isc << 28) | (pbdev->noi << 16) | >>> 788 (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) | >>> 789 pbdev->routes.adapter.summary_offset; >>> 790 stw_p(&fib.data, data); >>> 791 >>> 792 if (pbdev->fh >> ENABLE_BIT_OFFSET) { >> >> There's a fix for this (and the memory leak): >> >> http://marc.info/?l=qemu-devel&m=142124886620078&w=2 >> >> The patch is sitting in my queue, will send with the next pile of s390x >> updates. > > I can't see how > > @@ -787,7 +787,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba) > data = (pbdev->isc << 28) | (pbdev->noi << 16) | > (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) | > pbdev->routes.adapter.summary_offset; > - stw_p(&fib.data, data); > + stl_p(&fib.data, data); > > if (pbdev->fh >> ENABLE_BIT_OFFSET) { > fib.fc |= 0x80; > > fixes the implicit sign extension within the assignment preceding it. > Let me explain it again real slow: > > 1. pbdev->isc gets promoted from uint8_t to int as operand of binary << > (usual arithmetic conversions ISO/IEC 9899:1999 6.3.1.8) > > 2. The int result is shifted left 28 bits. This can set the MSB. > > 3. Likewise: pbdev->noi gets promoted from uint64_t to int, and shifted > left 16 bits. > > 4. The two shift results stay int and get ored. > > 5. pbdev->routes.adapter.ind_offset stays uint64_t, and is shifted left > 8 bits. > > 6. The next or's left operand is the int result of 4 and the right > operant is the uint64_t result of 5. Therefore, the left operand is > *sign-extended* from int to uint64_t. This copies bit#7 of > pbdev->isc to bits#31..63. Whoops. I neglected to say: we don't currently use the upper 32 bits, and as long as we do that, the sign extension is harmless. I'd recommend to avoid it all the same, for robustness, and to hush up Coverity. > Regarding the leak, I prefer my patch, because it avoids the free on > error. But you're the maintainer.