Hi Stefan,
Thanks for the detailed explanations.
On 04/02/2019 18:05, Stefan Berger wrote:
On 1/30/19 9:45 AM, Liam Merwick wrote:
Assert that various locality values don't exceed TPM_TIS_NUM_LOCALITIES
by adding specific calls to assert(TPM_TIS_NUM_LOCALITIES(l)) in order
to help static code analysis.
Signed-off-by: Liam Merwick <liam.merw...@oracle.com>
---
hw/tpm/tpm_tis.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index fd6bb9b59a96..2267bd8c346f 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -241,6 +241,9 @@ static void tpm_tis_abort(TPMState *s)
{
s->rw_offset = 0;
+ assert(TPM_TIS_IS_VALID_LOCTY(s->next_locty));
There are 2 callers of tpm_tis_abort: tpm_tis_prep_abort and
tpm_tis_request_completed. Both are checking s->next_locality already.
The former in line 270
assert(TPM_TIS_IS_VALID_LOCTY(newlocty));
s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */
s->next_locty = newlocty; /* locality after successful abort */
and the latter in line 321:
if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
tpm_tis_abort(s);
}
So this check would be redundant.
Agreed.
+ assert(TPM_TIS_IS_VALID_LOCTY(s->aborting_locty));
+
s->aborting_locty only serves as an index into the array in
tpm_tis_abort and only under the condition
if (s->aborting_locty == s->next_locty) {
As state above s->next_locty has been tested for being a valid locality
already elsewhere and we don't need to test it another time. They way
the code accesses this variable we do not need this check, either.
Besides that there is this comment in the code in line 272, which would
make this assert wrong.
s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */
Indeed.
trace_tpm_tis_abort(s->next_locty);
/*
@@ -531,6 +534,8 @@ static void tpm_tis_mmio_write(void *opaque,
hwaddr addr,
uint16_t len;
uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
+ assert(TPM_TIS_IS_VALID_LOCTY(locty));
+
We also do not need this check here since we are registering 0x5000
bytes of MMIO space, which gives us addresses [0x0..0x4fff], from which
we calculate the locality with a '>> 12':
static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
{
In that case would it be good to add this check to enforce the address
range?
assert(addr < TPM_TIS_ADDR_SIZE);
return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
}
this is where we register the MMIO memory:
memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
s, "tpm-tis-mmio",
TPM_TIS_NUM_LOCALITIES <<
TPM_TIS_LOCALITY_SHIFT);
The locality cannot be out-of-bounds.
From staring at the code, I do have one other question - why does the
check of the lower localities below only check 'l < locty - 1' before
setting s->loc[locty] - it seems like s->loc[locty - 1] is skipped.
627 /* cancel any seize by a lower locality */
628 for (l = 0; l < locty - 1; l++) {
629 s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
630 }
631
632 s->loc[locty].access |= TPM_TIS_ACCESS_SEIZE;
Regards,
Liam
