linux-kernel - Re: ibmvtpm byteswapping inconsistency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <d2da4b82-9686-187c-c514-96a506e994cc@linux.vnet.ibm.com>
Date:   Mon, 30 Jan 2017 12:34:45 -0800
From:   Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Michal Suchanek <hramrach@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:     Marcel Selhorst <tpmdd@...horst.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        tpmdd-devel@...ts.sourceforge.net,
        Paul Mackerras <paulus@...ba.org>,
        Ashley Lai <ashleydlai@...il.com>,
        Peter Huewe <peterhuewe@....de>,
        Michal Suchánek <msuchanek@...e.de>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: ibmvtpm byteswapping inconsistency

On 01/29/2017 08:32 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@...ux.vnet.ibm.com> writes:
> 
>> On 01/27/2017 01:03 AM, Michal Suchanek wrote:
>>> On 27 January 2017 at 02:50, Benjamin Herrenschmidt
>>> <benh@...nel.crashing.org> wrote:
>>>> On Thu, 2017-01-26 at 17:42 -0800, Tyrel Datwyler wrote:
>>>>> On 01/26/2017 12:22 PM, Michal Suchánek wrote:
>>>>>> Hello,
>>>>>>
>>>>>> building ibmvtpm I noticed gcc warning complaining that second word
>>>>>> of
>>>>>> struct ibmvtpm_crq in tpm_ibmvtpm_suspend is uninitialized.
>>>>>>
>>>>>> The structure is defined as
>>>>>>
>>>>>> struct ibmvtpm_crq {
>>>>>>         u8 valid;
>>>>>>         u8 msg;
>>>>>>         __be16 len;
>>>>>>         __be32 data;
>>>>>>         __be64 reserved;
>>>>>> } __attribute__((packed, aligned(8)));
>>>>>>
>>>>>> initialized as
>>>>>>
>>>>>>         struct ibmvtpm_crq crq;
>>>>>>         u64 *buf = (u64 *) &crq;
>>>>>> ...
>>>>>>         crq.valid = (u8)IBMVTPM_VALID_CMD;
>>>>>>         crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
>>>>>>
>>>>>> and submitted with
>>>>>>
>>>>>>         rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
>>>>>>                               cpu_to_be64(buf[1]));
>>>>>
>>>>> These should be be64_to_cpu() here. The underlying hcall made by
>>>>> ibmvtpm_send_crq() requires parameters to be in cpu endian unlike the
>>>>> RTAS interface which requires data in BE.
>>>>
>>>> Hrm... an hcall takes register arguments. Register arguments don't have
>>>> an endianness.
>>>>
>>>> The problem is that we are packing an in-memory structure into 2
>>>> registers and it's expected that this structure is laid out in the
>>>> registers as if it had been loaded by a BE CPU.
>>>>
>>>> So we have two things at play here:
>>>>
>>>>   - The >8-bit fields should be laid out BE in the memory image
>>>>   - That whole 128-bit structure should be loaded into 2 64-bit
>>>> registers MSB first.
>>>>
>>>> So the "double" swap is somewhat needed. The uglyness comes from the
>>>> passing-by-register of the h-call but it should work.
>>>>
>>>> That said, be64_to_cpup(buf) and be64_to_cpup(buf+1) might give you
>>>> better result (though recent gcc's might not make a difference).
>>>
>>> If this should work then the below case that swaps the fields separately is
>>> broken.
>>>
>>> Anyway, structures have no endianess so when they start with a byte they
>>> start with that byte no matter the host endian.
>>> crq.valid is the first byte always. And then each field is to be swapped
>>> separately.
>>>
>>> On the other hand, bitfields are part of an integer and the field should be
>>> swapped as part of the integer.
>>>
>>> That is,
>>> #define CRQ_VALID ((buf[0] >> 56) & 0xff)
>>> CRQ_VALID is part of an integer in buf and would be laid out differently
>>> on start or end depending on the host being BE or LE.
>>>
>>> And the question is what the PAPR actually defines because both ways are
>>> used in the code. You can describe an in-memory layout either way.
>>
>> Byte  |   0   |   1   |   2   |   3   |   4   |   5   |   6   |   7
>> -----------------------------------------------------------------------
>> Word0 | Valid | Type  |     Length    |              Data
>> -----------------------------------------------------------------------
>> Word1 |				Reserved
>> -----------------------------------------------------------------------
>>
>> The following definition looks to match:
>>
>> struct ibmvtpm_crq {
>>         u8 valid;
>>         u8 msg;
>>         __be16 len;
>>         __be32 data;
>>         __be64 reserved;
>> } __attribute__((packed, aligned(8)));
> 
> Well it's a partial match.
> 
> Your layout above doesn't define which byte of Length or Data is the MSB
> or LSB. So going by that we still don't know the endianness of either

I should have been explicit that PAPR uses Big Endian bit and byte
numbering throughout the spec unless otherwise noted.

-Tyrel

> field.
> 
> cheers
>