| Message ID | 20241015193916.59964-1-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] tpm: Allow the TPM2 pcr_extend HMAC capability to be disabled on boot | expand |
On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote: > The initial TPM2 HMAC session capability added HMAC authentication to > each and every TPM communication making the pcr_extend performance > abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was > configured by default on x86_64. > > The decision to use the TPM2 HMAC session capability feature doesn't > differentiate between the critical encrypted and the non-encrypted > communication, but when configured is required for all TPM communication. > > In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit > 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect > tpm2_pcr_extend() when used by "trusted keys" to lock the PCR. However, > locking the PCR is currently limited to TPM 1.2. > > We can revert the commit which adds the HMAC sessions for > tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be > disabled on boot for better IMA performance, or define a generic boot > command line option to disable HMAC in general. This patch allows > disabling the HMAC for just the TPM2_pcr_extend. > > Fixes: 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") > Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch - > commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open") > > .../admin-guide/kernel-parameters.txt | 5 ++ > drivers/char/tpm/tpm2-cmd.c | 41 ++++++++++--- > drivers/char/tpm/tpm2-sessions.c | 59 +++++++++++-------- > include/linux/tpm.h | 4 ++ > 4 files changed, 77 insertions(+), 32 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 1518343bbe22..c7811f32ba28 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6727,6 +6727,11 @@ > torture.verbose_sleep_duration= [KNL] > Duration of each verbose-printk() sleep in jiffies. > > + tpm_pcr_extend_hmac_disable [HW,TPM] > + Disable TPM2 pcr_extend HMAC for better IMA > + performance. By default is set to true (1). > + Mainly needed when using a HW TPM2. Thanks for doing this! I think the code change itself is pretty good but maybe we should not emphasize HMAC per se (applies to config flag too but it is what it is now) but instead that they are encrypted and integrity protected. I guess all these features intend to protect data from unintended and physical access, like in common sense terms. So like for any possible sysadmin and similar I think this would be something that anyone could grab: tpm_disable_protect_pcrs [HW,TPM] Do not protect PCR registers from unintended physical access, or interposers in the bus by the means of having an encrypted and integrity protected session wrapped around TPM2_PCR_Extend command. Consider this in a situation where TPM is heavily utilized by IMA, thus protection causing a major performance hit, and the space where machines are deployed is by other means guarded. Perhaps a bit long but at least it is clear and helps to make the right choice. BR, Jarkko
On Wed Oct 16, 2024 at 12:29 AM EEST, Jarkko Sakkinen wrote: > On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote: > > The initial TPM2 HMAC session capability added HMAC authentication to > > each and every TPM communication making the pcr_extend performance > > abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was > > configured by default on x86_64. > > > > The decision to use the TPM2 HMAC session capability feature doesn't > > differentiate between the critical encrypted and the non-encrypted > > communication, but when configured is required for all TPM communication. > > > > In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit > > 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect > > tpm2_pcr_extend() when used by "trusted keys" to lock the PCR. However, > > locking the PCR is currently limited to TPM 1.2. > > > > We can revert the commit which adds the HMAC sessions for > > tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be > > disabled on boot for better IMA performance, or define a generic boot > > command line option to disable HMAC in general. This patch allows > > disabling the HMAC for just the TPM2_pcr_extend. > > > > Fixes: 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") > > Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch - > > commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open") > > > > .../admin-guide/kernel-parameters.txt | 5 ++ > > drivers/char/tpm/tpm2-cmd.c | 41 ++++++++++--- > > drivers/char/tpm/tpm2-sessions.c | 59 +++++++++++-------- > > include/linux/tpm.h | 4 ++ > > 4 files changed, 77 insertions(+), 32 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 1518343bbe22..c7811f32ba28 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -6727,6 +6727,11 @@ > > torture.verbose_sleep_duration= [KNL] > > Duration of each verbose-printk() sleep in jiffies. > > > > + tpm_pcr_extend_hmac_disable [HW,TPM] > > + Disable TPM2 pcr_extend HMAC for better IMA > > + performance. By default is set to true (1). > > + Mainly needed when using a HW TPM2. > > Thanks for doing this! I think the code change itself is pretty good but > maybe we should not emphasize HMAC per se (applies to config flag too but > it is what it is now) but instead that they are encrypted and integrity > protected. > > I guess all these features intend to protect data from unintended and > physical access, like in common sense terms. > > So like for any possible sysadmin and similar I think this would be something > that anyone could grab: > > tpm_disable_protect_pcrs [HW,TPM] > Do not protect PCR registers from unintended physical > access, or interposers in the bus by the means of > having an encrypted and integrity protected session > wrapped around TPM2_PCR_Extend command. Consider this > in a situation where TPM is heavily utilized by > IMA, thus protection causing a major performance hit, > and the space where machines are deployed is by other > means guarded. > > Perhaps a bit long but at least it is clear and helps to make the right choice. Back in 2018 at LA, I think it was LSS, there was BoF where this was discussed I said that for me this feature does not necessarily make sense since data centers tend to have armed guards, and not black hat would ever take a even a minor risk of getting hole in the head :-) After that the whole ecosystem has changed, especially thanks to what Apple has done with their security chip and user friendly encrypted boot process, and that has reflected to systemd and the use TPM2, and thus as a feature bus protection has become relevant. So also based on these old conclusions I had I fully agree that we need such a flag to balance things between desktop/laptop and server use cases, which are both quite relevant. E.g. just me personally I really enjoy the experience of being able to boot my ThinkPad with encryption and without having to type a passphrase per boot. I.e. the buy-in part is totally addressed as far as I'm concerned :-) BR, Jarkko
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 1518343bbe22..c7811f32ba28 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6727,6 +6727,11 @@ torture.verbose_sleep_duration= [KNL] Duration of each verbose-printk() sleep in jiffies. + tpm_pcr_extend_hmac_disable [HW,TPM] + Disable TPM2 pcr_extend HMAC for better IMA + performance. By default is set to true (1). + Mainly needed when using a HW TPM2. + tpm_suspend_pcr=[HW,TPM] Format: integer pcr id Specify that at suspend time, the tpm driver diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index aba024cbe7c5..bac409520a72 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -14,6 +14,14 @@ #include "tpm.h" #include <crypto/hash_info.h> +static int __ro_after_init tpm2_pcr_extend_hmac = 1; +static int __init tpm2_pcr_extend_hmac_setup(char *str) +{ + tpm2_pcr_extend_hmac = 0; + return 0; +} +__setup("tpm2_pcr_extend_hmac_disable", tpm2_pcr_extend_hmac_setup); + static struct tpm2_hash tpm2_hash_map[] = { {HASH_ALGO_SHA1, TPM_ALG_SHA1}, {HASH_ALGO_SHA256, TPM_ALG_SHA256}, @@ -232,18 +240,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, int rc; int i; - rc = tpm2_start_auth_session(chip); - if (rc) - return rc; + if (tpm2_pcr_extend_hmac) { + rc = tpm2_start_auth_session(chip); + if (rc) + return rc; + } rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); if (rc) { - tpm2_end_auth_session(chip); + if (tpm2_pcr_extend_hmac) + tpm2_end_auth_session(chip); return rc; } - tpm_buf_append_name(chip, &buf, pcr_idx, NULL); - tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0); + if (tpm2_pcr_extend_hmac) { + tpm_buf_append_name(chip, &buf, pcr_idx, NULL); + tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0); + } else { + tpm_buf_append_handle(chip, &buf, pcr_idx, NULL); + tpm_buf_append_auth(chip, &buf, 0, NULL, 0); + } tpm_buf_append_u32(&buf, chip->nr_allocated_banks); @@ -253,9 +269,16 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, chip->allocated_banks[i].digest_size); } - tpm_buf_fill_hmac_session(chip, &buf); - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value"); - rc = tpm_buf_check_hmac_response(chip, &buf, rc); + if (tpm2_pcr_extend_hmac) { + tpm_buf_fill_hmac_session(chip, &buf); + rc = tpm_transmit_cmd(chip, &buf, 0, + "attempting extend a PCR value"); + rc = tpm_buf_check_hmac_response(chip, &buf, rc); + } else { + rc = tpm_transmit_cmd(chip, &buf, 0, + "attempting extend a PCR value"); + } + tpm_buf_destroy(&buf); diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index de860773eead..fae56dfe0d92 100644 --- a/drivers/char/tpm/tpm2-sessions.c +++ b/drivers/char/tpm/tpm2-sessions.c @@ -205,6 +205,14 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name) } #endif /* CONFIG_TCG_TPM2_HMAC */ +void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, + u32 handle, u8 *name) +{ + tpm_buf_append_u32(buf, handle); + /* count the number of handles in the upper bits of flags */ + buf->handles++; +} + /** * tpm_buf_append_name() - add a handle area to the buffer * @chip: the TPM chip structure @@ -237,9 +245,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf, #endif if (!tpm2_chip_auth(chip)) { - tpm_buf_append_u32(buf, handle); - /* count the number of handles in the upper bits of flags */ - buf->handles++; + tpm_buf_append_handle(chip, buf, handle, name); return; } @@ -272,6 +278,31 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf, } EXPORT_SYMBOL_GPL(tpm_buf_append_name); +void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf, + u8 attributes, u8 *passphrase, int passphrase_len) +{ + /* offset tells us where the sessions area begins */ + int offset = buf->handles * 4 + TPM_HEADER_SIZE; + u32 len = 9 + passphrase_len; + + if (tpm_buf_length(buf) != offset) { + /* not the first session so update the existing length */ + len += get_unaligned_be32(&buf->data[offset]); + put_unaligned_be32(len, &buf->data[offset]); + } else { + tpm_buf_append_u32(buf, len); + } + /* auth handle */ + tpm_buf_append_u32(buf, TPM2_RS_PW); + /* nonce */ + tpm_buf_append_u16(buf, 0); + /* attributes */ + tpm_buf_append_u8(buf, 0); + /* passphrase */ + tpm_buf_append_u16(buf, passphrase_len); + tpm_buf_append(buf, passphrase, passphrase_len); +} + /** * tpm_buf_append_hmac_session() - Append a TPM session element * @chip: the TPM chip structure @@ -309,26 +340,8 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf, #endif if (!tpm2_chip_auth(chip)) { - /* offset tells us where the sessions area begins */ - int offset = buf->handles * 4 + TPM_HEADER_SIZE; - u32 len = 9 + passphrase_len; - - if (tpm_buf_length(buf) != offset) { - /* not the first session so update the existing length */ - len += get_unaligned_be32(&buf->data[offset]); - put_unaligned_be32(len, &buf->data[offset]); - } else { - tpm_buf_append_u32(buf, len); - } - /* auth handle */ - tpm_buf_append_u32(buf, TPM2_RS_PW); - /* nonce */ - tpm_buf_append_u16(buf, 0); - /* attributes */ - tpm_buf_append_u8(buf, 0); - /* passphrase */ - tpm_buf_append_u16(buf, passphrase_len); - tpm_buf_append(buf, passphrase, passphrase_len); + tpm_buf_append_auth(chip, buf, attributes, passphrase, + passphrase_len); return; } diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 28a932aa0416..d30bb1c114f1 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -504,9 +504,13 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip) void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle, u8 *name); +void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, + u32 handle, u8 *name); void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf, u8 attributes, u8 *passphrase, int passphraselen); +void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf, + u8 attributes, u8 *passphrase, int passphraselen); static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip, struct tpm_buf *buf, u8 attributes,
The initial TPM2 HMAC session capability added HMAC authentication to each and every TPM communication making the pcr_extend performance abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was configured by default on x86_64. The decision to use the TPM2 HMAC session capability feature doesn't differentiate between the critical encrypted and the non-encrypted communication, but when configured is required for all TPM communication. In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect tpm2_pcr_extend() when used by "trusted keys" to lock the PCR. However, locking the PCR is currently limited to TPM 1.2. We can revert the commit which adds the HMAC sessions for tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be disabled on boot for better IMA performance, or define a generic boot command line option to disable HMAC in general. This patch allows disabling the HMAC for just the TPM2_pcr_extend. Fixes: 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch - commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open") .../admin-guide/kernel-parameters.txt | 5 ++ drivers/char/tpm/tpm2-cmd.c | 41 ++++++++++--- drivers/char/tpm/tpm2-sessions.c | 59 +++++++++++-------- include/linux/tpm.h | 4 ++ 4 files changed, 77 insertions(+), 32 deletions(-)