Re: [Qemu-devel] [PATCH v1] tpm: check localities index

[Stefan Berger]

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 <address@hidden>
> | >
> | > 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