On 21 January 2015 at 11:54, Markus Armbruster <arm...@redhat.com> wrote:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> Frank Blaschka <blasc...@linux.vnet.ibm.com> writes:
>>
>>> On Tue, Jan 20, 2015 at 01:56:09PM +0100, Markus Armbruster wrote:
>>>> Markus Armbruster <arm...@redhat.com> writes:
>>>> > 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.
>>> uint16_t to int
>>
>> Yes, that's what I meant :)
>>
>>>> >
>>>> > 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.
>>>>
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 5ea13e5..2bed3f5 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -785,8 +785,8 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1,
> uint64_t fiba)
> stq_p(&fib.fmb_addr, pbdev->fmb_addr);
>
> data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> - (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> - pbdev->routes.adapter.summary_offset;
> + ((uint32_t)pbdev->routes.adapter.ind_offset << 8) |
> + (pbdev->sum << 7) | pbdev->routes.adapter.summary_offset;
> stw_p(&fib.data, data);
>
> if (pbdev->fh >> ENABLE_BIT_OFFSET) {
>
This doesn't make sense to me as a fix for the problem you describe
above. Either
(1) pbdev->isc may have bit 3 set: in this case shifting it left
by 28 is undefined behaviour in C, and we must not do it
(and adding a cast to ind_offset doesn't help us at all)
(2) pbdev->isc is guaranteed never to have bit 3 set: in this
case the sign extension to uint64_t in step 6 above will
have no effect, because the sign bit in the int result will
be clear
So you can either:
(1) cast pbdev->isc to uint32_t before shifting, thus ensuring that
we do all our | operations on unsigned types and that we won't
shift into the sign bit regardless of pbdev->isc's value
(2) state that we know pbdev->isc is always less than 8 and so this
is a coverity false positive to be suppressed via the web UI
But the patch you have doesn't seem like the right thing to me.
thanks
-- PMM
