[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tpm: fix alignment issues
|
From: |
Marc-Andre Lureau |
|
Subject: |
Re: [Qemu-devel] [PATCH] tpm: fix alignment issues |
|
Date: |
Mon, 29 Jan 2018 18:51:24 +0100 |
Hi
On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger
<address@hidden> wrote:
> On 01/29/2018 07:32 AM, Marc-André Lureau wrote:
>>
>> The new tpm-crb-test fails on sparc host:
>>
>> TEST: tests/tpm-crb-test... (pid=230409)
>> /i386/tpm-crb/test:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02S29cea50247fe1efa59ee885a26d51a85
>> (pid=230423)
>> FAIL: tests/tpm-crb-test
>>
>> and generates a new clang sanitizer runtime warning:
>>
>> /home/petmay01/linaro/qemu-for-merges/hw/tpm/tpm_util.h:36:24: runtime
>> error: load of misaligned address 0x7fdc24c00002 for type 'const
>> uint32_t' (aka 'const unsigned int'), which requires 4 byte alignment
>> 0x7fdc24c00002: note: pointer points here
>> <memory cannot be printed>
>>
>> The sparc architecture does not allow misaligned loads and will
>> segfault if you try them. For example, this function:
>>
>> static inline uint32_t tpm_cmd_get_size(const void *b)
>> {
>> return be32_to_cpu(*(const uint32_t *)(b + 2));
>> }
>>
>> Should read,
>> return ldl_be_p(b + 2);
>>
>> As a general rule you can't take an arbitrary pointer into a byte
>> buffer and try to interpret it as a structure or a pointer to a
>> larger-than-bytesize-data simply by casting the pointer.
>>
>> Use this clean up as an opportunity to remove unnecessary temporary
>> buffers and casts.
>>
>> Reported-by: Peter Maydell <address@hidden>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>> hw/tpm/tpm_util.h | 17 ++++++++++-
>> hw/tpm/tpm_emulator.c | 14 ++++-----
>> hw/tpm/tpm_passthrough.c | 6 ++--
>> hw/tpm/tpm_util.c | 75
>> ++++++++++++++++++++++--------------------------
>> 4 files changed, 58 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
>> index 19b28474ae..c562140e52 100644
>> --- a/hw/tpm/tpm_util.h
>> +++ b/hw/tpm/tpm_util.h
>> @@ -31,9 +31,24 @@ bool tpm_util_is_selftest(const uint8_t *in, uint32_t
>> in_len);
>>
>> int tpm_util_test_tpmdev(int tpm_fd, TPMVersion *tpm_version);
>>
>> +static inline uint16_t tpm_cmd_get_tag(const void *b)
>> +{
>> + return lduw_be_p(b);;
>> +}
>> +
>> static inline uint32_t tpm_cmd_get_size(const void *b)
>> {
>> - return be32_to_cpu(*(const uint32_t *)(b + 2));
>> + return ldl_be_p(b + 2);;
>
>
> Why are there these two ';;' ?
>
obvious typo (repeated..)
>
>> +}
>> +
>> +static inline uint32_t tpm_cmd_get_ordinal(const void *b)
>> +{
>> + return ldl_be_p(b + 6);;
>> +}
>> +
>> +static inline uint32_t tpm_cmd_get_errcode(const void *b)
>> +{
>> + return ldl_be_p(b + 6);;
>> }
>>
>> int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>> index 35c78de5a9..a34a18ac7a 100644
>> --- a/hw/tpm/tpm_emulator.c
>> +++ b/hw/tpm/tpm_emulator.c
>> @@ -120,7 +120,6 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>> *tpm_emu,
>> {
>> ssize_t ret;
>> bool is_selftest = false;
>> - const struct tpm_resp_hdr *hdr = NULL;
>>
>> if (selftest_done) {
>> *selftest_done = false;
>> @@ -132,22 +131,21 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>> *tpm_emu,
>> return -1;
>> }
>>
>> - ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> sizeof(*hdr),
>> - err);
>> + ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> + sizeof(struct tpm_resp_hdr), err);
>> if (ret != 0) {
>> return -1;
>> }
>>
>> - hdr = (struct tpm_resp_hdr *)out;
>> - out += sizeof(*hdr);
>> - ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> - be32_to_cpu(hdr->len) - sizeof(*hdr) ,
>> err);
>> + ret = qio_channel_read_all(tpm_emu->data_ioc,
>> + (char *)out + sizeof(struct tpm_resp_hdr),
>> + tpm_cmd_get_size(out) - sizeof(struct tpm_resp_hdr), err);
>> if (ret != 0) {
>> return -1;
>> }
>>
>> if (is_selftest) {
>> - *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
>> + *selftest_done = tpm_cmd_get_errcode(out) == 0;
>> }
>>
>> return 0;
>> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
>> index 29142f38bb..537e11a3f9 100644
>> --- a/hw/tpm/tpm_passthrough.c
>> +++ b/hw/tpm/tpm_passthrough.c
>> @@ -87,7 +87,6 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState
>> *tpm_pt,
>> {
>> ssize_t ret;
>> bool is_selftest;
>> - const struct tpm_resp_hdr *hdr;
>>
>> /* FIXME: protect shared variables or use other sync mechanism */
>> tpm_pt->tpm_op_canceled = false;
>> @@ -116,15 +115,14 @@ static int
>> tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>> strerror(errno), errno);
>> }
>> } else if (ret < sizeof(struct tpm_resp_hdr) ||
>> - be32_to_cpu(((struct tpm_resp_hdr *)out)->len) != ret) {
>> + tpm_cmd_get_size(out) != ret) {
>> ret = -1;
>> error_report("tpm_passthrough: received invalid response "
>> "packet from TPM");
>> }
>>
>> if (is_selftest && (ret >= sizeof(struct tpm_resp_hdr))) {
>> - hdr = (struct tpm_resp_hdr *)out;
>> - *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
>> + *selftest_done = tpm_cmd_get_errcode(out) == 0;
>> }
>>
>> err_exit:
>> diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
>> index 747075e244..8abde59915 100644
>> --- a/hw/tpm/tpm_util.c
>> +++ b/hw/tpm/tpm_util.c
>> @@ -106,20 +106,16 @@ const PropertyInfo qdev_prop_tpm = {
>> void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
>> {
>> if (out_len >= sizeof(struct tpm_resp_hdr)) {
>> - struct tpm_resp_hdr *resp = (struct tpm_resp_hdr *)out;
>> -
>> - resp->tag = cpu_to_be16(TPM_TAG_RSP_COMMAND);
>> - resp->len = cpu_to_be32(sizeof(struct tpm_resp_hdr));
>> - resp->errcode = cpu_to_be32(TPM_FAIL);
>> + stw_be_p(out, TPM_TAG_RSP_COMMAND);
>> + stl_be_p(out + 2, sizeof(struct tpm_resp_hdr));
>> + stl_be_p(out + 6, TPM_FAIL);
>
>
> Since we wrapped the getters we should wrap the setters as well.
> tpm_cmd_set_len(), tpm_cmd_set_errcode.
They were not as widely used (there was only a getter so far), but
that makes sense too. Could be done on top.
>
>
>> }
>> }
>>
>> bool tpm_util_is_selftest(const uint8_t *in, uint32_t in_len)
>> {
>> - struct tpm_req_hdr *hdr = (struct tpm_req_hdr *)in;
>> -
>> - if (in_len >= sizeof(*hdr)) {
>> - return (be32_to_cpu(hdr->ordinal) == TPM_ORD_ContinueSelfTest);
>> + if (in_len >= sizeof(struct tpm_req_hdr)) {
>> + return tpm_cmd_get_ordinal(in) == TPM_ORD_ContinueSelfTest;
>> }
>>
>> return false;
>> @@ -129,12 +125,11 @@ bool tpm_util_is_selftest(const uint8_t *in,
>> uint32_t in_len)
>> * Send request to a TPM device. We expect a response within one second.
>> */
>> static int tpm_util_request(int fd,
>> - unsigned char *request,
>> + const void *request,
>> size_t requestlen,
>> - unsigned char *response,
>> + void *response,
>> size_t responselen)
>> {
>> - struct tpm_resp_hdr *resp;
>> fd_set readfds;
>> int n;
>> struct timeval tv = {
>> @@ -164,9 +159,8 @@ static int tpm_util_request(int fd,
>> return -EFAULT;
>> }
>>
>> - resp = (struct tpm_resp_hdr *)response;
>> /* check the header */
>> - if (be32_to_cpu(resp->len) != n) {
>> + if (tpm_cmd_get_size(response) != n) {
>> return -EMSGSIZE;
>> }
>>
>> @@ -178,12 +172,11 @@ static int tpm_util_request(int fd,
>> * (error response is fine).
>> */
>> static int tpm_util_test(int fd,
>> - unsigned char *request,
>> + const void *request,
>> size_t requestlen,
>> uint16_t *return_tag)
>> {
>> - struct tpm_resp_hdr *resp;
>> - unsigned char buf[1024];
>> + char buf[1024];
>> ssize_t ret;
>>
>> ret = tpm_util_request(fd, request, requestlen,
>> @@ -192,8 +185,7 @@ static int tpm_util_test(int fd,
>> return ret;
>> }
>>
>> - resp = (struct tpm_resp_hdr *)buf;
>> - *return_tag = be16_to_cpu(resp->tag);
>> + *return_tag = tpm_cmd_get_tag(buf);
>>
>> return 0;
>> }
>> @@ -228,7 +220,7 @@ int tpm_util_test_tpmdev(int tpm_fd, TPMVersion
>> *tpm_version)
>> int ret;
>>
>> /* Send TPM 2 command */
>> - ret = tpm_util_test(tpm_fd, (unsigned char *)&test_req_tpm2,
>> + ret = tpm_util_test(tpm_fd, &test_req_tpm2,
>> sizeof(test_req_tpm2), &return_tag);
>> /* TPM 2 would respond with a tag of TPM2_ST_NO_SESSIONS */
>> if (!ret && return_tag == TPM2_ST_NO_SESSIONS) {
>> @@ -237,7 +229,7 @@ int tpm_util_test_tpmdev(int tpm_fd, TPMVersion
>> *tpm_version)
>> }
>>
>> /* Send TPM 1.2 command */
>> - ret = tpm_util_test(tpm_fd, (unsigned char *)&test_req,
>> + ret = tpm_util_test(tpm_fd, &test_req,
>> sizeof(test_req), &return_tag);
>> if (!ret && return_tag == TPM_TAG_RSP_COMMAND) {
>> *tpm_version = TPM_VERSION_1_2;
>> @@ -253,7 +245,6 @@ int tpm_util_test_tpmdev(int tpm_fd, TPMVersion
>> *tpm_version)
>> int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>> size_t *buffersize)
>> {
>> - unsigned char buf[1024];
>> int ret;
>>
>> switch (tpm_version) {
>> @@ -277,26 +268,27 @@ int tpm_util_get_buffer_size(int tpm_fd, TPMVersion
>> tpm_version,
>> struct tpm_resp_hdr hdr;
>> uint32_t len;
>> uint32_t buffersize;
>> - } QEMU_PACKED *tpm_resp = (struct tpm_resp_get_buffer_size *)buf;
>> + } QEMU_PACKED tpm_resp;
>>
>> - ret = tpm_util_request(tpm_fd, (unsigned char
>> *)&tpm_get_buffer_size,
>> - sizeof(tpm_get_buffer_size), buf,
>> sizeof(buf));
>> + ret = tpm_util_request(tpm_fd, &tpm_get_buffer_size,
>> + sizeof(tpm_get_buffer_size),
>> + &tpm_resp, sizeof(tpm_resp));
>> if (ret < 0) {
>> return ret;
>> }
>>
>> - if (be32_to_cpu(tpm_resp->hdr.len) != sizeof(*tpm_resp) ||
>> - be32_to_cpu(tpm_resp->len) != sizeof(uint32_t)) {
>> + if (be32_to_cpu(tpm_resp.hdr.len) != sizeof(tpm_resp) ||
>> + be32_to_cpu(tpm_resp.len) != sizeof(uint32_t)) {
>> DPRINTF("tpm_resp->hdr.len = %u, expected = %zu\n",
>> - be32_to_cpu(tpm_resp->hdr.len), sizeof(*tpm_resp));
>> + be32_to_cpu(tpm_resp.hdr.len), sizeof(tpm_resp));
>> DPRINTF("tpm_resp->len = %u, expected = %zu\n",
>> - be32_to_cpu(tpm_resp->len), sizeof(uint32_t));
>> + be32_to_cpu(tpm_resp.len), sizeof(uint32_t));
>> error_report("tpm_util: Got unexpected response to "
>> "TPM_GetCapability; errcode: 0x%x",
>> - be32_to_cpu(tpm_resp->hdr.errcode));
>> + be32_to_cpu(tpm_resp.hdr.errcode));
>> return -EFAULT;
>> }
>> - *buffersize = be32_to_cpu(tpm_resp->buffersize);
>> + *buffersize = be32_to_cpu(tpm_resp.buffersize);
>> break;
>> }
>> case TPM_VERSION_2_0: {
>> @@ -324,27 +316,28 @@ int tpm_util_get_buffer_size(int tpm_fd, TPMVersion
>> tpm_version,
>> uint32_t value1;
>> uint32_t property2;
>> uint32_t value2;
>> - } QEMU_PACKED *tpm2_resp = (struct tpm2_resp_get_buffer_size
>> *)buf;
>> + } QEMU_PACKED tpm2_resp;
>>
>> - ret = tpm_util_request(tpm_fd, (unsigned char
>> *)&tpm2_get_buffer_size,
>> - sizeof(tpm2_get_buffer_size), buf,
>> sizeof(buf));
>> + ret = tpm_util_request(tpm_fd, &tpm2_get_buffer_size,
>> + sizeof(tpm2_get_buffer_size),
>> + &tpm2_resp, sizeof(tpm2_resp));
>> if (ret < 0) {
>> return ret;
>> }
>>
>> - if (be32_to_cpu(tpm2_resp->hdr.len) != sizeof(*tpm2_resp) ||
>> - be32_to_cpu(tpm2_resp->count) != 2) {
>> + if (be32_to_cpu(tpm2_resp.hdr.len) != sizeof(tpm2_resp) ||
>> + be32_to_cpu(tpm2_resp.count) != 2) {
>> DPRINTF("tpm2_resp->hdr.len = %u, expected = %zu\n",
>> - be32_to_cpu(tpm2_resp->hdr.len), sizeof(*tpm2_resp));
>> + be32_to_cpu(tpm2_resp.hdr.len), sizeof(tpm2_resp));
>> DPRINTF("tpm2_resp->len = %u, expected = %u\n",
>> - be32_to_cpu(tpm2_resp->count), 2);
>> + be32_to_cpu(tpm2_resp.count), 2);
>> error_report("tpm_util: Got unexpected response to "
>> "TPM2_GetCapability; errcode: 0x%x",
>> - be32_to_cpu(tpm2_resp->hdr.errcode));
>> + be32_to_cpu(tpm2_resp.hdr.errcode));
>> return -EFAULT;
>> }
>> - *buffersize = MAX(be32_to_cpu(tpm2_resp->value1),
>> - be32_to_cpu(tpm2_resp->value2));
>> + *buffersize = MAX(be32_to_cpu(tpm2_resp.value1),
>> + be32_to_cpu(tpm2_resp.value2));
>> break;
>> }
>> case TPM_VERSION_UNSPEC:
>
>
>