* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18-2
From: Jarkko Sakkinen @ 2025-10-10 5:38 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Huewe, Jason Gunthorpe, David Howells, keyrings,
linux-integrity, linux-kernel
In-Reply-To: <aOibAOKu_lEsSlC8@kernel.org>
On Fri, Oct 10, 2025 at 08:35:00AM +0300, Jarkko Sakkinen wrote:
> As per Chris' feedback, commands fail because it is based on Google's a
> non-standard proprietary TPM alike implementation. And the issue is not
> PC Client Profile specific. "typical profiles" are fine when they become
"atypical profiles"
> "typical profiles".
BR, Jarkko
^ permalink raw reply
* [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18-2
From: Jarkko Sakkinen @ 2025-10-10 5:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Huewe, Jason Gunthorpe, David Howells, keyrings,
linux-integrity, linux-kernel
The following changes since commit 5472d60c129f75282d94ae5ad072ee6dfb7c7246:
Merge tag 'trace-v6.18-2' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace (2025-10-09 12:18:22 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git tags/tpmdd-next-v6.18-2
for you to fetch changes up to a29ad21b988652dc60aa99c6d3b1e3d52dc69c30:
tpm: Prevent local DOS via tpm/tpm0/ppi/*operations (2025-10-10 08:21:45 +0300)
----------------------------------------------------------------
Hi,
Round #2.
O_EXCL patches will be postponed to 6.19 as they need some restructuring.
E.g., there's in-between series regression breaking the user space as lack
of O_EXCL flag handling causes obviously unconditional O_EXCL.
As per Chris' feedback, commands fail because it is based on Google's a
non-standard proprietary TPM alike implementation. And the issue is not
PC Client Profile specific. "typical profiles" are fine when they become
"typical profiles".
The null key can be verified with vendor certificate tied keys, and there's
challenge-response process using them for certifying any other key by a
remote party.
Performance hit on generation aside, if really starting to cut hairs null
keys are the most secure option, and it's a non-debatable fact: they have
shortest expiration times as seed changes per power cycle.
Based on this TCG_TPM2_HMAC is disabled from defconfig exactly for the sake
of the performance issues.
BR, Jarkko
----------------------------------------------------------------
Denis Aleksandrov (1):
tpm: Prevent local DOS via tpm/tpm0/ppi/*operations
Eric Biggers (2):
tpm: Compare HMAC values in constant time
tpm: Use HMAC-SHA256 library instead of open-coded HMAC
Gunnar Kudrjavets (1):
tpm_tis: Fix incorrect arguments in tpm_tis_probe_irq_single
Jarkko Sakkinen (2):
tpm: Disable TPM2_TCG_HMAC by default
tpm: use a map for tpm2_calc_ordinal_duration()
drivers/char/tpm/Kconfig | 3 +-
drivers/char/tpm/tpm-interface.c | 2 +-
drivers/char/tpm/tpm.h | 2 +-
drivers/char/tpm/tpm2-cmd.c | 127 ++++++++++-----------------------------
drivers/char/tpm/tpm2-sessions.c | 104 +++++++++-----------------------
drivers/char/tpm/tpm_ppi.c | 89 ++++++++++++++++++++-------
drivers/char/tpm/tpm_tis_core.c | 4 +-
include/linux/tpm.h | 5 +-
8 files changed, 137 insertions(+), 199 deletions(-)
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: Jarkko Sakkinen @ 2025-10-10 4:01 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <aOiDqjEyowUkegbd@kernel.org>
On Fri, Oct 10, 2025 at 06:55:26AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 09, 2025 at 01:58:17PM +0200, Thorsten Blum wrote:
> > Use designated initializers for 'key_format_tokens' and 'key_tokens' to
> > allow struct fields to be reordered more easily and to improve
> > readability.
> >
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> > security/keys/encrypted-keys/encrypted.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> > index aef438d18da8..76a6dab2f4d2 100644
> > --- a/security/keys/encrypted-keys/encrypted.c
> > +++ b/security/keys/encrypted-keys/encrypted.c
> > @@ -62,17 +62,17 @@ enum {
> > };
> >
> > static const match_table_t key_format_tokens = {
> > - {Opt_default, "default"},
> > - {Opt_ecryptfs, "ecryptfs"},
> > - {Opt_enc32, "enc32"},
> > - {Opt_error, NULL}
> > + { .token = Opt_default, .pattern = "default"},
> > + { .token = Opt_ecryptfs, .pattern = "ecryptfs"},
> > + { .token = Opt_enc32, .pattern = "enc32"},
> > + { .token = Opt_error, .pattern = NULL}
> > };
> >
> > static const match_table_t key_tokens = {
> > - {Opt_new, "new"},
> > - {Opt_load, "load"},
> > - {Opt_update, "update"},
> > - {Opt_err, NULL}
> > + { .token = Opt_new, .pattern = "new"},
> > + { .token = Opt_load, .pattern = "load"},
> > + { .token = Opt_update, .pattern = "update"},
> > + { .token = Opt_err, .pattern = NULL}
> > };
> >
> > static bool user_decrypted_data = IS_ENABLED(CONFIG_USER_DECRYPTED_DATA);
> > --
> > 2.51.0
> >
>
> For me this look like a "convert tuple alike initializations into struct
> alike initializations" type of change :-)
>
> In a context the change would make sense. E.g., if an optional field was
> required.
If we had struct initializers I would equally nak "convert struct
initializers to tuple initializers" type of change.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: Jarkko Sakkinen @ 2025-10-10 3:55 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <20251009115817.368170-2-thorsten.blum@linux.dev>
On Thu, Oct 09, 2025 at 01:58:17PM +0200, Thorsten Blum wrote:
> Use designated initializers for 'key_format_tokens' and 'key_tokens' to
> allow struct fields to be reordered more easily and to improve
> readability.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> security/keys/encrypted-keys/encrypted.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index aef438d18da8..76a6dab2f4d2 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -62,17 +62,17 @@ enum {
> };
>
> static const match_table_t key_format_tokens = {
> - {Opt_default, "default"},
> - {Opt_ecryptfs, "ecryptfs"},
> - {Opt_enc32, "enc32"},
> - {Opt_error, NULL}
> + { .token = Opt_default, .pattern = "default"},
> + { .token = Opt_ecryptfs, .pattern = "ecryptfs"},
> + { .token = Opt_enc32, .pattern = "enc32"},
> + { .token = Opt_error, .pattern = NULL}
> };
>
> static const match_table_t key_tokens = {
> - {Opt_new, "new"},
> - {Opt_load, "load"},
> - {Opt_update, "update"},
> - {Opt_err, NULL}
> + { .token = Opt_new, .pattern = "new"},
> + { .token = Opt_load, .pattern = "load"},
> + { .token = Opt_update, .pattern = "update"},
> + { .token = Opt_err, .pattern = NULL}
> };
>
> static bool user_decrypted_data = IS_ENABLED(CONFIG_USER_DECRYPTED_DATA);
> --
> 2.51.0
>
For me this look like a "convert tuple alike initializations into struct
alike initializations" type of change :-)
In a context the change would make sense. E.g., if an optional field was
required.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok
From: Jarkko Sakkinen @ 2025-10-10 3:40 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-hardening, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <20251009180316.394708-3-thorsten.blum@linux.dev>
On Thu, Oct 09, 2025 at 08:03:17PM +0200, Thorsten Blum wrote:
> strncpy() is deprecated for NUL-terminated destination buffers; use
> strscpy_pad() instead.
>
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Deprecated or not but commit message still should have a better
explanation :-) Sure there's also the link too that explains it
all but still one should get the gist without cross-referencing
into it.
It's a permanent amortized cost as everytime when revisiting this
commit (e.g. over the course of bisecting or whatever), you need
a web browser in order to make anything out of the message.
> ---
> security/keys/encrypted-keys/ecryptfs_format.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/ecryptfs_format.c b/security/keys/encrypted-keys/ecryptfs_format.c
> index 8fdd76105ce3..2fc6f3a66135 100644
> --- a/security/keys/encrypted-keys/ecryptfs_format.c
> +++ b/security/keys/encrypted-keys/ecryptfs_format.c
> @@ -54,8 +54,7 @@ int ecryptfs_fill_auth_tok(struct ecryptfs_auth_tok *auth_tok,
> auth_tok->version = (((uint16_t)(major << 8) & 0xFF00)
> | ((uint16_t)minor & 0x00FF));
> auth_tok->token_type = ECRYPTFS_PASSWORD;
> - strncpy((char *)auth_tok->token.password.signature, key_desc,
> - ECRYPTFS_PASSWORD_SIG_SIZE);
> + strscpy_pad(auth_tok->token.password.signature, key_desc);
> auth_tok->token.password.session_key_encryption_key_bytes =
> ECRYPTFS_MAX_KEY_BYTES;
> /*
> --
> 2.51.0
>
BR, Jarkko
^ permalink raw reply
* [PATCH] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok
From: Thorsten Blum @ 2025-10-09 18:03 UTC (permalink / raw)
To: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn
Cc: linux-hardening, Thorsten Blum, linux-integrity, keyrings,
linux-security-module, linux-kernel
strncpy() is deprecated for NUL-terminated destination buffers; use
strscpy_pad() instead.
Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/keys/encrypted-keys/ecryptfs_format.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/security/keys/encrypted-keys/ecryptfs_format.c b/security/keys/encrypted-keys/ecryptfs_format.c
index 8fdd76105ce3..2fc6f3a66135 100644
--- a/security/keys/encrypted-keys/ecryptfs_format.c
+++ b/security/keys/encrypted-keys/ecryptfs_format.c
@@ -54,8 +54,7 @@ int ecryptfs_fill_auth_tok(struct ecryptfs_auth_tok *auth_tok,
auth_tok->version = (((uint16_t)(major << 8) & 0xFF00)
| ((uint16_t)minor & 0x00FF));
auth_tok->token_type = ECRYPTFS_PASSWORD;
- strncpy((char *)auth_tok->token.password.signature, key_desc,
- ECRYPTFS_PASSWORD_SIG_SIZE);
+ strscpy_pad(auth_tok->token.password.signature, key_desc);
auth_tok->token.password.session_key_encryption_key_bytes =
ECRYPTFS_MAX_KEY_BYTES;
/*
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: James Bottomley @ 2025-10-09 13:51 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <93D80E9A-7CBC-40D1-BC21-7BC2BB465AC4@linux.dev>
On Thu, 2025-10-09 at 15:30 +0200, Thorsten Blum wrote:
> On 9. Oct 2025, at 14:44, James Bottomley wrote:
> > On Thu, 2025-10-09 at 13:58 +0200, Thorsten Blum wrote:
> > > Use designated initializers for 'key_format_tokens' and
> > > 'key_tokens' to allow struct fields to be reordered more easily
> >
> > How does it improve that? The key,value pairs are surrounded by
> > braces so we just cut and paste the lot anyway.
>
> Using designated initializers (especially for global structs) allows
> the fields of struct match_token from linux/parser.h to be reordered
> or extended more easily, improving overall maintainability.
Why would we ever want to reorder them? The reason the ordering is
{token, parser} string is because that's the nicest order to read them
in.
>
> > > and to improve readability.
> >
> > I don't think I agree with this when looking through the code,
> > especially because this is the way it's done for *every* option in
> > the entire key subsystem. So firstly I really don't think it's
> > helpful for only encrypted keys to be different from everything
> > else and secondly when I read the code (as I often do to figure out
> > what the options mean), the additional .token and .pattern just get
> > in the way of what I'm looking for.
>
> I just stumbled upon this and didn't check any other files.
jejb@lingrow:~/git/linux> git grep 'match_table_t'|wc -l
49
I'll leave it as an exercise to you to figure out how many use the
style you're proposing.
There's definite advantage in uniformity and even if I accepted the
readability argument, which I don't, it's too small a reason to churn
nearly 50 files one at a time.
Regards,
James
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: Thorsten Blum @ 2025-10-09 13:30 UTC (permalink / raw)
To: James Bottomley
Cc: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <9e7488652ab73d7c5c2f93ea3c68253a9f08cd82.camel@HansenPartnership.com>
On 9. Oct 2025, at 14:44, James Bottomley wrote:
> On Thu, 2025-10-09 at 13:58 +0200, Thorsten Blum wrote:
>> Use designated initializers for 'key_format_tokens' and 'key_tokens'
>> to allow struct fields to be reordered more easily
>
> How does it improve that? The key,value pairs are surrounded by braces
> so we just cut and paste the lot anyway.
Using designated initializers (especially for global structs) allows the
fields of struct match_token from linux/parser.h to be reordered or
extended more easily, improving overall maintainability.
>> and to improve readability.
>
> I don't think I agree with this when looking through the code,
> especially because this is the way it's done for *every* option in the
> entire key subsystem. So firstly I really don't think it's helpful for
> only encrypted keys to be different from everything else and secondly
> when I read the code (as I often do to figure out what the options
> mean), the additional .token and .pattern just get in the way of what
> I'm looking for.
I just stumbled upon this and didn't check any other files.
I do think it helps with readability, especially for people unfamiliar
with the code. With designated initializers, it's clear what each value
represents without having to look up the definition of 'match_token' in
linux/parser.h.
Thanks,
Thorsten
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: James Bottomley @ 2025-10-09 12:44 UTC (permalink / raw)
To: Thorsten Blum, Mimi Zohar, David Howells, Jarkko Sakkinen,
Paul Moore, James Morris, Serge E. Hallyn
Cc: linux-integrity, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251009115817.368170-2-thorsten.blum@linux.dev>
On Thu, 2025-10-09 at 13:58 +0200, Thorsten Blum wrote:
> Use designated initializers for 'key_format_tokens' and 'key_tokens'
> to allow struct fields to be reordered more easily
How does it improve that? The key,value pairs are surrounded by braces
so we just cut and paste the lot anyway.
> and to improve readability.
I don't think I agree with this when looking through the code,
especially because this is the way it's done for *every* option in the
entire key subsystem. So firstly I really don't think it's helpful for
only encrypted keys to be different from everything else and secondly
when I read the code (as I often do to figure out what the options
mean), the additional .token and .pattern just get in the way of what
I'm looking for.
Regards,
James
^ permalink raw reply
* [PATCH] KEYS: encrypted: Use designated initializers for match_table_t structs
From: Thorsten Blum @ 2025-10-09 11:58 UTC (permalink / raw)
To: Mimi Zohar, David Howells, Jarkko Sakkinen, Paul Moore,
James Morris, Serge E. Hallyn
Cc: Thorsten Blum, linux-integrity, keyrings, linux-security-module,
linux-kernel
Use designated initializers for 'key_format_tokens' and 'key_tokens' to
allow struct fields to be reordered more easily and to improve
readability.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/keys/encrypted-keys/encrypted.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index aef438d18da8..76a6dab2f4d2 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -62,17 +62,17 @@ enum {
};
static const match_table_t key_format_tokens = {
- {Opt_default, "default"},
- {Opt_ecryptfs, "ecryptfs"},
- {Opt_enc32, "enc32"},
- {Opt_error, NULL}
+ { .token = Opt_default, .pattern = "default"},
+ { .token = Opt_ecryptfs, .pattern = "ecryptfs"},
+ { .token = Opt_enc32, .pattern = "enc32"},
+ { .token = Opt_error, .pattern = NULL}
};
static const match_table_t key_tokens = {
- {Opt_new, "new"},
- {Opt_load, "load"},
- {Opt_update, "update"},
- {Opt_err, NULL}
+ { .token = Opt_new, .pattern = "new"},
+ { .token = Opt_load, .pattern = "load"},
+ { .token = Opt_update, .pattern = "update"},
+ { .token = Opt_err, .pattern = NULL}
};
static bool user_decrypted_data = IS_ENABLED(CONFIG_USER_DECRYPTED_DATA);
--
2.51.0
^ permalink raw reply related
* [PATCH] ima: Attach CREDS_CHECK IMA hook to bprm_creds_from_file LSM hook
From: Roberto Sassu @ 2025-10-08 11:35 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, eric.snowberg, paul, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel,
Roberto Sassu, Matthew Garrett, Eric W . Biederman, Jann Horn,
Christian Brauner, Kees Cook
From: Roberto Sassu <roberto.sassu@huawei.com>
Since commit 56305aa9b6fa ("exec: Compute file based creds only once"), the
credentials to be applied to the process after execution are not calculated
anymore for each step of finding intermediate interpreters (including the
final binary), but only after the final binary to be executed without
interpreter has been found.
In particular, that means that the bprm_check_security LSM hook will not
see the updated cred->e[ug]id for the intermediate and for the final binary
to be executed, since the function doing this task has been moved from
prepare_binprm(), which calls the bprm_check_security hook, to
bprm_creds_from_file().
This breaks the IMA expectation for the CREDS_CHECK hook, introduced with
commit d906c10d8a31 ("IMA: Support using new creds in appraisal policy"),
which expects to evaluate "the credentials that will be committed when the
new process is started". This is clearly not the case for the CREDS_CHECK
IMA hook, which is attached to bprm_check_security.
This issue does not affect systems which load a policy with the BPRM_CHECK
hook with no other criteria, as is the case with the built-in "tcb" and/or
"appraise_tcb" IMA policies. The "tcb" built-in policy measures all
executions regardless of the new credentials, and the "appraise_tcb" policy
is written in terms of the file owner, rather than IMA hooks.
However, it does affect systems without a BPRM_CHECK policy rule or with a
BPRM_CHECK policy rule that does not include what CREDS_CHECK evaluates. As
an extreme example, taking a standalone rule like:
measure func=CREDS_CHECK euid=0
This will not measure for example sudo (because CREDS_CHECK still sees the
bprm->cred->euid set to the regular user UID), but only the subsequent
commands after the euid was applied to the children.
Make set[ug]id programs measured/appraised again by splitting
ima_bprm_check() in two separate hook implementations (CREDS_CHECK now
being implemented by ima_creds_check()), and by attaching CREDS_CHECK to
the bprm_creds_from_file LSM hook.
The limitation of this approach is that CREDS_CHECK will not be invoked
anymore for the intermediate interpreters, like it was before, but only for
the final binary. This limitation can be removed only by reverting commit
56305aa9b6fa ("exec: Compute file based creds only once").
Link: https://github.com/linux-integrity/linux/issues/3
Fixes: 56305aa9b6fa ("exec: Compute file based creds only once")
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
security/integrity/ima/ima_main.c | 42 ++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cdd225f65a62..ebaebccfbe9a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -573,18 +573,41 @@ static int ima_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
*/
static int ima_bprm_check(struct linux_binprm *bprm)
{
- int ret;
struct lsm_prop prop;
security_current_getlsmprop_subj(&prop);
- ret = process_measurement(bprm->file, current_cred(),
- &prop, NULL, 0, MAY_EXEC, BPRM_CHECK);
- if (ret)
- return ret;
-
- security_cred_getlsmprop(bprm->cred, &prop);
- return process_measurement(bprm->file, bprm->cred, &prop, NULL, 0,
- MAY_EXEC, CREDS_CHECK);
+ return process_measurement(bprm->file, current_cred(),
+ &prop, NULL, 0, MAY_EXEC, BPRM_CHECK);
+}
+
+/**
+ * ima_creds_check - based on policy, collect/store measurement.
+ * @bprm: contains the linux_binprm structure
+ * @file: contains the file descriptor of the binary being executed
+ *
+ * The OS protects against an executable file, already open for write,
+ * from being executed in deny_write_access() and an executable file,
+ * already open for execute, from being modified in get_write_access().
+ * So we can be certain that what we verify and measure here is actually
+ * what is being executed.
+ *
+ * The difference from ima_bprm_check() is that ima_creds_check() is invoked
+ * only after determining the final binary to be executed without interpreter,
+ * and not when searching for intermediate binaries. The reason is that since
+ * commit 56305aa9b6fab ("exec: Compute file based creds only once"), the
+ * credentials to be applied to the process are calculated only at that stage
+ * (bprm_creds_from_file security hook instead of bprm_check_security).
+ *
+ * On success return 0. On integrity appraisal error, assuming the file
+ * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
+ */
+static int ima_creds_check(struct linux_binprm *bprm, const struct file *file)
+{
+ struct lsm_prop prop;
+
+ security_current_getlsmprop_subj(&prop);
+ return process_measurement((struct file *)file, bprm->cred, &prop, NULL,
+ 0, MAY_EXEC, CREDS_CHECK);
}
/**
@@ -1242,6 +1265,7 @@ static int __init init_ima(void)
static struct security_hook_list ima_hooks[] __ro_after_init = {
LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec),
+ LSM_HOOK_INIT(bprm_creds_from_file, ima_creds_check),
LSM_HOOK_INIT(file_post_open, ima_file_check),
LSM_HOOK_INIT(inode_post_create_tmpfile, ima_post_create_tmpfile),
LSM_HOOK_INIT(file_release, ima_file_free),
--
2.43.0
^ permalink raw reply related
* Re: [LTP] [PATCH 2/4] ima_conditionals.sh: Split test by request
From: Petr Vorel @ 2025-10-08 4:38 UTC (permalink / raw)
To: Andrea Cervesato; +Cc: ltp, linux-integrity
In-Reply-To: <DDB7LLBEXIQM.2KSWEVEFU0TVS@suse.com>
> > verify_measurement() writes into /sys/kernel/security/ima/policy which on
> > kernels without CONFIG_IMA_WRITE_POLICY requires SUT reboot. Because LTP does
> > not support any reboot, this needs to be handled after test finishes. That's why
> > I separated the tests. This helps to cover more than the first test case out of
> > four.
> Now I see, this is clearly a corner case given by LTP limitations. I
> think we should probably start to think about reboot flag then. Once the
> PR for the new SUT will be implemented in kirk, it's time to work on
> this.
I would not wait for this as this is a long term project (on both LTP side and
IMA tests being converted into shell API at least). Once there is this
functionality implemented the split can be reverted.
Kind regards,
Petr
^ permalink raw reply
* Re: [PATCH] tpm: infineon: add bounds check in tpm_inf_recv
From: Jarkko Sakkinen @ 2025-10-07 19:25 UTC (permalink / raw)
To: Shahriyar Jalayeri; +Cc: peterhuewe, jgg, linux-integrity, linux-kernel
In-Reply-To: <20251007090736.17942-1-shahriyar@posteo.de>
On Tue, Oct 07, 2025 at 09:07:39AM +0000, Shahriyar Jalayeri wrote:
> Add two buffer size validations to prevent buffer overflows in
> tpm_inf_recv():
>
> 1. Validate that the provided buffer can hold at least the 4-byte header
> before attempting to read it.
> 2. Validate that the buffer is large enough to hold the data size reported
> by the TPM before reading the payload.
>
> Without these checks, a malicious or malfunctioning TPM could cause buffer
> overflows by reporting data sizes larger than the provided buffer, leading
> to memory corruption.
>
> Fixes: ebb81fdb3dd0 ("[PATCH] tpm: Support for Infineon TPM")
> Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
> ---
> drivers/char/tpm/tpm_infineon.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> index 7638b65b8..8b90a8191 100644
> --- a/drivers/char/tpm/tpm_infineon.c
> +++ b/drivers/char/tpm/tpm_infineon.c
> @@ -250,6 +250,11 @@ static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf, size_t count)
> number_of_wtx = 0;
>
> recv_begin:
> + /* expect at least 1-byte VL header, 1-byte ctrl-tag, 2-byte data size */
> + if (count < 4) {
> + return -EIO;
> + }
> +
> /* start receiving header */
> for (i = 0; i < 4; i++) {
> ret = wait(chip, STAT_RDA);
> @@ -268,6 +273,10 @@ static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf, size_t count)
> /* size of the data received */
> size = ((buf[2] << 8) | buf[3]);
>
> + if (size + 6 > count) {
> + return -EIO;
> + }
> +
> for (i = 0; i < size; i++) {
> wait(chip, STAT_RDA);
> buf[i] = tpm_data_in(RDFIFO);
> --
> 2.43.0
>
Nitpick: we don't use curly braces for one line statements.
AFAIK scripts/checkpatch.pl complains about this. Other than that
I don't see any issues.
BR, Jarkko
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-07 14:38 UTC (permalink / raw)
To: James Bottomley
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel
In-Reply-To: <aOUkcytS21zQs71I@kernel.org>
On Tue, Oct 07, 2025 at 05:32:24PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 06, 2025 at 07:57:10PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 06, 2025 at 07:51:51PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 06, 2025 at 10:33:40AM -0400, James Bottomley wrote:
> > > > On Mon, 2025-10-06 at 17:12 +0300, Jarkko Sakkinen wrote:
> > > > > 2. Null seed was extremely bad idea. The way I'm planning to actually
> > > > > fix this is to parametrize the primary key to a persistent key
> > > > > handle
> > > > > stored into nvram of the chip instead of genration. This will
> > > > > address
> > > > > also ambiguity and can be linked directly to vendor ceritifcate
> > > > > for e.g. to perfom remote attesttion.
> > > >
> > > > Just a minute, there's been no discussion or debate about this on the
> > > > list. The rationale for using the NULL seed is clearly laid out here:
> > > >
> > > > https://docs.kernel.org/security/tpm/tpm-security.html
> > > >
> > > > But in brief it is the only way to detect reset attacks against the TPM
> > > > and a reset attack is the single simplest attack an interposer can do.
> > > >
> > > > If you think there's a problem with the approach, by all means let's
> > > > have a debate, since TPM security is always a trade off, but you can't
> > > > simply come to your own opinion and try to impose it by fiat without at
> > > > least raising whatever issue you think you've found with the parties
> > > > who contributed the code in the first place.
> > >
> > > Ok fair enough, it's quite context dependent what is not secure and
> > > what is secure.
> > >
> > > What I've thought, or have planned to implement, is not to discard null
> > > seed but instead parmetrize the primary key as a kernel command-line
> > > parameter.
> > >
> > > E.g. "tpm.integrity_key={off,null,handle}" and
> > > "tpm.integrity_key_handle" to specify an NV index. The default value is
> > > off and I think also that with this change and possibly with some
> > > additional polishing it can reappear in default config,
> > >
> > > This out of context for the PR but I will take your comment into account
> > > in the pull request.
> > >
> > > My main issue preventing sending a new pull request is that weird list
> > > of core TPM2 features that is claimed "not to be required" with zero
> > > references. Especially it is contraditory claim that TPM2_CreatePrimary
> > > would be optional feature as the whole chip standard is based on three
> > > random seeds from which primary keys are templated and used as root
> > > keys for other keys.
> > >
> > > So I guess I cherry-pick the claims from Chris' patch that I can cope
> > > with, look what I wrote to my commit and adjust that accordingly and
> > > finally write a tag message with summarization of all this. I exactly
> > > drop the arguments with no quantitative evidence, which is probably
> > > a sane way to move forward.
> >
> > Personally I think that once there's correctly implemented command-line
> > option, the feature flag is somewhat redundant (and we've never had one
> > for /dev/tpmrm0). And it will help a lot with kernel QA as you can run
> > tests with same kernel image without recompilation.
>
> I don't really see any possible security issues either with null seed.
>
> It's no different as per remote attestation when compared storage keys.
> In a power cycle it's like same as per TPM2_Certify is considered. It's
> pretty much exactly performance issues but depending on deployment.
> Sometimes storage key root would be probably a better choice.
>
> I really tried to dig something else than exactly perf stuff but was
> unsuccessful to find anything, and I've actually done a lot of work
> at work on remote attestation so everything is also somewhat fresh
> on my mind.
>
> Still rooting to perf, immediate action being default option disable,
> and long term action being replacing the compilation option with
> kernel command-line options. I.e., I'll stay on track what I'e
> been already doing for some time :-)
>
> That said, my PR cover letter (or the tag message) did suck and
> I'll just address next during exactly why something is or isn't
> an issue. I think this is really good outcome for everyone in
> the long run (because everyone will get the outcome they were
> looking for).
And in the business I'm ATM i.e., covering like with 20-30% market share
Finnish high school IT deplyoment going extremes is feasible, as some
kids are smart and capable of hacking the systems so to speak ;-) I'd
likely enable this feature e.g., in that type of deployment together
with remote attesation just to max out defence in depth.
BR, Jarkko
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-07 14:32 UTC (permalink / raw)
To: James Bottomley
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel
In-Reply-To: <aOP04Yy3m23E4kjf@kernel.org>
On Mon, Oct 06, 2025 at 07:57:10PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 06, 2025 at 07:51:51PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 06, 2025 at 10:33:40AM -0400, James Bottomley wrote:
> > > On Mon, 2025-10-06 at 17:12 +0300, Jarkko Sakkinen wrote:
> > > > 2. Null seed was extremely bad idea. The way I'm planning to actually
> > > > fix this is to parametrize the primary key to a persistent key
> > > > handle
> > > > stored into nvram of the chip instead of genration. This will
> > > > address
> > > > also ambiguity and can be linked directly to vendor ceritifcate
> > > > for e.g. to perfom remote attesttion.
> > >
> > > Just a minute, there's been no discussion or debate about this on the
> > > list. The rationale for using the NULL seed is clearly laid out here:
> > >
> > > https://docs.kernel.org/security/tpm/tpm-security.html
> > >
> > > But in brief it is the only way to detect reset attacks against the TPM
> > > and a reset attack is the single simplest attack an interposer can do.
> > >
> > > If you think there's a problem with the approach, by all means let's
> > > have a debate, since TPM security is always a trade off, but you can't
> > > simply come to your own opinion and try to impose it by fiat without at
> > > least raising whatever issue you think you've found with the parties
> > > who contributed the code in the first place.
> >
> > Ok fair enough, it's quite context dependent what is not secure and
> > what is secure.
> >
> > What I've thought, or have planned to implement, is not to discard null
> > seed but instead parmetrize the primary key as a kernel command-line
> > parameter.
> >
> > E.g. "tpm.integrity_key={off,null,handle}" and
> > "tpm.integrity_key_handle" to specify an NV index. The default value is
> > off and I think also that with this change and possibly with some
> > additional polishing it can reappear in default config,
> >
> > This out of context for the PR but I will take your comment into account
> > in the pull request.
> >
> > My main issue preventing sending a new pull request is that weird list
> > of core TPM2 features that is claimed "not to be required" with zero
> > references. Especially it is contraditory claim that TPM2_CreatePrimary
> > would be optional feature as the whole chip standard is based on three
> > random seeds from which primary keys are templated and used as root
> > keys for other keys.
> >
> > So I guess I cherry-pick the claims from Chris' patch that I can cope
> > with, look what I wrote to my commit and adjust that accordingly and
> > finally write a tag message with summarization of all this. I exactly
> > drop the arguments with no quantitative evidence, which is probably
> > a sane way to move forward.
>
> Personally I think that once there's correctly implemented command-line
> option, the feature flag is somewhat redundant (and we've never had one
> for /dev/tpmrm0). And it will help a lot with kernel QA as you can run
> tests with same kernel image without recompilation.
I don't really see any possible security issues either with null seed.
It's no different as per remote attestation when compared storage keys.
In a power cycle it's like same as per TPM2_Certify is considered. It's
pretty much exactly performance issues but depending on deployment.
Sometimes storage key root would be probably a better choice.
I really tried to dig something else than exactly perf stuff but was
unsuccessful to find anything, and I've actually done a lot of work
at work on remote attestation so everything is also somewhat fresh
on my mind.
Still rooting to perf, immediate action being default option disable,
and long term action being replacing the compilation option with
kernel command-line options. I.e., I'll stay on track what I'e
been already doing for some time :-)
That said, my PR cover letter (or the tag message) did suck and
I'll just address next during exactly why something is or isn't
an issue. I think this is really good outcome for everyone in
the long run (because everyone will get the outcome they were
looking for).
BR, Jarkko
^ permalink raw reply
* [PATCH] tpm: infineon: add bounds check in tpm_inf_recv
From: Shahriyar Jalayeri @ 2025-10-07 9:07 UTC (permalink / raw)
To: jarkko; +Cc: shahriyar, peterhuewe, jgg, linux-integrity, linux-kernel
Add two buffer size validations to prevent buffer overflows in
tpm_inf_recv():
1. Validate that the provided buffer can hold at least the 4-byte header
before attempting to read it.
2. Validate that the buffer is large enough to hold the data size reported
by the TPM before reading the payload.
Without these checks, a malicious or malfunctioning TPM could cause buffer
overflows by reporting data sizes larger than the provided buffer, leading
to memory corruption.
Fixes: ebb81fdb3dd0 ("[PATCH] tpm: Support for Infineon TPM")
Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
---
drivers/char/tpm/tpm_infineon.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 7638b65b8..8b90a8191 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -250,6 +250,11 @@ static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf, size_t count)
number_of_wtx = 0;
recv_begin:
+ /* expect at least 1-byte VL header, 1-byte ctrl-tag, 2-byte data size */
+ if (count < 4) {
+ return -EIO;
+ }
+
/* start receiving header */
for (i = 0; i < 4; i++) {
ret = wait(chip, STAT_RDA);
@@ -268,6 +273,10 @@ static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf, size_t count)
/* size of the data received */
size = ((buf[2] << 8) | buf[3]);
+ if (size + 6 > count) {
+ return -EIO;
+ }
+
for (i = 0; i < size; i++) {
wait(chip, STAT_RDA);
buf[i] = tpm_data_in(RDFIFO);
--
2.43.0
^ permalink raw reply related
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: James Bottomley @ 2025-10-06 23:11 UTC (permalink / raw)
To: Jonathan McDowell, Jarkko Sakkinen
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel, Jonathan McDowell
In-Reply-To: <aOQ3V6M-wWQxxCWK@earth.li>
On Mon, 2025-10-06 at 22:40 +0100, Jonathan McDowell wrote:
> What should we do if we get O_EXCL and O_RDONLY? Return an error?
> Ignore the O_EXCL flag?
As a command/response interface, it can't be operated read only ... so
I think we should just return -EINVAL (or something more appropriate)
if someone tries to open it O_RDONLY regardless of O_EXCL.
Regards,
James
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Linus Torvalds @ 2025-10-06 22:09 UTC (permalink / raw)
To: Jonathan McDowell
Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel, Jonathan McDowell
In-Reply-To: <aOQ3V6M-wWQxxCWK@earth.li>
On Mon, 6 Oct 2025 at 14:40, Jonathan McDowell <noodles@earth.li> wrote:
>
> I'm not sure _reads_ make sense for the TPM device files at all. It's a
> command + response interface.
Yeah, I won't disagree. My main worry was one of "existing users doing
odd things shouldn't then get new semantics"
I don't actually _expect_ people to do odd things wrt /dev/tpm and
friends, but I've so often been surprised by the odd things that do
happen that...
> What should we do if we get O_EXCL and O_RDONLY? Return an error? Ignore
> the O_EXCL flag?
Either of those likely is perfectly fine. Just ignoring it is the one
that adds the smallest possible ABI change, so in that sense it's the
"safer" option.
But you are probably right that there aren't any users that open that
thing read-only in the first place, much less then using O_EXCL.
Except I've seen programs that "work" by virtue of having done
entirely the wrong thing that just happened to work. Things like using
the wrong values entirely for 'flags' to open(), but the bits we cared
about happened to be ok, so...
And we have *traditionally* very much ignored O_EXCL when not paired
with O_CREAT (and even then it's iffy with NFS).
So people might just have random crud in the flags. That's why I
reacted originally: O_EXCL does make sense as a "open exclusively"
based on the name, but realistically we never actually *use* it that
way except for block devices.
So we *have* had those semantics for block devices, and they make
sense, and I don't mind the change at all per se.
But this is the kind of thing that I react to just because we have had
unexpected behavior in the past when a bit goes from "don't care" to
"meaningful".
Put another way: I'm probably just being unreasonably worried.
Linus
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jonathan McDowell @ 2025-10-06 21:40 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel, Jonathan McDowell
In-Reply-To: <aOO3NKegSrUQ4ewg@kernel.org>
On Mon, Oct 06, 2025 at 03:33:56PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 05, 2025 at 11:24:09AM -0700, Linus Torvalds wrote:
> > On Sun, 5 Oct 2025 at 08:47, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > The exclusive access looks debatable to me too. I think you should
> > also require that the open was done not only with O_EXCL, but as a
> > write too.
> >
> > Exclusive reads do not make sense.
>
> True, I agree with this.
I'm not sure _reads_ make sense for the TPM device files at all. It's a
command + response interface.
What should we do if we get O_EXCL and O_RDONLY? Return an error? Ignore
the O_EXCL flag?
> After reading this email I realized also another issue with these patch
> when I tested them sequentially building a VM for each commit ID.
>
> Without "tpm: Require O_EXCL for exclusive /dev/tpm access" applied,
> there's a regression: usually a daemon of some sort opens /dev/tpm0:
>
> COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
> tpm2-abrm 771444 tss 5u CHR 10,224 0t0 94 /dev/tpm0
>
> Without top patch this leaves /dev/tpmrm0 unusuable, which is a huge
> developer experience downgrade as it is nice and covenient device
> to try and do things. I.e. tail patch needs to be squashed and
> the whole patch set needs to be re-reviewed.
That's a fair point; I structured the patches in that fashion because I
felt the O_EXCL patch was potentially contentious and might not be
accepted.
> And based on this I'm happy to postpone O_EXCL changes to 6.19.
> Patch set just needs to be restructured better so that in-the
> middle of the series patches don't break things. And also it'd
> be better if this patch would be relocated as the first in the
> series: "tpm: Remove tpm_find_get_ops".
I'll spin a set with the tpm_find_get_ops removal first, then the O_EXCL
patch, then the other two, which I think fixes all the ordering
concerns.
J.
--
... Nice world. Let's make it weirder.
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-06 18:50 UTC (permalink / raw)
To: Jonathan McDowell
Cc: James Bottomley, Linus Torvalds, Peter Huewe, Jason Gunthorpe,
David Howells, keyrings, linux-integrity, linux-kernel
In-Reply-To: <aOP2DUj67yB0afUt@earth.li>
On Mon, Oct 06, 2025 at 06:02:05PM +0100, Jonathan McDowell wrote:
> On Mon, Oct 06, 2025 at 07:51:46PM +0300, Jarkko Sakkinen wrote:
> > My main issue preventing sending a new pull request is that weird list
> > of core TPM2 features that is claimed "not to be required" with zero
> > references. Especially it is contraditory claim that TPM2_CreatePrimary
> > would be optional feature as the whole chip standard is based on three
> > random seeds from which primary keys are templated and used as root
> > keys for other keys.
>
> My understanding here is that the main specification about what's
> "required" for TPMs to implement is from the PC Client Platform TPM
> Profile. There's no specific server PTP (though there is talk about
> creating one), so _most_ vendors just implement the PC Client PTP. It
> doesn't mean a TPM that doesn't do so isn't TPM compliant, just not PC
> Client PTP compliant.
>
> Google have taken the approach in their Titan based TPM implementation
> to avoid implementing features they don't need, to reduce attack
> surface.
If it is an internal product, it does not qualify for as an argument but
good that you brought this detail out. I mean the action is the same
and debate is really what are correct preconditions for taking any
action.
So I'll cover the next trial for PR with:
1. I'll retain my existing commit with no changes.
2. In the cover letter I can address the details brough by Chris with
the clause that Titan specific arguments are not basis for any
decision making, as it is wrong and does not scale given that
any possible company in this planet can have their own random
incompatible partial TPM implementation (and most likely there
are at least few of such).
3. I'll link Chris' message to the email. I.e. based on other
claims in that mail we can already fully justify the action
itself. I'm just knowingly ignoring that list of "incompatible"
features, and remark that so that the process is transparent.
>
> I'm not aware of anyone else who has done this.
>
> J.
>
> --
> ... You non-conformists are all alike.
BR, Jarkko
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jonathan McDowell @ 2025-10-06 17:02 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: James Bottomley, Linus Torvalds, Peter Huewe, Jason Gunthorpe,
David Howells, keyrings, linux-integrity, linux-kernel
In-Reply-To: <aOPzovsBYlH3ojTR@kernel.org>
On Mon, Oct 06, 2025 at 07:51:46PM +0300, Jarkko Sakkinen wrote:
> My main issue preventing sending a new pull request is that weird list
> of core TPM2 features that is claimed "not to be required" with zero
> references. Especially it is contraditory claim that TPM2_CreatePrimary
> would be optional feature as the whole chip standard is based on three
> random seeds from which primary keys are templated and used as root
> keys for other keys.
My understanding here is that the main specification about what's
"required" for TPMs to implement is from the PC Client Platform TPM
Profile. There's no specific server PTP (though there is talk about
creating one), so _most_ vendors just implement the PC Client PTP. It
doesn't mean a TPM that doesn't do so isn't TPM compliant, just not PC
Client PTP compliant.
Google have taken the approach in their Titan based TPM implementation
to avoid implementing features they don't need, to reduce attack
surface.
I'm not aware of anyone else who has done this.
J.
--
... You non-conformists are all alike.
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-06 16:57 UTC (permalink / raw)
To: James Bottomley
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel
In-Reply-To: <aOPzovsBYlH3ojTR@kernel.org>
On Mon, Oct 06, 2025 at 07:51:51PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 06, 2025 at 10:33:40AM -0400, James Bottomley wrote:
> > On Mon, 2025-10-06 at 17:12 +0300, Jarkko Sakkinen wrote:
> > > 2. Null seed was extremely bad idea. The way I'm planning to actually
> > > fix this is to parametrize the primary key to a persistent key
> > > handle
> > > stored into nvram of the chip instead of genration. This will
> > > address
> > > also ambiguity and can be linked directly to vendor ceritifcate
> > > for e.g. to perfom remote attesttion.
> >
> > Just a minute, there's been no discussion or debate about this on the
> > list. The rationale for using the NULL seed is clearly laid out here:
> >
> > https://docs.kernel.org/security/tpm/tpm-security.html
> >
> > But in brief it is the only way to detect reset attacks against the TPM
> > and a reset attack is the single simplest attack an interposer can do.
> >
> > If you think there's a problem with the approach, by all means let's
> > have a debate, since TPM security is always a trade off, but you can't
> > simply come to your own opinion and try to impose it by fiat without at
> > least raising whatever issue you think you've found with the parties
> > who contributed the code in the first place.
>
> Ok fair enough, it's quite context dependent what is not secure and
> what is secure.
>
> What I've thought, or have planned to implement, is not to discard null
> seed but instead parmetrize the primary key as a kernel command-line
> parameter.
>
> E.g. "tpm.integrity_key={off,null,handle}" and
> "tpm.integrity_key_handle" to specify an NV index. The default value is
> off and I think also that with this change and possibly with some
> additional polishing it can reappear in default config,
>
> This out of context for the PR but I will take your comment into account
> in the pull request.
>
> My main issue preventing sending a new pull request is that weird list
> of core TPM2 features that is claimed "not to be required" with zero
> references. Especially it is contraditory claim that TPM2_CreatePrimary
> would be optional feature as the whole chip standard is based on three
> random seeds from which primary keys are templated and used as root
> keys for other keys.
>
> So I guess I cherry-pick the claims from Chris' patch that I can cope
> with, look what I wrote to my commit and adjust that accordingly and
> finally write a tag message with summarization of all this. I exactly
> drop the arguments with no quantitative evidence, which is probably
> a sane way to move forward.
Personally I think that once there's correctly implemented command-line
option, the feature flag is somewhat redundant (and we've never had one
for /dev/tpmrm0). And it will help a lot with kernel QA as you can run
tests with same kernel image without recompilation.
BR, Jarkko
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-06 16:51 UTC (permalink / raw)
To: James Bottomley
Cc: Linus Torvalds, Peter Huewe, Jason Gunthorpe, David Howells,
keyrings, linux-integrity, linux-kernel
In-Reply-To: <125ba81bb222cdffef05ef9868c68002efd61235.camel@HansenPartnership.com>
On Mon, Oct 06, 2025 at 10:33:40AM -0400, James Bottomley wrote:
> On Mon, 2025-10-06 at 17:12 +0300, Jarkko Sakkinen wrote:
> > 2. Null seed was extremely bad idea. The way I'm planning to actually
> > fix this is to parametrize the primary key to a persistent key
> > handle
> > stored into nvram of the chip instead of genration. This will
> > address
> > also ambiguity and can be linked directly to vendor ceritifcate
> > for e.g. to perfom remote attesttion.
>
> Just a minute, there's been no discussion or debate about this on the
> list. The rationale for using the NULL seed is clearly laid out here:
>
> https://docs.kernel.org/security/tpm/tpm-security.html
>
> But in brief it is the only way to detect reset attacks against the TPM
> and a reset attack is the single simplest attack an interposer can do.
>
> If you think there's a problem with the approach, by all means let's
> have a debate, since TPM security is always a trade off, but you can't
> simply come to your own opinion and try to impose it by fiat without at
> least raising whatever issue you think you've found with the parties
> who contributed the code in the first place.
Ok fair enough, it's quite context dependent what is not secure and
what is secure.
What I've thought, or have planned to implement, is not to discard null
seed but instead parmetrize the primary key as a kernel command-line
parameter.
E.g. "tpm.integrity_key={off,null,handle}" and
"tpm.integrity_key_handle" to specify an NV index. The default value is
off and I think also that with this change and possibly with some
additional polishing it can reappear in default config,
This out of context for the PR but I will take your comment into account
in the pull request.
My main issue preventing sending a new pull request is that weird list
of core TPM2 features that is claimed "not to be required" with zero
references. Especially it is contraditory claim that TPM2_CreatePrimary
would be optional feature as the whole chip standard is based on three
random seeds from which primary keys are templated and used as root
keys for other keys.
So I guess I cherry-pick the claims from Chris' patch that I can cope
with, look what I wrote to my commit and adjust that accordingly and
finally write a tag message with summarization of all this. I exactly
drop the arguments with no quantitative evidence, which is probably
a sane way to move forward.
>
> Regards,
>
> James
BR, Jarkko
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: James Bottomley @ 2025-10-06 14:33 UTC (permalink / raw)
To: Jarkko Sakkinen, Linus Torvalds
Cc: Peter Huewe, Jason Gunthorpe, David Howells, keyrings,
linux-integrity, linux-kernel
In-Reply-To: <aOPOZwp_inGui9Bx@kernel.org>
On Mon, 2025-10-06 at 17:12 +0300, Jarkko Sakkinen wrote:
> 2. Null seed was extremely bad idea. The way I'm planning to actually
> fix this is to parametrize the primary key to a persistent key
> handle
> stored into nvram of the chip instead of genration. This will
> address
> also ambiguity and can be linked directly to vendor ceritifcate
> for e.g. to perfom remote attesttion.
Just a minute, there's been no discussion or debate about this on the
list. The rationale for using the NULL seed is clearly laid out here:
https://docs.kernel.org/security/tpm/tpm-security.html
But in brief it is the only way to detect reset attacks against the TPM
and a reset attack is the single simplest attack an interposer can do.
If you think there's a problem with the approach, by all means let's
have a debate, since TPM security is always a trade off, but you can't
simply come to your own opinion and try to impose it by fiat without at
least raising whatever issue you think you've found with the parties
who contributed the code in the first place.
Regards,
James
^ permalink raw reply
* Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18
From: Jarkko Sakkinen @ 2025-10-06 14:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Huewe, Jason Gunthorpe, David Howells, keyrings,
linux-integrity, linux-kernel
In-Reply-To: <aOPPpVK8rJUuDgWD@kernel.org>
On Mon, Oct 06, 2025 at 05:18:13PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 06, 2025 at 05:13:02PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 06, 2025 at 02:58:17PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 05, 2025 at 11:09:08AM -0700, Linus Torvalds wrote:
> > > > On Sun, 5 Oct 2025 at 08:47, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > >
> > > > > This pull request disables
> > > > > TCG_TPM2_HMAC from the default configuration as it does not perform well
> > > > > enough [1].
> > > > >
> > > > > [1] https://lore.kernel.org/linux-integrity/20250825203223.629515-1-jarkko@kernel.org/
> > > >
> > > > This link is entirely useless, and doesn't explain what the problem
> > > > was and *why* TPM2_TCG_HMAC shouldn't be on by default.
> > > >
> > > > I think a much better link is
> > > >
> > > > https://lore.kernel.org/linux-integrity/20250814162252.3504279-1-cfenn@google.com/
> > > >
> > > > which talks about the problems that TPM2_TCG_HMAC causes.
> > > >
> > > > Which weren't just about "not performing well enough", but actually
> > > > about how it breaks TPM entirely for some cases.
> > >
> > > Fair enough. I'll also enumerate the issues, and also roadmap
> > > to heal the feature.
> >
> > So some of the arguments in Chris' email are debatable, such as
> > this list:
> >
> > - TPM_RH_NULL
> > - TPM2_CreatePrimary
> > - TPM2_ContextSave
> > - ECDH-P256
> > - AES-128-CFB
> >
> >
> > We've never encountered a TPM chip without those TPM commands, and e.g.
> > /dev/tpmrm0 heavily relies on TPM2_ContextSave, and has been in the
> > mainline since 2017. And further, this has been the case on ARM too.
> >
> > So using all of the arguments as rationale for the change that according
> > to Chris' email are broken because I cannnot objectively on all of the
> > arguments.
> >
> > E.g. I have to assume to this day that all known TPM chips have those
> > commands because no smoking gun exists. And if DID exist, then I'd
> > assume someone would fixed /dev/tpmrm0 ages ago.
> >
> > That said, I do agree on disabling the feature for the time being:
> > we have consensus on actions but not really on stimulus tbh.
> > And if there is stimulus I would postpone this patch to create
> > fix also for /dev/tpmrm0.
> >
> > Argument where I meet with Chris suggestion are:
> >
> > 1. Performance. The key generation during boot is extremely bad
> > idea and depending on the deployment the encryption cost is
> > too much (e.g. with my laptop having fTPM it does not really
> > matter).
> > 2. Null seed was extremely bad idea. The way I'm planning to actually
> > fix this is to parametrize the primary key to a persistent key handle
> > stored into nvram of the chip instead of genration. This will address
> > also ambiguity and can be linked directly to vendor ceritifcate
> > for e.g. to perfom remote attesttion.
> >
> > Things don't go broken by saying that they are broken and nothing
> > elsewhere in the mainline has supporting evidence that those commands
> > would be optional. I cannot agree on argument which I have zero
> > means to measure in any possible way.
> >
> > This is exactly also the root reason why I wrote my own commit instead
> > with the same change: I could have never signed off the commit that
> > I don't believe is true in its storyline.
> >
> > So if I write cover for the pull request where I use the subset of
> > arguments with shared consensus would that be enough to get this
> > through? As for primary key handle fix I rather do that with
> > time and proper care.
>
> I had to use few hours to remind why I did my commit instead of acking
> the original and this is the root. We've never had e.g. a bug in the
> wild that would /dev/tpmrm0 to be broken because ContextSave is not
> available, and it is *widely* used device across all major platforms.
Here's mobile client profile:
https://trustedcomputinggroup.org/wp-content/uploads/TPM_2.0_Mobile_Common_Profile_v2r31_FINAL.pdf
Unless I missed a tidbit I see nothing in it saying that ContextSave
would be optional. If there was even a known legit spec bringing some
context to the claims, that would move things forward.
Section 2.3 states this about ContextSave:
"The symmetric cipher mode TPM_ALG_CFB is REQUIRED by TCG TPM 2.0
Library specification Part 1 [1] and is also necessary for
implementation of TPM2_Create, TPM2_Load, TPM 2_ContextSave,
TPM2_ContextLoad, and other TPM commands"
which actually claims that TPM_ALG_CFB is required where as Chris'
patch claims 180 degrees opposite what the spec says.
Perhaps there's some other random TCG spec that I've missed, it's
entirely possible...
BR, Jarkko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox