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(-)
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 <peter.maydell@linaro.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
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);;
+}
+
+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);
}
}
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:
--
2.16.0.rc1.1.gef27df75a1
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 <peter.maydell@linaro.org> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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 ';;' ? > +} > + > +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. > } > } > > 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:
Hi On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger <stefanb@linux.vnet.ibm.com> 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 <peter.maydell@linaro.org> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> 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: > > >
On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote: > Hi > > On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger > <stefanb@linux.vnet.ibm.com> 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 <peter.maydell@linaro.org> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> 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. Also please rebase on top of your series of patches. I had some problems with tpm_passthrough.c around line 115. Error reporting there has changed.
Hi On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote: > On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote: >> >> Hi >> >> On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger >> <stefanb@linux.vnet.ibm.com> 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 <peter.maydell@linaro.org> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> --- >>>> 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. > > > Also please rebase on top of your series of patches. I had some problems > with tpm_passthrough.c around line 115. Error reporting there has changed. > > The patch is meant to be applied first on top of master, to avoid intermediary regressions (it passes patchew: http://patchew.org/QEMU/20180129123227.15778-1-marcandre.lureau@redhat.com/) There was no conflicts after a rebase of my series. If it helps, I can resend my pending tpm queue.
On 01/29/2018 01:02 PM, Marc-Andre Lureau wrote: > Hi > > On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger > <stefanb@linux.vnet.ibm.com> wrote: >> On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote: >>> Hi >>> >>> On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger >>> <stefanb@linux.vnet.ibm.com> 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 <peter.maydell@linaro.org> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>> --- >>>>> 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. >> >> Also please rebase on top of your series of patches. I had some problems >> with tpm_passthrough.c around line 115. Error reporting there has changed. >> >> > The patch is meant to be applied first on top of master, to avoid > intermediary regressions (it passes patchew: > http://patchew.org/QEMU/20180129123227.15778-1-marcandre.lureau@redhat.com/) > > There was no conflicts after a rebase of my series. If it helps, I can > resend my pending tpm queue. > Please resent the series. Stefan
© 2016 - 2024 Red Hat, Inc.