From: Stefan Berger <stefanb@linux.vnet.ibm.com>
To: "Marc-André Lureau" <marcandre.lureau@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v4 1/5] tpm: fix alignment issues
Date: Mon, 29 Jan 2018 14:19:52 -0500 [thread overview]
Message-ID: <c67c4999-a6a0-d85d-705a-47fb9b7c3b11@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180129183307.19689-2-marcandre.lureau@redhat.com>
On 01/29/2018 01:33 PM, 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>
Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
[I'll prepare a patch for the setters I mentioned in the previous review]
> ---
> 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..f003d15615 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:
next prev parent reply other threads:[~2018-01-29 19:20 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-29 18:33 [Qemu-devel] [PATCH v4 0/5] tpm: CRB device and cleanups Marc-André Lureau
2018-01-29 18:33 ` [Qemu-devel] [PATCH v4 1/5] tpm: fix alignment issues Marc-André Lureau
2018-01-29 19:19 ` Stefan Berger [this message]
2018-01-29 18:33 ` [Qemu-devel] [PATCH v4 2/5] tpm: lookup cancel path under tpm device class Marc-André Lureau
2018-01-29 18:33 ` [Qemu-devel] [PATCH v4 3/5] tpm: replace GThreadPool with AIO threadpool Marc-André Lureau
2018-01-29 18:33 ` [Qemu-devel] [PATCH v4 4/5] tpm: report backend request error Marc-André Lureau
2018-01-29 18:33 ` [Qemu-devel] [PATCH v4 5/5] tpm: add CRB device Marc-André Lureau
2018-01-31 15:01 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c67c4999-a6a0-d85d-705a-47fb9b7c3b11@linux.vnet.ibm.com \
--to=stefanb@linux.vnet.ibm.com \
--cc=marcandre.lureau@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).