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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Cornelia Huck <address@hidden> writes:
>
>> On Tue, 20 Jan 2015 10:45:41 +0100
>> Markus Armbruster <address@hidden> 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.