Skip to contentSkip to conversation

tpm: reject duplicate PCR banks in tpm2_get_pcr_allocation

PATCH In review Yuqi Xu 4 participants 10 messages Sep 28 – Oct 21

Subsystems (3)
lore
Share & tools
lore

Yuqi Xu

author
#0 lore raw

v2 cover tpm: reject duplicate PCR banks in tpm2_get_pcr_allocation

Hi Linux kernel maintainers,

We found and validated an issue in drivers/char/tpm/tpm2-cmd.c. The bug is triggered by an attacker-controlled TPM 2.0 backend that returns duplicate PCR bank selections in its TPM2_CAP_PCRS response during device probe.
We've tested it, and it should not affect any other functionality.

We will provide detailed information about the bug in this email, along with a PoC to trigger it.

Changes in v2:
- Return -EIO on a duplicate PCR bank instead of silently ignoring it, as requested by Jarkko Sakkinen.
- v1 Link: https://lore.kernel.org/all/[email protected]/

---- details below ----

Bug details:

struct tpm_chip reserves chip->groups as an array with only 3 + TPM_MAX_HASHES slots (8, with TPM_MAX_HASHES = 5). tpm2_get_pcr_allocation() parses the TPM2_CAP_PCRS response and adds every selection with a non-zero pcr_select to chip->allocated_banks[]. It rejects more than TPM2_MAX_PCR_BANKS (8) selections but never checks that the hash algorithm is unique. tpm_sysfs_add_device() then stores the device group plus one PCR group per allocated bank with chip->groups[chip->groups_cnt++], so eight duplicate SHA1 banks make it write index 8 of an 8-element array, before the trailing WARN_ON(chip->groups_cnt > TPM_MAX_HASHES + 1).

The TPM response is external input: a malicious or buggy TPM 2.0 backend can report the same bank eight times and drive the out-of-bounds write during tpm_chip_register() -> tpm_sysfs_add_device().

Return -EIO when a hash algorithm is reported twice. The TPM protocol does not allow that duplicate, so the chip is not enabled instead of silently dropping the extra selection.

Reproducer:

cd /root
./poc.sh

poc.sh starts malicious_tpm.py, a small swtpm-control-protocol backend that answers TPM2_CAP_PCRS with eight duplicate SHA1 selections, and boots a kernel with -device tpm-tis pointing at it. The script defaults KERNEL_SRC to linux-lts-v6.12.74 and treats tpm-sysfs.c:513 as success. That default tree is not the build in the crash log below.

The captured run is 7.3.0-rc3-00322-gab411db3c3b1 on a 2 vCPU, 2 GB RAM x86 QEMU/KVM guest, with CONFIG_TCG_TPM, CONFIG_TCG_TIS and CONFIG_UBSAN_BOUNDS enabled and panic_on_warn=1 on the command line. UBSAN in that log reports tpm-sysfs.c:517. CONFIG_TCG_TPM2_HMAC is disabled so the probe reaches TPM2_CAP_PCRS without the fake backend having to answer CREATE_PRIMARY.

------BEGIN poc.sh------

#!/usr/bin/env bash
set -euo pipefail

DIR="$(cd -- "$(dirname -- "$0")" && pwd)" KERNEL_SRC="${1:-/home/data/data/repos/linux-repos/linux-lts-v6.12.74}" CTRL_SOCK="$(mktemp -u /tmp/malicious-tpm-XXXXXX.sock)" BACKEND_LOG="${DIR}/backend.log" RAW_QEMU_LOG="${DIR}/qemu-raw.log" QEMU_LOG="${DIR}/qemu.log"

cleanup() {
if [[ -n "${BACKEND_PID:-}" ]] && kill -0 "${BACKEND_PID}" 2>/dev/null; then

    kill -TERM "${BACKEND_PID}" 2>/dev/null || true
    wait "${BACKEND_PID}" 2>/dev/null || true

fi
rm -f "${CTRL_SOCK}"
}
trap cleanup EXIT

: > "${BACKEND_LOG}"

python3 "${DIR}/malicious_tpm.py" --ctrl "${CTRL_SOCK}" --log "${BACKEND_LOG}" & BACKEND_PID=$!

echo "backend log: ${BACKEND_LOG}"
echo "raw qemu log: ${RAW_QEMU_LOG}"
echo "sanitized qemu log: ${QEMU_LOG}"
echo "The VM should exit on its own after the panic. If it does not, kill the printed pid."

script -q -f "${RAW_QEMU_LOG}" -c "${DIR}/qemu-start-kernel-tpm.sh ${KERNEL_SRC} ${CTRL_SOCK}" perl -pe 's/\e\[[0-9;?]*[ -\/]*[@-~]//g; s/\ec//g; s/\r//g' "${RAW_QEMU_LOG}" > "${QEMU_LOG}"

if grep -q 'UBSAN: array-index-out-of-bounds in ../drivers/char/tpm/tpm-sysfs.c:513:16' "${QEMU_LOG}" &&
grep -q 'Kernel panic - not syncing: UBSAN: panic_on_warn set' "${QEMU_LOG}"; then
echo "Trigger confirmed. See ${QEMU_LOG}"
else
echo "Trigger not observed. See ${QEMU_LOG} and ${BACKEND_LOG}" >&2
exit 1
fi

------END poc.sh--------

------BEGIN malicious_tpm.py excerpt------

ALG_SHA1 = 0x0004

def build_get_cap_pcrs():

    entries = []
    for _ in range(8):
        entries.append(be16(ALG_SHA1) + b"\x03" + b"\x01\x00\x00")
    payload = b"\x00" + be32(TPM2_CAP_PCRS) + be32(8) + b"".join(entries)
    return build_header(TPM2_RC_SUCCESS, payload)

        if cap == TPM2_CAP_PCRS:
            return build_get_cap_pcrs()

------END malicious_tpm.py excerpt------

Captured 7.3.0-rc3 log (7.3.0-rc3-00322-gab411db3c3b1). This is not the script's default linux-lts-v6.12.74 tree.

----BEGIN crash log----

[    0.487531] ------------[ cut here ]------------
[    0.487533] UBSAN: array-index-out-of-bounds in /home/lucas/work/net-tpm-675/drivers/char/tpm/tpm-sysfs.c:517:16
[    0.487535] index 8 is out of range for type 'attribute_group *[8]'
[    0.487538] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc3-00322-gab411db3c3b1 #2 PREEMPT(lazy) 
[    0.487541] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-10.fc44 06/10/2025
[    0.487542] Call Trace:
[    0.487553]  <TASK>
[    0.487555]  dump_stack_lvl+0x4d/0x70
[    0.487561]  ubsan_epilogue+0x5/0x2b
[    0.487564]  __ubsan_handle_out_of_bounds.cold+0x4e/0x58
[    0.487567]  tpm_sysfs_add_device+0x29f/0x380
[    0.487573]  tpm_chip_register+0x3d/0x1e0
[    0.487576]  ? srso_alias_return_thunk+0x5/0xfbef5
[    0.487579]  ? srso_alias_return_thunk+0x5/0xfbef5
[    0.487580]  tpm_tis_core_init.cold+0x1bc/0x3a1
[    0.487584]  tpm_tis_plat_probe+0x101/0x130
[    0.487588]  ? srso_alias_return_thunk+0x5/0xfbef5
[    0.487590]  platform_probe+0x74/0xc0
[    0.487594]  ? driver_sysfs_add+0x50/0x80
[    0.487596]  really_probe+0xdd/0x290
[    0.487597]  ? srso_alias_return_thunk+0x5/0xfbef5
[    0.487599]  __driver_probe_device+0x99/0x180
[    0.487601]  ? __pfx___driver_attach+0x10/0x10
[    0.487602]  driver_probe_device+0x1a/0xa0
[    0.487604]  ? __pfx___driver_attach+0x10/0x10
[    0.487605]  __driver_attach+0xab/0x180
[    0.487607]  ? srso_alias_return_thunk+0x5/0xfbef5
[    0.487608]  bus_for_each_dev+0x92/0xf0
[    0.487611]  bus_add_driver+0x104/0x220
[    0.487613]  ? __pfx_init_tis+0x10/0x10
[    0.487617]  driver_register+0x70/0xe0
[    0.487619]  init_tis+0x9b/0xf0
[    0.487623]  do_one_initcall+0x84/0x260
[    0.487626]  kernel_init_freeable+0x249/0x2c0
[    0.487630]  ? __pfx_kernel_init+0x10/0x10
[    0.487633]  kernel_init+0x1b/0x140
[    0.487635]  ? __pfx_kernel_init+0x10/0x10
[    0.487637]  ret_from_fork+0x196/0x260
[    0.487640]  ? __pfx_kernel_init+0x10/0x10
[    0.487641]  ? __pfx_kernel_init+0x10/0x10
[    0.487643]  ret_from_fork_asm+0x1a/0x30
[    0.487646]  </TASK>
[    0.487647] ---[ end trace ]---
[    0.487648] Kernel panic - not syncing: UBSAN: panic_on_warn set ...

-----END crash log-----

Best regards,
Yuqi Xu

Yuqi Xu (1):
tpm: reject duplicate PCR banks in tpm2_get_pcr_allocation

 drivers/char/tpm/tpm2-cmd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

base-commit: ab411db3c3b1fd0c70a36fb32c96b2c60175210b

Signature · 1 line
2.55.0
author
#1 lore raw

PATCH v2 1/1 tpm: reject duplicate PCR banks in tpm2_get_pcr_allocation

The TPM2_CAP_PCRS response is external input. tpm2_get_pcr_allocation() adds every active selection to allocated_banks, while tpm_sysfs_add_device() has one group slot per distinct hash algorithm. Eight duplicate selections can therefore write past chip->groups[].

The TPM protocol does not allow a hash algorithm to appear twice in this capability response. Treat a duplicate as a broken TPM and return -EIO so the chip is not enabled, instead of silently dropping the extra selection.

Fixes aab73d952402 ("tpm: add sysfs exports for all banks of PCR registers")
Assisted-by LLM

---
Changes in v2:
- Return -EIO on a duplicate PCR bank instead of silently ignoring it, as requested by Jarkko Sakkinen. A duplicate violates the TPM protocol, so the chip is not enabled.
- v1 Link: https://lore.kernel.org/all/[email protected]/

 drivers/char/tpm/tpm2-cmd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/char/tpm/tpm2-cmd.c +18 -0
index ae22295df798..a94f4395c198 100644
@@ -527,6 +527,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	u32 rsp_len;
 	int rc;
 	int i = 0;
+	int j;
 
 	struct tpm_buf *buf __free(kfree) = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
 	if (!buf)
@@ -569,6 +570,23 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		pcr_select_offset = memchr_inv(pcr_selection.pcr_select, 0,
 					       pcr_selection.size_of_select);
 		if (pcr_select_offset) {
+			for (j = 0; j < nr_alloc_banks; j++) {
+				if (chip->allocated_banks[j].alg_id == hash_alg)
+					break;
+			}
+
+			/*
+			 * The TPM protocol does not allow the same hash
+			 * algorithm twice.  A duplicate means this device
+			 * is not behaving; do not enable it.
+			 */
+			if (j != nr_alloc_banks) {
+				pr_err("tpm: duplicate PCR bank for algorithm 0x%04x\n",
+				       hash_alg);
+				rc = -EIO;
+				break;
+			}
+
 			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
 
 			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
Signature · 1 line
2.55.0
maintainer Reviewed-by on v2 1/1
#2 lore raw
Quoted 64 lines · Yuqi Xu

The TPM2_CAP_PCRS response is external input. tpm2_get_pcr_allocation() adds every active selection to allocated_banks, while tpm_sysfs_add_device() has one group slot per distinct hash algorithm. Eight duplicate selections can therefore write past chip->groups[].

The TPM protocol does not allow a hash algorithm to appear twice in this capability response. Treat a duplicate as a broken TPM and return -EIO so the chip is not enabled, instead of silently dropping the extra selection.

Fixes aab73d952402 ("tpm: add sysfs exports for all banks of PCR registers")
Assisted-by LLM

---
Changes in v2:
- Return -EIO on a duplicate PCR bank instead of silently ignoring it, as requested by Jarkko Sakkinen. A duplicate violates the TPM protocol, so the chip is not enabled.
- v1 Link: https://lore.kernel.org/all/[email protected]/

 drivers/char/tpm/tpm2-cmd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/char/tpm/tpm2-cmd.c +18 -0
index ae22295df798..a94f4395c198 100644
@@ -527,6 +527,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	u32 rsp_len;
 	int rc;
 	int i = 0;
+	int j;
 
 	struct tpm_buf *buf __free(kfree) = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
 	if (!buf)
@@ -569,6 +570,23 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		pcr_select_offset = memchr_inv(pcr_selection.pcr_select, 0,
 					       pcr_selection.size_of_select);
 		if (pcr_select_offset) {
+			for (j = 0; j < nr_alloc_banks; j++) {
+				if (chip->allocated_banks[j].alg_id == hash_alg)
+					break;
+			}
+
+			/*
+			 * The TPM protocol does not allow the same hash
+			 * algorithm twice.  A duplicate means this device
+			 * is not behaving; do not enable it.
+			 */
+			if (j != nr_alloc_banks) {
+				pr_err("tpm: duplicate PCR bank for algorithm 0x%04x\n",
+				       hash_alg);
+				rc = -EIO;
+				break;
+			}
+
 			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
 
 			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
Signature · 1 line
2.55.0

Yeah, so in some places like this some of the TPM driver code does return -EFAULT but I'd like to progressively refactor so that any corrupted traffic is always -EIO and use -EFAULT for memory related issues.

Thank you!

Br, Jarkko

maintainer
#3 lore raw
Quoted 71 lines · Jarkko Sakkinen
Quoted 64 lines · Yuqi Xu

The TPM2_CAP_PCRS response is external input. tpm2_get_pcr_allocation() adds every active selection to allocated_banks, while tpm_sysfs_add_device() has one group slot per distinct hash algorithm. Eight duplicate selections can therefore write past chip->groups[].

The TPM protocol does not allow a hash algorithm to appear twice in this capability response. Treat a duplicate as a broken TPM and return -EIO so the chip is not enabled, instead of silently dropping the extra selection.

Fixes aab73d952402 ("tpm: add sysfs exports for all banks of PCR registers")
Assisted-by LLM

---
Changes in v2:
- Return -EIO on a duplicate PCR bank instead of silently ignoring it, as requested by Jarkko Sakkinen. A duplicate violates the TPM protocol, so the chip is not enabled.
- v1 Link: https://lore.kernel.org/all/[email protected]/

 drivers/char/tpm/tpm2-cmd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/char/tpm/tpm2-cmd.c +18 -0
index ae22295df798..a94f4395c198 100644
@@ -527,6 +527,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	u32 rsp_len;
 	int rc;
 	int i = 0;
+	int j;
 
 	struct tpm_buf *buf __free(kfree) = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
 	if (!buf)
@@ -569,6 +570,23 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		pcr_select_offset = memchr_inv(pcr_selection.pcr_select, 0,
 					       pcr_selection.size_of_select);
 		if (pcr_select_offset) {
+			for (j = 0; j < nr_alloc_banks; j++) {
+				if (chip->allocated_banks[j].alg_id == hash_alg)
+					break;
+			}
+
+			/*
+			 * The TPM protocol does not allow the same hash
+			 * algorithm twice.  A duplicate means this device
+			 * is not behaving; do not enable it.
+			 */
+			if (j != nr_alloc_banks) {
+				pr_err("tpm: duplicate PCR bank for algorithm 0x%04x\n",
+				       hash_alg);
+				rc = -EIO;
+				break;
+			}
+
 			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
 
 			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
Signature · 1 line
2.55.0

Yeah, so in some places like this some of the TPM driver code does return -EFAULT but I'd like to progressively refactor so that any corrupted traffic is always -EIO and use -EFAULT for memory related issues.

Quoted 3 lines

Thank you!

WARNING: Reported-by: should be immediately followed by Closes: or Link: with a URL to the report #18:

Assisted-by LLM

No need to resend a new version but I'd need a link to the *report* to retain reported-by tag here.

Br, Jarkko

author
#4 lore raw
Quoted 2 lines · Jarkko Sakkinen

No need to resend a new version but I'd need a link to the *report* to retain reported-by tag here.

https://bugtracker.nebusec.ai/f/675

This is the report. The page requires a sign-in or registration before it opens.

Thanks,
Yuqi Xu

#5 lore raw
Quoted 10 lines · Yuqi Xu
Quoted 2 lines · Jarkko Sakkinen

No need to resend a new version but I'd need a link to the *report* to retain reported-by tag here.

https://bugtracker.nebusec.ai/f/675

This is the report. The page requires a sign-in or registration before it opens.

Thanks,
Yuqi Xu

Hi Jarkko,

This bug report comes from our bug tracker[1]. The tracker automatically collects bug reports from different sources, such as Sashiko and bugs found by our own tools, and then attempts to generate reproducers to verify that the reported issues are real rather than LLM hallucinations.

Some maintainers were concerned that some of these bugs could have security implications, so we currently restrict registration to email addresses listed in the MAINTAINERS file.

At the moment, we have implemented automatic fix tracking and Sashiko report collection for the networking subsystem. These features are not yet available for other subsystems, so if you log in, you may notice that some bugs in the system have already been fixed upstream.

We'd be happy to provide support if you're interested in using the tracker.

Best,
Yuan

Background about this tracker:
[1] https://lore.kernel.org/all/[email protected]/

<https://lore.kernel.org/all/[email protected]/>

#6 lore raw
In reply to Yuqi Xu
Quoted 8 lines · Yuqi Xu
Quoted 2 lines · Jarkko Sakkinen

No need to resend a new version but I'd need a link to the *report* to retain reported-by tag here.

https://bugtracker.nebusec.ai/f/675

This is the report. The page requires a sign-in or registration before it opens.

For me this is good enough given that there is nothing unclear in the bug fix itself.

Thanks.

Quoted 3 lines

Thanks,
Yuqi Xu

Br, Jarkko

maintainer
#7 lore raw
Quoted 7 lines · Jarkko Sakkinen
Quoted 6 lines · Yuqi Xu
Quoted 2 lines · Jarkko Sakkinen

No need to resend a new version but I'd need a link to the *report* to retain reported-by tag here.

https://bugtracker.nebusec.ai/f/675

Quoted 7 lines
Quoted 2 lines

This is the report. The page requires a sign-in or registration before it opens.

For me this is good enough given that there is nothing unclear in the bug fix itself.

Thanks.

OK, so now that I was writing summary of TPM pull request changes, I came into conclusion that this is not a bug fix, unless you can show from TCG specifications that this is possible spec allowed behaviour.

So I can keep the commit but would remove fixes and bug related tags from this. It is essentially hardening and safety measure but not a bug fix.

Do you have something to say that I might have missed?

Br, Jarkko

author
#8 lore raw

Hi Jarkko,

Quoted 3 lines · Jarkko Sakkinen

OK, so now that I was writing summary of TPM pull request changes, I came into conclusion that this is not a bug fix, unless you can show from TCG specifications that this is possible spec allowed behaviour.

I cannot. Part 1 identifies a PCR bank by its hash algorithm, so one hash is one bank. In the TCG reference implementation, TPM2_PCR_Allocate keeps only the last selection when the same hash appears more than once, and TPM2_GetCapability(TPM_CAP_PCRS) returns that allocation. A conformant TPM will not report a duplicate.

Quoted 3 lines

So I can keep the commit but would remove fixes and bug related tags from this. It is essentially hardening and safety measure but not a bug fix.

Please do. Drop Fixes:, Cc: stable, Reported-by:, and Closes:. The code change can stay as hardening.

Quoted 1 line

Do you have something to say that I might have missed?

Only that the commit is not on the current master after the rebase. I can send a v3 without those tags if that is easier than reapplying it yourself.

Thanks,
Yuqi

maintainer
#9 lore raw
Quoted 17 lines · Yuqi Xu

Hi Jarkko,

Quoted 3 lines · Jarkko Sakkinen

OK, so now that I was writing summary of TPM pull request changes, I came into conclusion that this is not a bug fix, unless you can show from TCG specifications that this is possible spec allowed behaviour.

I cannot. Part 1 identifies a PCR bank by its hash algorithm, so one hash is one bank. In the TCG reference implementation, TPM2_PCR_Allocate keeps only the last selection when the same hash appears more than once, and TPM2_GetCapability(TPM_CAP_PCRS) returns that allocation. A conformant TPM will not report a duplicate.

Quoted 3 lines

So I can keep the commit but would remove fixes and bug related tags from this. It is essentially hardening and safety measure but not a bug fix.

Quoted 8 lines

Please do. Drop Fixes:, Cc: stable, Reported-by:, and Closes:. The code change can stay as hardening.

Quoted 1 line

Do you have something to say that I might have missed?

Only that the commit is not on the current master after the rebase. I can send a v3 without those tags if that is easier than reapplying it yourself.

Thanks for quick reaction. I can still get it to the pull request.

This was just a question of taxonomy. I think it is otherwise useful to have this validation in place (i.e. not disregarding the change overall).

Quoted 4 lines

Thanks,
Yuqi

Br, Jarkko