Integer underflow results in memory corruption

[jeremyncc]

The tis_recv function contains an integer underflow problem that could lead to memory corruption.

size_t tis_recv(struct tpmbuff *buf)
{
    u32 expected;
    u8 status, *buf_ptr;
    struct tpm_header *hdr;
    ...
    hdr = (struct tpm_header *)buf->head;
    expected = sizeof(struct tpm_header);
    if (recv_data(buf->head, expected) < expected)
        goto err;
    ...
    hdr->size = be32_to_cpu(hdr->size);
    ...
    expected = hdr->size - expected;
    buf_ptr = tpmb_put(buf, expected);
    if (! buf_ptr)
        goto err;

    if (recv_data(buf_ptr, expected - 1) < (expected - 1))
        goto err;
    ...

Above, the length value that was pulled out of the TPM response header, hdr->size, could be manipulated by a MITM on the serial bus that connects to the TPM peripheral. If this interposer device sets hdr->size such that it equals sizeof(struct tpm_header), then expected will be equal to 0 after the subtraction on line 172.

On line 173, tpmb_put is invoked with expected=0 for its size argument. This allows the length sanitization within the function to pass without error:

u8 *tpmb_put(struct tpmbuff *b, size_t size)
{
    u8 *tail = b->tail;

    if ((b->len + size) > b->truesize)
        return NULL; /* TODO: add overflow buffer support */

    b->tail += size;
    b->len += size;

    return tail;
}

Then finally recv_data is called with expected-1 as its length argument, which underflows to a very large positive integer 0xFFFF_FFFF. The interposer can now send more bytes over the bus than will fit in the destination buffer. This of course results in an overflow of buf.

static size_t recv_data(unsigned char *buf, size_t len)
{
    size_t size = 0;
    u8 status, *bufptr;
    u32 burstcnt = 0;

    bufptr = (u8 *)buf;

    while (tis_data_available(locality) && size < len) {
        burstcnt = burst_wait();
        for (; burstcnt > 0 && size < len; burstcnt--) {
            *bufptr = tpm_read8(DATA_FIFO(locality));
            bufptr++;
            size++;
        }
    }

    return size;
}

To prevent this issue, hdr->size needs to be bounds checked in tis_recv. It must be at least as large as the TPM response header sizeof(struct tpm_header) + 1 to prevent expected from underflowing.

Also, there is a comment in tpmb_put which suggests that an integer overflow check is missing. While that is true, I do not think that an underflow of expected is exploitable in this function. The value expected is a u32 type but is cast to size_t when passed as an argument, and b->len is size_t. Therefore, b->len + size should not be able to wrap, except on 32-bit systems where u32 and size_t are the same width. But still, checking for integer wrap here is probably a good idea.

Finally, I think the exact same issue is present in the landing-zone repository, here: https://github.com/TrenchBoot/landing-zone/blob/747bf7a1a6f625559146cf33b9a888403b8cb765/early_tpm.c#L387

[dpsmith]

@jeremyncc thank you very much for the review!

As to your landing-zone comment, you are correct that this is used there. This is meant to be a simple, independent (aka license unencumbered) implementation that we can embedded into boot loaders and kernels.

There is still quite a bit of work to go with the implementation and you are welcome review and/or contribute.

[dpsmith]

Resolved