Re: [PATCH] x86/vLAPIC: adjust types in internal read/write handling

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/06/15 13:55, Jan Beulich wrote:
>>>> On 22.06.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
>> On 22/06/15 12:49, Jan Beulich wrote:
>>> @@ -847,47 +834,41 @@ static int vlapic_write(struct vcpu *v, 
>>>       * According to the IA32 Manual, all accesses should be 32 bits.
>>>       * Some OSes do 8- or 16-byte accesses, however.
>>>       */
>>> -    val = (uint32_t)val;
>>> -    if ( len != 4 )
>>> +    if ( unlikely(len != 4) )
>>>      {
>>> -        unsigned int tmp;
>>> -        unsigned char alignment;
>>> -
>>> -        gdprintk(XENLOG_INFO, "Notice: Local APIC write with len = %lx\n",len);
>>> -
>>> -        alignment = offset & 0x3;
>>> -        (void)vlapic_read_aligned(vlapic, offset & ~0x3, &tmp);
>>> +        unsigned int tmp = vlapic_read_aligned(vlapic, offset & ~3);
>>> +        unsigned char alignment = (offset & 3) * 8;
>>>  
>>>          switch ( len )
>>>          {
>>>          case 1:
>>> -            val = ((tmp & ~(0xff << (8*alignment))) |
>>> -                   ((val & 0xff) << (8*alignment)));
>>> +            val = ((tmp & ~(0xff << alignment)) |
>>> +                   ((val & 0xff) << alignment));
>> These should probably be explicitly unsigned constants, to avoid issues
>> with shifting a 1 into the sign bit.
> I don't see what harm the sign bit would do here - even if the shift
> operation is one on signed int, the & converts the operand to
> unsigned int anyway (and with them being the same size, the
> binary representation doesn't change).

The problem is with 0xff << 24, which where the sign bit will change
given the shift.

If 0xff is interpreted as signed, then shifted, then promoted to
unsigned by the ~ operation, then the result is undefined behaviour
(altering the sign bit of a number with a shift).

If 0xff is interpreted as unsigned straight away, then everything is
fine, as 0xffu << 24 is completely defined behaviour.

>
>>  (I can't quite decide whether 0xff
>> will be interpreted as signed or unsigned, given the integer promotion
>> rules.)
> Literal numbers representable as int will always be "promoted to"
> int.

Which suggested that the code above does demonstrate UB.

~Andrew