On 11/20/18 1:06 PM, P J P wrote:
Hello Stefan,
+-- On Tue, 20 Nov 2018, Stefan Berger wrote --+
| On 11/20/18 2:22 AM, P J P wrote:
| > From: Prasad J Pandit <p...@fedoraproject.org>
| >
| > While performing mmio device r/w operations, guest could set 'addr'
| > parameter such that 'locty' index exceeds TPM_TIS_NUM_LOCALITIES=5
| > after setting new 'locty' via 'tpm_tis_new_active_locality'.
| > Add check to avoid OOB access.
|
| Do you have test code that can set the memory to such a locality or is this
| purely hypothetical at the moment?
It's not hypothetical, reporter used tpm-emulator below
-> https://github.com/PeterHuewe/tpm-emulator
with an old version of QEMU. I shared the details with Marc-Andre earlier
today.
| We are registering this MMIO area:
|
| memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
| s, "tpm-tis-mmio",
| TPM_TIS_NUM_LOCALITIES <<
| TPM_TIS_LOCALITY_SHIFT);
|
| #define TPM_TIS_NUM_LOCALITIES 5
|
| #define TPM_TIS_LOCALITY_SHIFT 12
|
| --> 5 << 12 = 0x5000, thus we get memory locations [0 .. 0x4fff]
|
|
| We have the following code to get from an address to the locality:
|
| static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
| {
| return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
| }
|
| With this we would get localities of [0x0 .. 0x4] following the memory
| locations [0 .. 0x4fff] above.
Right. IIUC mmio r/w routines called with 'addr' value like 0x07000 may set
'locty' to > TPM_TIS_NUM_LOCALITIES. Also in mmio_write at one point,
offsets >= 0x5000 would never reach the tpm_tis emulator. If 0x7000 was
to reach it, why not 0x70000 also or any other offset ?
'active_locty' is set to TPM_TIS_NO_LOCALITY(=0xff), which may be used to set
'new_locty'.
There's no 'new_locty'. These variables are critical, so which one do
you mean?
I audited all functions yesterday and my proposed patches are on the
mailing list. The abort related ones seem most critical but they are all
passed values they can handle. I do not think that an out-of-bounds
access can occur with the current code.
I concur with Marc's comments that an
'assert(TPM_TIS_IS_VALID_LOCTY(locty));' on a preceding ' uint8_t locty
= tpm_tis_locality_from_addr(addr);' isn't necessary unless something in
the core code is seriously broken. tpm_tis covers address offsets of
[0x0 .. 0x4fff] from its base address , which maps to localities 0..4.
tpm_tis_mmio_write
...
active_locty = TPM_TIS_NO_LOCALITY(=0xff);
...
tpm_tis_new_active_locality(s, active_locty);
And this particular function can handle active_locty = 0xff:
static void tpm_tis_new_active_locality(TPMState *s, uint8_t
new_active_locty)
{
bool change = (s->active_locty != new_active_locty);
bool is_seize;
uint8_t mask;
if (change && TPM_TIS_IS_VALID_LOCTY(s->active_locty)) {
is_seize = TPM_TIS_IS_VALID_LOCTY(new_active_locty) &&
s->loc[new_active_locty].access & TPM_TIS_ACCESS_SEIZE;
if (is_seize) {
mask = ~(TPM_TIS_ACCESS_ACTIVE_LOCALITY);
} else {
mask = ~(TPM_TIS_ACCESS_ACTIVE_LOCALITY|
TPM_TIS_ACCESS_REQUEST_USE);
}
/* reset flags on the old active locality */
s->loc[s->active_locty].access &= mask;
if (is_seize) {
s->loc[s->active_locty].access |= TPM_TIS_ACCESS_BEEN_SEIZED;
}
}
s->active_locty = new_active_locty;
trace_tpm_tis_new_active_locality(s->active_locty);
if (TPM_TIS_IS_VALID_LOCTY(new_active_locty)) {
/* set flags on the new active locality */
s->loc[new_active_locty].access |= TPM_TIS_ACCESS_ACTIVE_LOCALITY;
s->loc[new_active_locty].access &= ~(TPM_TIS_ACCESS_REQUEST_USE |
TPM_TIS_ACCESS_SEIZE);
}
if (change) {
tpm_tis_raise_irq(s, s->active_locty,
TPM_TIS_INT_LOCALITY_CHANGED);
}
}
There's no out-of-bounds access possible in this function if
new_active_locty = 0xff. tpm_tis_raise_irq is protecting itself.
The function may set the active_locty to 0xff, meaning there's no
active locality. From what I can see all functions using active_locty
with 0xff afterwards can handle it.
tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
...
s->active_locty = new_active_locty;
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
