[U-Boot] [PATCH 1/2] tpm: sandbox: fix wrong check on pcr_map
The second check on pcr_map in sandbox_tpm2_xfer() is wrong. It should check for pcr_map not being empty. Instead, it is a pure copy/paste of the first check which is redundant.
This has been found thanks to a Coverity Scan report:
CID 183370: Memory - illegal accesses (UNINIT) Using uninitialized value "pcr_index". put_unaligned_be32(tpm->pcr_extensions[pcr_index], recv);
This is because pcr_index is initialized only if the user input is correct, ie. at least one valid bit is set in pcr_map.
Fix the second check and also initialize pcr_index to 0 (which is harmless in case of error) to make Coverity Scan happy.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/tpm/tpm2_tis_sandbox.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index 66f6c9ba82..b15ec732ad 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -272,7 +272,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, u32 capability, property, property_count;
/* TPM2_PCR_Read/Extend variables */ - int pcr_index; + int pcr_index = 0; u64 pcr_map = 0; u32 selections, pcr_nb; u16 alg; @@ -483,8 +483,8 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); }
- if (pcr_map >> SANDBOX_TPM_PCR_NB) { - printf("Wrong PCR map.\n"); + if (!pcr_map) { + printf("Empty PCR map.\n"); rc = TPM2_RC_VALUE; return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); }
The recv variable in sandbox_tpm2_fill_buf() is a pointer on a pointer of a char array. It means accessing *recv is the char array pointer itself while **recv is the first character of that array. There is no need for such indirection here, so simplify the code.
Simplifying things will make the last assignment right: "*recv = NULL" is now correct. The issue has been found by the following Coverity Scan report:
CID 183371: Incorrect expression (UNUSED_VALUE) Assigning value "4UL" to "*recv" here, but that stored value is overwritten before it can be used. 232 *recv += sizeof(rc); 233 234 /* Add trailing \0 */ 235 *recv = NULL;
While at simplifying things, use '\0' instead of NULL when adding an empty char at the end of the buffer.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/tpm/tpm2_tis_sandbox.c | 52 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index b15ec732ad..f282ea6adf 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -215,24 +215,24 @@ static int sandbox_tpm2_check_readyness(struct udevice *dev, int command) return 0; }
-static int sandbox_tpm2_fill_buf(u8 **recv, size_t *recv_len, u16 tag, u32 rc) +static int sandbox_tpm2_fill_buf(u8 *recv, size_t *recv_len, u16 tag, u32 rc) { *recv_len = sizeof(tag) + sizeof(u32) + sizeof(rc);
/* Write tag */ - put_unaligned_be16(tag, *recv); - *recv += sizeof(tag); + put_unaligned_be16(tag, recv); + recv += sizeof(tag);
/* Write length */ - put_unaligned_be32(*recv_len, *recv); - *recv += sizeof(u32); + put_unaligned_be32(*recv_len, recv); + recv += sizeof(u32);
/* Write return code */ - put_unaligned_be32(rc, *recv); - *recv += sizeof(rc); + put_unaligned_be32(rc, recv); + recv += sizeof(rc);
/* Add trailing \0 */ - *recv = NULL; + *recv = '\0';
return 0; } @@ -287,7 +287,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, printf("TPM2: Unmatching length, received: %ld, expected: %d\n", send_size, length); rc = TPM2_RC_SIZE; - sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); return 0; }
@@ -295,13 +295,13 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, sent += sizeof(command); rc = sandbox_tpm2_check_readyness(dev, command); if (rc) { - sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); return 0; }
rc = sandbox_tpm2_check_session(dev, command, tag, &sent, &hierarchy); if (rc) { - sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); return 0; }
@@ -319,7 +319,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf,
tpm->startup_done = true;
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
case TPM2_CC_SELF_TEST: @@ -335,7 +335,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf,
tpm->tests_done = true;
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
case TPM2_CC_CLEAR: @@ -358,7 +358,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, tpm->pcr[i][j] = 0; }
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
case TPM2_CC_HIERCHANGEAUTH: @@ -372,7 +372,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, sent += new_pw_sz; }
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
case TPM2_CC_GET_CAPABILITY: @@ -392,7 +392,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (!property_count || property + property_count > TPM2_PROPERTY_NB) { rc = TPM2_RC_HANDLE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
/* Write tag */ @@ -445,7 +445,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, tpm->properties[TPM2_LOCKOUT_RECOVERY] = get_unaligned_be32(sent); sent += sizeof(*tpm->properties);
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
case TPM2_CC_PCR_READ: @@ -454,7 +454,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (selections != 1) { printf("Sandbox cannot handle more than one PCR\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
alg = get_unaligned_be16(sent); @@ -462,7 +462,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (alg != TPM2_ALG_SHA256) { printf("Sandbox TPM only handle SHA256 algorithm\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
pcr_array_sz = *sent; @@ -470,7 +470,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (!pcr_array_sz || pcr_array_sz > 8) { printf("Sandbox TPM cannot handle so much PCRs\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
for (i = 0; i < pcr_array_sz; i++) @@ -480,13 +480,13 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, printf("Sandbox TPM handles up to %d PCR(s)\n", SANDBOX_TPM_PCR_NB); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
if (!pcr_map) { printf("Empty PCR map.\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
for (i = 0; i < SANDBOX_TPM_PCR_NB; i++) @@ -538,7 +538,7 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (pcr_nb != 1) { printf("Sandbox cannot handle more than one PCR\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
/* Check the hash algorithm */ @@ -547,19 +547,19 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (alg != TPM2_ALG_SHA256) { printf("Sandbox TPM only handle SHA256 algorithm\n"); rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
/* Extend the PCR */ rc = sandbox_tpm2_extend(dev, pcr_index, sent);
- sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break;
default: printf("TPM2 command %02x unknown in Sandbox\n", command); rc = TPM2_RC_COMMAND_CODE; - sandbox_tpm2_fill_buf(&recv, recv_len, tag, rc); + sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); }
return 0;
On 5 August 2018 at 10:53, Miquel Raynal miquel.raynal@bootlin.com wrote:
The recv variable in sandbox_tpm2_fill_buf() is a pointer on a pointer of a char array. It means accessing *recv is the char array pointer itself while **recv is the first character of that array. There is no need for such indirection here, so simplify the code.
Simplifying things will make the last assignment right: "*recv = NULL" is now correct. The issue has been found by the following Coverity Scan report:
CID 183371: Incorrect expression (UNUSED_VALUE) Assigning value "4UL" to "*recv" here, but that stored value is overwritten before it can be used. 232 *recv += sizeof(rc); 233 234 /* Add trailing \0 */ 235 *recv = NULL;While at simplifying things, use '\0' instead of NULL when adding an empty char at the end of the buffer.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/tpm/tpm2_tis_sandbox.c | 52 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Aug 05, 2018 at 06:53:07PM +0200, Miquel Raynal wrote:
The recv variable in sandbox_tpm2_fill_buf() is a pointer on a pointer of a char array. It means accessing *recv is the char array pointer itself while **recv is the first character of that array. There is no need for such indirection here, so simplify the code.
Simplifying things will make the last assignment right: "*recv = NULL" is now correct. The issue has been found by the following Coverity Scan report:
CID 183371: Incorrect expression (UNUSED_VALUE) Assigning value "4UL" to "*recv" here, but that stored value is overwritten before it can be used. 232 *recv += sizeof(rc); 233 234 /* Add trailing \0 */ 235 *recv = NULL;While at simplifying things, use '\0' instead of NULL when adding an empty char at the end of the buffer.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
On 5 August 2018 at 10:53, Miquel Raynal miquel.raynal@bootlin.com wrote:
The second check on pcr_map in sandbox_tpm2_xfer() is wrong. It should check for pcr_map not being empty. Instead, it is a pure copy/paste of the first check which is redundant.
This has been found thanks to a Coverity Scan report:
CID 183370: Memory - illegal accesses (UNINIT) Using uninitialized value "pcr_index". put_unaligned_be32(tpm->pcr_extensions[pcr_index], recv);This is because pcr_index is initialized only if the user input is correct, ie. at least one valid bit is set in pcr_map.
Fix the second check and also initialize pcr_index to 0 (which is harmless in case of error) to make Coverity Scan happy.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/tpm/tpm2_tis_sandbox.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
It is always nice to see a test update with a bug fix (so we test that the problem is fixed). I'm not sure if that is possible?
- Simon
On Sun, Aug 05, 2018 at 06:53:06PM +0200, Miquel Raynal wrote:
The second check on pcr_map in sandbox_tpm2_xfer() is wrong. It should check for pcr_map not being empty. Instead, it is a pure copy/paste of the first check which is redundant.
This has been found thanks to a Coverity Scan report:
CID 183370: Memory - illegal accesses (UNINIT) Using uninitialized value "pcr_index". put_unaligned_be32(tpm->pcr_extensions[pcr_index], recv);This is because pcr_index is initialized only if the user input is correct, ie. at least one valid bit is set in pcr_map.
Fix the second check and also initialize pcr_index to 0 (which is harmless in case of error) to make Coverity Scan happy.
Reported-by: Tom Rini trini@konsulko.com Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Miquel Raynal -
Simon Glass -
Tom Rini