trenchboot / tpmlib Goto Github PK

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