From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDY06-0004G1-Lz for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:33:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDY01-0001KC-6y for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:33:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38643) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDY01-0001Ij-0L for qemu-devel@nongnu.org; Tue, 20 Jan 2015 07:33:37 -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> Date: Tue, 20 Jan 2015 13:33:27 +0100 In-Reply-To: <20150120110348.66afc423.cornelia.huck@de.ibm.com> (Cornelia Huck's message of "Tue, 20 Jan 2015 11:03:48 +0100") Message-ID: <87ppa9iqco.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 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. Regarding the leak, I prefer my patch, because it avoids the free on error. But you're the maintainer.