Re: [Qemu-devel] [PATCH 2/3 V3] s390: implement pci instructions

[Markus Armbruster <armbru@redhat.com>]

Cornelia Huck <cornelia.huck@de.ibm.com> writes:

> On Tue, 20 Jan 2015 10:45:41 +0100
> Markus Armbruster <armbru@redhat.com> 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.