* Re: [PATCH] KEYS: trusted: Re-orchestrate tpm2_read_public() calls
From: Jarkko Sakkinen @ 2025-12-05 0:49 UTC (permalink / raw)
To: linux-integrity
Cc: Peter Huewe, Jason Gunthorpe, James Bottomley, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
open list, open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <aTIXOr3rpI9xufTl@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]
On Fri, Dec 05, 2025 at 01:20:30AM +0200, Jarkko Sakkinen wrote:
> On Fri, Dec 05, 2025 at 12:31:27AM +0200, Jarkko Sakkinen wrote:
> > tpm2_load_cmd() and tpm2_unseal_cmd() use the same parent, and calls to
> > tpm_buf_append_name() cause the exact same TPM2_ReadPublic command to be
> > sent to the chip, causing unnecessary traffic.
> >
> > 1. Export tpm2_read_public in order to make it callable from 'trusted_tpm2'.
> > 2. Re-orchestrate tpm2_seal_trusted() and tpm2_unseal_trusted() in order to
> > halve the name resolutions required:
> > 2a. Move tpm2_read_public() calls into trusted_tpm2.
> > 2b. Pass TPM name to tpm_buf_append_name().
> > 2c. Rework tpm_buf_append_name() to use the pre-resolved name.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> If ASN.1 blob would contain also name of the parent then zero
> tpm2_read_public() calls would be required i.e., the main bottleneck
> here inherits from the limitations of the file format itself.
Along the lines of attached patch.
BR, Jarkko
[-- Attachment #2: 0001-KEYS-trusted-Extend-TPMKey-ASN.1-definition-with-par.patch --]
[-- Type: text/plain, Size: 7032 bytes --]
From c5fc7e38aae838dd1190b33545a8b1a1696e9ce8 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: Fri, 5 Dec 2025 01:53:33 +0200
Subject: [PATCH] KEYS: trusted: Extend TPMKey ASN.1 definition with
'parentName'
Extend TPMKey ASN.1 definition [1] with an optional 'parentName' attribute
containing TPMT_HA blob for the parent. Encode this attribute for the
generated TPM keys, which allows skipping TPM2_ReadPublic when unsealing
the key.
[1] https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.txt
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
security/keys/trusted-keys/tpm2key.asn1 | 17 ++++-
security/keys/trusted-keys/trusted_tpm2.c | 80 +++++++++++++++--------
2 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
index f57f869ad600..080f0e399982 100644
--- a/security/keys/trusted-keys/tpm2key.asn1
+++ b/security/keys/trusted-keys/tpm2key.asn1
@@ -1,11 +1,26 @@
---
--- ASN.1 for TPM 2.0 keys
---
+TPMPolicy ::= SEQUENCE {
+ commandCode [0] EXPLICIT INTEGER,
+ commandPolicy [1] EXPLICIT OCTET STRING
+}
+
+TPMAuthPolicy ::= SEQUENCE {
+ name [0] EXPLICIT UTF8String OPTIONAL,
+ policy [1] EXPLICIT SEQUENCE OF TPMPolicy
+}
TPMKey ::= SEQUENCE {
type OBJECT IDENTIFIER ({tpm2_key_type}),
emptyAuth [0] EXPLICIT BOOLEAN OPTIONAL,
+ policy [1] EXPLICIT SEQUENCE OF TPMPolicy OPTIONAL,
+ secret [2] EXPLICIT OCTET STRING OPTIONAL,
+ authPolicy [3] EXPLICIT SEQUENCE OF TPMAuthPolicy OPTIONAL,
+ description [4] EXPLICIT UTF8String OPTIONAL,
+ rsaParent [5] EXPLICIT BOOLEAN OPTIONAL,
+ parentName [6] EXPLICIT OCTET STRING ({tpm2_key_parent_name}),
parent INTEGER ({tpm2_key_parent}),
pubkey OCTET STRING ({tpm2_key_pub}),
privkey OCTET STRING ({tpm2_key_priv})
- }
+}
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 88bafbcc011a..85fd34457431 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -20,16 +20,26 @@
static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
+enum tpm_key_tag {
+ TPM_KEY_TAG_EMPTY_AUTH = 0,
+ TPM_KEY_TAG_POLICY = 1,
+ TPM_KEY_TAG_SECRET = 2,
+ TPM_KEY_TAG_AUTH_POLICY = 3,
+ TPM_KEY_TAG_DESCRIPTION = 4,
+ TPM_KEY_TAG_RSA_PARENT = 5,
+ TPM_KEY_TAG_PARENT_NAME = 6,
+};
+
static int tpm2_key_encode(struct trusted_key_payload *payload,
struct trusted_key_options *options,
- u8 *src, u32 len)
+ u8 *src, u32 len, u8 *parent_name,
+ u16 parent_name_size)
{
const int SCRATCH_SIZE = PAGE_SIZE;
- u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
- u8 *work = scratch, *work1;
- u8 *end_work = scratch + SCRATCH_SIZE;
- u8 *priv, *pub;
+ u8 *end_work, *end_name;
u16 priv_len, pub_len;
+ u8 *work, *work1;
+ u8 *priv, *pub;
int ret;
priv_len = get_unaligned_be16(src) + 2;
@@ -40,9 +50,13 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
pub_len = get_unaligned_be16(src) + 2;
pub = src;
+ u8 *scratch __free(kfree) = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
if (!scratch)
return -ENOMEM;
+ work = scratch;
+ end_work = scratch + SCRATCH_SIZE;
+
work = asn1_encode_oid(work, end_work, tpm2key_oid,
asn1_oid_len(tpm2key_oid));
@@ -50,13 +64,22 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
unsigned char bool[3], *w = bool;
/* tag 0 is emptyAuth */
w = asn1_encode_boolean(w, w + sizeof(bool), true);
- if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
- ret = PTR_ERR(w);
- goto err;
- }
- work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
+ if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+ return PTR_ERR(w);
+ work = asn1_encode_tag(work, end_work, TPM_KEY_TAG_EMPTY_AUTH,
+ bool, w - bool);
}
+ u8 *name_encoded __free(kfree) = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
+ if (!name_encoded)
+ return -ENOMEM;
+
+ end_name = asn1_encode_octet_string(name_encoded,
+ name_encoded + SCRATCH_SIZE,
+ parent_name, parent_name_size);
+ work = asn1_encode_tag(work, end_work, TPM_KEY_TAG_PARENT_NAME,
+ name_encoded, end_name - name_encoded);
+
/*
* Assume both octet strings will encode to a 2 byte definite length
*
@@ -65,8 +88,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
*/
if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
"BUG: scratch buffer is too small")) {
- ret = -EINVAL;
- goto err;
+ return -EINVAL;
}
work = asn1_encode_integer(work, end_work, options->keyhandle);
@@ -79,15 +101,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
if (IS_ERR(work1)) {
ret = PTR_ERR(work1);
pr_err("BUG: ASN.1 encoder failed with %d\n", ret);
- goto err;
+ return ret;
}
- kfree(scratch);
return work1 - payload->blob;
-
-err:
- kfree(scratch);
- return ret;
}
struct tpm2_key_context {
@@ -96,11 +113,13 @@ struct tpm2_key_context {
u32 pub_len;
const u8 *priv;
u32 priv_len;
+ const u8 *name;
+ u32 name_len;
};
static int tpm2_key_decode(struct trusted_key_payload *payload,
struct trusted_key_options *options,
- u8 **buf)
+ u8 **buf, u8 *parent_name, u16 *parent_name_size)
{
int ret;
struct tpm2_key_context ctx;
@@ -127,6 +146,8 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
blob += ctx.priv_len;
memcpy(blob, ctx.pub, ctx.pub_len);
+ memcpy(parent_name, ctx.name, ctx.name_len);
+ *parent_name_size = ctx.name_len;
return 0;
}
@@ -190,6 +211,16 @@ int tpm2_key_priv(void *context, size_t hdrlen,
return 0;
}
+int tpm2_key_parent_name(void *context, size_t hdrlen, unsigned char tag,
+ const void *value, size_t vlen)
+{
+ struct tpm2_key_context *ctx = context;
+
+ ctx->name = value;
+ ctx->name_len = vlen;
+
+ return 0;
+}
/**
* tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
*
@@ -347,7 +378,8 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
+ blob_len = tpm2_key_encode(payload, options, &buf.data[offset],
+ blob_len, parent_name, parent_name_size);
if (blob_len < 0)
rc = blob_len;
@@ -602,7 +634,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
* Try to decode the provided blob as an ASN.1 blob. Assume that the
* blob is in the legacy format if decoding does not end successfully.
*/
- rc = tpm2_key_decode(payload, options, &blob);
+ rc = tpm2_key_decode(payload, options, &blob, &parent_name[0], &parent_name_size);
if (rc) {
blob = payload->blob;
payload->old_format = 1;
@@ -617,12 +649,6 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
if (rc)
return rc;
- rc = tpm2_read_public(chip, options->keyhandle, parent_name);
- if (rc < 0)
- goto out;
-
- parent_name_size = rc;
-
rc = tpm2_load_cmd(chip, payload, options, parent_name,
parent_name_size, blob, &blob_handle);
if (rc)
--
2.39.5
^ permalink raw reply related
* [GIT PULL] capabilities update for v6.19
From: Serge E. Hallyn @ 2025-12-05 0:27 UTC (permalink / raw)
To: torvalds, linux-security-module, Linux Kernel Mailing List
Cc: Ryan Foster, Christian Brauner
Hi Linus,
This contains the capabilities changes for 6.19. There is only a single commit,
Clarify the rootid_owns_currentns
which introduces no functional change. Ryan Foster had sent a patch
to add testing of the security/commoncap.c:rootid_owns_currentns()
function. The patch pointed out that this function was not as clear
as it should be.
This commit has two purposes:
1. Clarify the intent of the function in the name
2. Split the function so that the base functionality is easier
to test from a kunit test.
This commit has been in linux-next since November 18 with no reported
issues. Ryan has posted an updated test patch based on this commit.
The following changes since commit 211ddde0823f1442e4ad052a2f30f050145ccada:
Linux 6.18-rc2 (2025-10-19 15:19:16 -1000)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/sergeh/linux.git tags/caps-pr-20251204
for you to fetch changes up to 9891d2f79a9fe9b77ad290f950eb8fa3e375330e:
Clarify the rootid_owns_currentns (2025-11-18 18:00:19 -0600)
----------------------------------------------------------------
Capabilities patch for v6.19
There is only a single commit,
Clarify the rootid_owns_currentns
which introduces no functional change. Ryan Foster had sent a patch
to add testing of the security/commoncap.c:rootid_owns_currentns()
function. The patch pointed out that this function was not as clear
as it should be.
This commit has two purposes:
1. Clarify the intent of the function in the name
2. Split the function so that the base functionality is easier
to test from a kunit test.
This commit has been in linux-next since November 18 with no reported
issues. Ryan has posted an updated test patch based on this commit.
----------------------------------------------------------------
Serge Hallyn (1):
Clarify the rootid_owns_currentns
security/commoncap.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Re-orchestrate tpm2_read_public() calls
From: Jarkko Sakkinen @ 2025-12-04 23:20 UTC (permalink / raw)
To: linux-integrity
Cc: Peter Huewe, Jason Gunthorpe, James Bottomley, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
open list, open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <20251204223128.435109-1-jarkko@kernel.org>
On Fri, Dec 05, 2025 at 12:31:27AM +0200, Jarkko Sakkinen wrote:
> tpm2_load_cmd() and tpm2_unseal_cmd() use the same parent, and calls to
> tpm_buf_append_name() cause the exact same TPM2_ReadPublic command to be
> sent to the chip, causing unnecessary traffic.
>
> 1. Export tpm2_read_public in order to make it callable from 'trusted_tpm2'.
> 2. Re-orchestrate tpm2_seal_trusted() and tpm2_unseal_trusted() in order to
> halve the name resolutions required:
> 2a. Move tpm2_read_public() calls into trusted_tpm2.
> 2b. Pass TPM name to tpm_buf_append_name().
> 2c. Rework tpm_buf_append_name() to use the pre-resolved name.
>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
If ASN.1 blob would contain also name of the parent then zero
tpm2_read_public() calls would be required i.e., the main bottleneck
here inherits from the limitations of the file format itself.
BR, Jarkko
^ permalink raw reply
* [PATCH] KEYS: trusted: Re-orchestrate tpm2_read_public() calls
From: Jarkko Sakkinen @ 2025-12-04 22:31 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, James Bottomley,
Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, open list, open list:KEYS-TRUSTED,
open list:SECURITY SUBSYSTEM
tpm2_load_cmd() and tpm2_unseal_cmd() use the same parent, and calls to
tpm_buf_append_name() cause the exact same TPM2_ReadPublic command to be
sent to the chip, causing unnecessary traffic.
1. Export tpm2_read_public in order to make it callable from 'trusted_tpm2'.
2. Re-orchestrate tpm2_seal_trusted() and tpm2_unseal_trusted() in order to
halve the name resolutions required:
2a. Move tpm2_read_public() calls into trusted_tpm2.
2b. Pass TPM name to tpm_buf_append_name().
2c. Rework tpm_buf_append_name() to use the pre-resolved name.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
drivers/char/tpm/tpm2-cmd.c | 3 +-
drivers/char/tpm/tpm2-sessions.c | 95 +++++------------
include/linux/tpm.h | 10 +-
security/keys/trusted-keys/trusted_tpm2.c | 124 ++++++++++++++--------
4 files changed, 118 insertions(+), 114 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 3a77be7ebf4a..1f561ad3bdcf 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -202,7 +202,8 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (!disable_pcr_integrity) {
- rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, (u8 *)&pcr_idx,
+ sizeof(u32));
if (rc) {
tpm_buf_destroy(&buf);
return rc;
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 4149379665c4..e33be09446ff 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -136,8 +136,8 @@ struct tpm2_auth {
* handle, but they are part of the session by name, which
* we must compute and remember
*/
- u32 name_h[AUTH_MAX_NAMES];
u8 name[AUTH_MAX_NAMES][2 + SHA512_DIGEST_SIZE];
+ u16 name_size_tbl[AUTH_MAX_NAMES];
};
#ifdef CONFIG_TCG_TPM2_HMAC
@@ -163,7 +163,17 @@ static int name_size(const u8 *name)
}
}
-static int tpm2_read_public(struct tpm_chip *chip, u32 handle, void *name)
+/**
+ * tpm2_read_public: Resolve TPM name for a handle
+ * @chip: TPM chip to use.
+ * @handle: TPM handle.
+ * @name: A buffer for returning the name blob. Must have a
+ * capacity of 'SHA512_DIGET_SIZE + 2' bytes at minimum
+ *
+ * Returns size of TPM handle name of success.
+ * Returns tpm_transmit_cmd error codes when TPM2_ReadPublic fails.
+ */
+int tpm2_read_public(struct tpm_chip *chip, u32 handle, void *name)
{
u32 mso = tpm2_handle_mso(handle);
off_t offset = TPM_HEADER_SIZE;
@@ -219,14 +229,16 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, void *name)
memcpy(name, &buf.data[offset], rc);
return name_size_alg;
}
+EXPORT_SYMBOL_GPL(tpm2_read_public);
#endif /* CONFIG_TCG_TPM2_HMAC */
/**
- * tpm_buf_append_name() - add a handle area to the buffer
- * @chip: the TPM chip structure
- * @buf: The buffer to be appended
- * @handle: The handle to be appended
- * @name: The name of the handle (may be NULL)
+ * tpm_buf_append_name() - Append a handle and store TPM name
+ * @chip: TPM chip to use.
+ * @buf: TPM buffer containing the TPM command in-transit.
+ * @handle: TPM handle to be appended.
+ * @name: TPM name of the handle
+ * @name_size: Size of the TPM name.
*
* In order to compute session HMACs, we need to know the names of the
* objects pointed to by the handles. For most objects, this is simply
@@ -243,15 +255,14 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, void *name)
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*
- * Ends the authorization session on failure.
+ * Returns zero on success.
+ * Returns -EIO when the authorization area state is malformed.
*/
int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name)
+ u32 handle, u8 *name, u16 name_size)
{
#ifdef CONFIG_TCG_TPM2_HMAC
- enum tpm2_mso_type mso = tpm2_handle_mso(handle);
struct tpm2_auth *auth;
- u16 name_size_alg;
int slot;
int ret;
#endif
@@ -276,36 +287,15 @@ int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
}
tpm_buf_append_u32(buf, handle);
auth->session += 4;
-
- if (mso == TPM2_MSO_PERSISTENT ||
- mso == TPM2_MSO_VOLATILE ||
- mso == TPM2_MSO_NVRAM) {
- if (!name) {
- ret = tpm2_read_public(chip, handle, auth->name[slot]);
- if (ret < 0)
- goto err;
-
- name_size_alg = ret;
- }
- } else {
- if (name) {
- dev_err(&chip->dev, "handle 0x%08x does not use a name\n",
- handle);
- ret = -EIO;
- goto err;
- }
- }
-
- auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size_alg);
+ memcpy(auth->name[slot], name, name_size);
+ auth->name_size_tbl[slot] = name_size;
#endif
return 0;
#ifdef CONFIG_TCG_TPM2_HMAC
err:
tpm2_end_auth_session(chip);
- return tpm_ret_to_err(ret);
+ return ret;
#endif
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -613,22 +603,8 @@ int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
+ offset_s += handles * sizeof(u32);
- /*
- * just check the names, it's easy to make mistakes. This
- * would happen if someone added a handle via
- * tpm_buf_append_u32() instead of tpm_buf_append_name()
- */
- for (i = 0; i < handles; i++) {
- u32 handle = tpm_buf_read_u32(buf, &offset_s);
-
- if (auth->name_h[i] != handle) {
- dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
- ret = -EIO;
- goto err;
- }
- }
- /* point offset_s to the start of the sessions */
val = tpm_buf_read_u32(buf, &offset_s);
/* point offset_p to the start of the parameters */
offset_p = offset_s + val;
@@ -689,23 +665,8 @@ int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
/* ordinal is already BE */
sha256_update(&sctx, (u8 *)&head->ordinal, sizeof(head->ordinal));
/* add the handle names */
- for (i = 0; i < handles; i++) {
- enum tpm2_mso_type mso = tpm2_handle_mso(auth->name_h[i]);
-
- if (mso == TPM2_MSO_PERSISTENT ||
- mso == TPM2_MSO_VOLATILE ||
- mso == TPM2_MSO_NVRAM) {
- ret = name_size(auth->name[i]);
- if (ret < 0)
- goto err;
-
- sha256_update(&sctx, auth->name[i], ret);
- } else {
- __be32 h = cpu_to_be32(auth->name_h[i]);
-
- sha256_update(&sctx, (u8 *)&h, 4);
- }
- }
+ for (i = 0; i < handles; i++)
+ sha256_update(&sctx, auth->name[i], auth->name_size_tbl[i]);
if (offset_s != tpm_buf_length(buf))
sha256_update(&sctx, &buf->data[offset_s],
tpm_buf_length(buf) - offset_s);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 202da079d500..319ba75dd79a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -530,7 +530,7 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
}
int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name);
+ u32 handle, u8 *name, u16 name_size);
void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
u8 attributes, u8 *passphrase,
int passphraselen);
@@ -544,6 +544,7 @@ int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
int rc);
void tpm2_end_auth_session(struct tpm_chip *chip);
+int tpm2_read_public(struct tpm_chip *chip, u32 handle, void *name);
#else
#include <linux/unaligned.h>
@@ -567,6 +568,13 @@ static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
{
return rc;
}
+
+static inline int tpm2_read_public(struct tpm_chip *chip, u32 handle,
+ void *name)
+{
+ memcpy(name, &handle, sizeof(u32));
+ return sizeof(u32);
+}
#endif /* CONFIG_TCG_TPM2_HMAC */
#endif
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index a7ea4a1c3bed..88bafbcc011a 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -233,8 +233,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
+ u8 parent_name[2 + SHA512_DIGEST_SIZE];
off_t offset = TPM_HEADER_SIZE;
struct tpm_buf buf, sized;
+ u16 parent_name_size;
int blob_len = 0;
int hash;
u32 flags;
@@ -251,6 +253,12 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
if (rc)
return rc;
+ rc = tpm2_read_public(chip, options->keyhandle, parent_name);
+ if (rc < 0)
+ goto out_put;
+
+ parent_name_size = rc;
+
rc = tpm2_start_auth_session(chip);
if (rc)
goto out_put;
@@ -268,7 +276,8 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out_put;
}
- rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, parent_name,
+ parent_name_size);
if (rc)
goto out;
@@ -355,21 +364,25 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
}
/**
- * tpm2_load_cmd() - execute a TPM2_Load command
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: returned blob handle
+ * tpm2_load_cmd() - Execute TPM2_Load
+ * @chip: TPM chip to use.
+ * @payload: Key data in clear text.
+ * @options: Trusted key options.
+ * @parent_name: A cryptographic name, i.e. a TPMT_HA blob, of the
+ * parent key.
+ * @blob: The decoded payload for the key.
+ * @blob_handle: On success, will contain handle to the loaded keyedhash
+ * blob.
*
- * Return: 0 on success.
- * -E2BIG on wrong payload size.
- * -EPERM on tpm error status.
- * < 0 error from tpm_send.
+ * Return -E2BIG when the blob size is too small for all the data.
+ * Returns tpm_transmit_cmd() error codes when either TPM2_Load fails.
*/
static int tpm2_load_cmd(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options,
+ u8 *parent_name,
+ u16 parent_name_size,
+ const u8 *blob,
u32 *blob_handle)
{
u8 *blob_ref __free(kfree) = NULL;
@@ -377,27 +390,13 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
unsigned int private_len;
unsigned int public_len;
unsigned int blob_len;
- u8 *blob, *pub;
+ const u8 *pub;
int rc;
u32 attrs;
- rc = tpm2_key_decode(payload, options, &blob);
- if (rc) {
- /* old form */
- blob = payload->blob;
- payload->old_format = 1;
- } else {
- /* Bind for cleanup: */
- blob_ref = blob;
- }
-
- /* new format carries keyhandle but old format doesn't */
- if (!options->keyhandle)
- return -EINVAL;
-
/* must be big enough for at least the two be16 size counts */
if (payload->blob_len < 4)
- return -EINVAL;
+ return -E2BIG;
private_len = get_unaligned_be16(blob);
@@ -433,7 +432,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
return rc;
}
- rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, parent_name,
+ parent_name_size);
if (rc)
goto out;
@@ -465,20 +465,23 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
}
/**
- * tpm2_unseal_cmd() - execute a TPM2_Unload command
+ * tpm2_unseal_cmd() - Execute TPM2_Unload
*
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: blob handle
+ * @chip: TPM chip to use
+ * @payload: Key data in clear text.
+ * @options: Trusted key options.
+ * @parent_name: A cryptographic name, i.e. a TPMT_HA blob, of the
+ * parent key.
+ * @blob_handle: Handle to the loaded keyedhash blob.
*
- * Return: 0 on success
- * -EPERM on tpm error status
- * < 0 error from tpm_send
+ * Return -E2BIG when the blob size is too small for all the data.
+ * Returns tpm_transmit_cmd() error codes when either TPM2_Load fails.
*/
static int tpm2_unseal_cmd(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options,
+ u8 *parent_name,
+ u16 parent_name_size,
u32 blob_handle)
{
struct tpm_header *head;
@@ -498,7 +501,8 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
return rc;
}
- rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, parent_name,
+ parent_name_size);
if (rc)
goto out;
@@ -573,30 +577,60 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
}
/**
- * tpm2_unseal_trusted() - unseal the payload of a trusted key
+ * tpm2_unseal_trusted() - Unseal a trusted key
+ * @chip: TPM chip to use.
+ * @payload: Key data in clear text.
+ * @options: Trusted key options.
*
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- *
- * Return: Same as with tpm_send.
+ * Return -E2BIG when the blob size is too small for all the data.
+ * Return -EINVAL when parent's key handle has not been set.
+ * Returns tpm_transmit_cmd() error codes when either TPM2_Load or TPM2_Unseal
+ * fails.
*/
int tpm2_unseal_trusted(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
+ u8 *blob_ref __free(kfree) = NULL;
+ u8 parent_name[2 + SHA512_DIGEST_SIZE];
+ u16 parent_name_size;
u32 blob_handle;
+ u8 *blob;
int rc;
+ /*
+ * Try to decode the provided blob as an ASN.1 blob. Assume that the
+ * blob is in the legacy format if decoding does not end successfully.
+ */
+ rc = tpm2_key_decode(payload, options, &blob);
+ if (rc) {
+ blob = payload->blob;
+ payload->old_format = 1;
+ } else {
+ blob_ref = blob;
+ }
+
+ if (!options->keyhandle)
+ return -EINVAL;
+
rc = tpm_try_get_ops(chip);
if (rc)
return rc;
- rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
+ rc = tpm2_read_public(chip, options->keyhandle, parent_name);
+ if (rc < 0)
+ goto out;
+
+ parent_name_size = rc;
+
+ rc = tpm2_load_cmd(chip, payload, options, parent_name,
+ parent_name_size, blob, &blob_handle);
if (rc)
goto out;
- rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
+ rc = tpm2_unseal_cmd(chip, payload, options, parent_name,
+ parent_name_size, blob_handle);
+
tpm2_flush_context(chip, blob_handle);
out:
--
2.52.0
^ permalink raw reply related
* [PATCH] security: Add KUnit tests for kuid_root_in_ns and vfsuid_root_in_currentns
From: Ryan Foster @ 2025-12-04 21:56 UTC (permalink / raw)
To: linux-security-module; +Cc: serge, paul, linux-kernel, Ryan Foster
In-Reply-To: <aSXICxT6u0Rx1FhW@mail.hallyn.com>
Add comprehensive KUnit tests for the namespace-related capability
functions that Serge Hallyn refactored in commit 9891d2f79a9f
("Clarify the rootid_owns_currentns").
The tests verify:
- Basic functionality: UID 0 in init namespace, invalid vfsuid, non-zero UIDs
- Actual namespace traversal: Creating user namespaces with different UID
mappings where uid 0 maps to different kuids (e.g., 1000, 2000, 3000)
- Hierarchy traversal: Testing multiple nested namespaces to verify
correct namespace hierarchy traversal
This addresses the feedback to "test the actual functionality" by creating
real user namespaces with different values for the namespace's uid 0, rather
than just basic input validation.
The test file is included at the end of commoncap.c when
CONFIG_SECURITY_COMMONCAP_KUNIT_TEST is enabled, following the standard
kernel pattern (e.g., scsi_lib.c, ext4/mballoc.c). This allows tests to
access static functions in the same compilation unit without modifying
production code based on test configuration.
All 7 tests pass:
- test_vfsuid_root_in_currentns_init_ns
- test_vfsuid_root_in_currentns_invalid
- test_vfsuid_root_in_currentns_nonzero
- test_kuid_root_in_ns_init_ns_uid0
- test_kuid_root_in_ns_init_ns_nonzero
- test_kuid_root_in_ns_with_mapping
- test_kuid_root_in_ns_with_different_mappings
---
security/Kconfig | 17 +++
security/commoncap.c | 4 +
security/commoncap_test.c | 290 ++++++++++++++++++++++++++++++++++++++
3 files changed, 311 insertions(+)
create mode 100644 security/commoncap_test.c
diff --git a/security/Kconfig b/security/Kconfig
index 285f284dfcac..c7b3f42ef875 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -284,6 +284,23 @@ config LSM
If unsure, leave this as the default.
+config SECURITY_COMMONCAP_KUNIT_TEST
+ bool "Build KUnit tests for commoncap" if !KUNIT_ALL_TESTS
+ depends on KUNIT=y
+ default KUNIT_ALL_TESTS
+ help
+ This builds the commoncap KUnit tests.
+
+ KUnit tests run during boot and output the results to the debug log
+ in TAP format (https://testanything.org/). Only useful for kernel devs
+ running KUnit test harness and are not for inclusion into a
+ production build.
+
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
source "security/Kconfig.hardening"
endmenu
diff --git a/security/commoncap.c b/security/commoncap.c
index 8a23dfab7fac..3399535808fe 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1521,3 +1521,7 @@ DEFINE_LSM(capability) = {
};
#endif /* CONFIG_SECURITY */
+
+#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
+#include "commoncap_test.c"
+#endif
diff --git a/security/commoncap_test.c b/security/commoncap_test.c
new file mode 100644
index 000000000000..1088364a54e6
--- /dev/null
+++ b/security/commoncap_test.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * KUnit tests for commoncap.c security functions
+ *
+ * Tests for security-critical functions in the capability subsystem,
+ * particularly namespace-related capability checks.
+ */
+
+#include <kunit/test.h>
+#include <linux/user_namespace.h>
+#include <linux/uidgid.h>
+#include <linux/cred.h>
+#include <linux/mnt_idmapping.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/refcount.h>
+
+#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
+
+/* Functions are static in commoncap.c, but we can call them since we're
+ * included in the same compilation unit when tests are enabled.
+ */
+
+/**
+ * test_vfsuid_root_in_currentns_init_ns - Test vfsuid_root_in_currentns with init ns
+ *
+ * Verifies that UID 0 in the init namespace correctly owns the current
+ * namespace when running in init_user_ns.
+ *
+ * @test: KUnit test context
+ */
+static void test_vfsuid_root_in_currentns_init_ns(struct kunit *test)
+{
+ vfsuid_t vfsuid;
+ kuid_t kuid;
+
+ /* Create UID 0 in init namespace */
+ kuid = KUIDT_INIT(0);
+ vfsuid = VFSUIDT_INIT(kuid);
+
+ /* In init namespace, UID 0 should own current namespace */
+ KUNIT_EXPECT_TRUE(test, vfsuid_root_in_currentns(vfsuid));
+}
+
+/**
+ * test_vfsuid_root_in_currentns_invalid - Test vfsuid_root_in_currentns with invalid vfsuid
+ *
+ * Verifies that an invalid vfsuid correctly returns false.
+ *
+ * @test: KUnit test context
+ */
+static void test_vfsuid_root_in_currentns_invalid(struct kunit *test)
+{
+ vfsuid_t invalid_vfsuid;
+
+ /* Use the predefined invalid vfsuid */
+ invalid_vfsuid = INVALID_VFSUID;
+
+ /* Invalid vfsuid should return false */
+ KUNIT_EXPECT_FALSE(test, vfsuid_root_in_currentns(invalid_vfsuid));
+}
+
+/**
+ * test_vfsuid_root_in_currentns_nonzero - Test vfsuid_root_in_currentns with non-zero UID
+ *
+ * Verifies that a non-zero UID correctly returns false.
+ *
+ * @test: KUnit test context
+ */
+static void test_vfsuid_root_in_currentns_nonzero(struct kunit *test)
+{
+ vfsuid_t vfsuid;
+ kuid_t kuid;
+
+ /* Create a non-zero UID */
+ kuid = KUIDT_INIT(1000);
+ vfsuid = VFSUIDT_INIT(kuid);
+
+ /* Non-zero UID should return false */
+ KUNIT_EXPECT_FALSE(test, vfsuid_root_in_currentns(vfsuid));
+}
+
+/**
+ * test_kuid_root_in_ns_init_ns_uid0 - Test kuid_root_in_ns with init namespace and UID 0
+ *
+ * Verifies that kuid_root_in_ns correctly identifies UID 0 in init namespace.
+ * This tests the core namespace traversal logic. In init namespace, UID 0
+ * maps to itself, so it should own the namespace.
+ *
+ * @test: KUnit test context
+ */
+static void test_kuid_root_in_ns_init_ns_uid0(struct kunit *test)
+{
+ kuid_t kuid;
+ struct user_namespace *init_ns;
+
+ kuid = KUIDT_INIT(0);
+ init_ns = &init_user_ns;
+
+ /* UID 0 should own init namespace */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(kuid, init_ns));
+}
+
+/**
+ * test_kuid_root_in_ns_init_ns_nonzero - Test kuid_root_in_ns with init namespace and non-zero UID
+ *
+ * Verifies that kuid_root_in_ns correctly rejects non-zero UIDs in init namespace.
+ * Only UID 0 should own a namespace.
+ *
+ * @test: KUnit test context
+ */
+static void test_kuid_root_in_ns_init_ns_nonzero(struct kunit *test)
+{
+ kuid_t kuid;
+ struct user_namespace *init_ns;
+
+ kuid = KUIDT_INIT(1000);
+ init_ns = &init_user_ns;
+
+ /* Non-zero UID should not own namespace */
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(kuid, init_ns));
+}
+
+/**
+ * create_test_user_ns_with_mapping - Create a mock user namespace with UID mapping
+ *
+ * Creates a minimal user namespace structure for testing where uid 0 in the
+ * namespace maps to a specific kuid in the parent namespace.
+ *
+ * @test: KUnit test context
+ * @parent_ns: Parent namespace (typically init_user_ns)
+ * @mapped_kuid: The kuid that uid 0 in this namespace maps to in parent
+ *
+ * Returns: Pointer to allocated namespace, or NULL on failure
+ */
+static struct user_namespace *create_test_user_ns_with_mapping(struct kunit *test,
+ struct user_namespace *parent_ns,
+ kuid_t mapped_kuid)
+{
+ struct user_namespace *ns;
+ struct uid_gid_extent extent;
+
+ /* Allocate a test namespace - use kzalloc to zero all fields */
+ ns = kunit_kzalloc(test, sizeof(*ns), GFP_KERNEL);
+ if (!ns)
+ return NULL;
+
+ /* Initialize basic namespace structure fields */
+ ns->parent = parent_ns;
+ ns->level = parent_ns ? parent_ns->level + 1 : 0;
+ ns->owner = mapped_kuid;
+ ns->group = KGIDT_INIT(0);
+
+ /* Initialize ns_common structure */
+ refcount_set(&ns->ns.__ns_ref, 1);
+ ns->ns.inum = 0; /* Mock inum */
+
+ /* Set up uid mapping: uid 0 in this namespace maps to mapped_kuid in parent
+ * Format: first (uid in ns) : lower_first (kuid in parent) : count
+ * So: uid 0 in ns -> kuid mapped_kuid in parent
+ * This means from_kuid(ns, mapped_kuid) returns 0
+ */
+ extent.first = 0; /* uid 0 in this namespace */
+ extent.lower_first = __kuid_val(mapped_kuid); /* maps to this kuid in parent */
+ extent.count = 1;
+
+ ns->uid_map.extent[0] = extent;
+ ns->uid_map.nr_extents = 1;
+
+ /* Set up gid mapping: gid 0 maps to gid 0 in parent (simplified) */
+ extent.first = 0;
+ extent.lower_first = 0;
+ extent.count = 1;
+
+ ns->gid_map.extent[0] = extent;
+ ns->gid_map.nr_extents = 1;
+
+ return ns;
+}
+
+/**
+ * test_kuid_root_in_ns_with_mapping - Test kuid_root_in_ns with namespace where uid 0
+ * maps to different kuid
+ *
+ * Creates a user namespace where uid 0 maps to kuid 1000 in the parent namespace.
+ * Verifies that kuid_root_in_ns correctly identifies kuid 1000 as owning the namespace.
+ *
+ * Note: kuid_root_in_ns walks up the namespace hierarchy, so it checks the current
+ * namespace first, then parent, then parent's parent, etc. So:
+ * - kuid 1000 owns test_ns because from_kuid(test_ns, 1000) == 0
+ * - kuid 0 also owns test_ns because from_kuid(init_user_ns, 0) == 0
+ * (checked in parent)
+ *
+ * This tests the actual functionality as requested: creating namespaces with
+ * different values for the namespace's uid 0.
+ *
+ * @test: KUnit test context
+ */
+static void test_kuid_root_in_ns_with_mapping(struct kunit *test)
+{
+ struct user_namespace *test_ns;
+ struct user_namespace *parent_ns;
+ kuid_t mapped_kuid, other_kuid;
+
+ parent_ns = &init_user_ns;
+ mapped_kuid = KUIDT_INIT(1000);
+ other_kuid = KUIDT_INIT(2000);
+
+ test_ns = create_test_user_ns_with_mapping(test, parent_ns, mapped_kuid);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_ns);
+
+ /* kuid 1000 should own test_ns because it maps to uid 0 in test_ns */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(mapped_kuid, test_ns));
+
+ /* kuid 0 should also own test_ns (checked via parent init_user_ns) */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(0), test_ns));
+
+ /* Other kuids should not own test_ns */
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(other_kuid, test_ns));
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(KUIDT_INIT(500), test_ns));
+}
+
+/**
+ * test_kuid_root_in_ns_with_different_mappings - Test with multiple namespaces
+ *
+ * Creates multiple user namespaces with different UID mappings to verify
+ * that kuid_root_in_ns correctly handles different namespace hierarchies.
+ *
+ * Since kuid_root_in_ns walks up the hierarchy, kuids that map to 0 in init_user_ns
+ * will own all namespaces, while kuids that only map to 0 in specific namespaces
+ * will only own those namespaces and their children.
+ *
+ * @test: KUnit test context
+ */
+static void test_kuid_root_in_ns_with_different_mappings(struct kunit *test)
+{
+ struct user_namespace *ns1, *ns2, *ns3;
+
+ /* Create ns1 where uid 0 maps to kuid 1000 */
+ ns1 = create_test_user_ns_with_mapping(test, &init_user_ns, KUIDT_INIT(1000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns1);
+
+ /* Create ns2 where uid 0 maps to kuid 2000 */
+ ns2 = create_test_user_ns_with_mapping(test, &init_user_ns, KUIDT_INIT(2000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns2);
+
+ /* Create ns3 as a child of ns1 where uid 0 maps to kuid 3000 */
+ ns3 = create_test_user_ns_with_mapping(test, ns1, KUIDT_INIT(3000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns3);
+
+ /* Test ns1: kuid 1000 owns it, kuid 0 owns it (via parent), kuid 2000 does not */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(1000), ns1));
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(0), ns1));
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(KUIDT_INIT(2000), ns1));
+
+ /* Test ns2: kuid 2000 owns it, kuid 0 owns it (via parent), kuid 1000 does not */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(2000), ns2));
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(0), ns2));
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(KUIDT_INIT(1000), ns2));
+
+ /* Test ns3: kuid 3000 owns it, kuid 1000 owns it (via parent ns1),
+ * kuid 0 owns it (via init_user_ns), kuid 2000 does not
+ */
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(3000), ns3));
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(1000), ns3));
+ KUNIT_EXPECT_TRUE(test, kuid_root_in_ns(KUIDT_INIT(0), ns3));
+ KUNIT_EXPECT_FALSE(test, kuid_root_in_ns(KUIDT_INIT(2000), ns3));
+}
+
+static struct kunit_case commoncap_test_cases[] = {
+ KUNIT_CASE(test_vfsuid_root_in_currentns_init_ns),
+ KUNIT_CASE(test_vfsuid_root_in_currentns_invalid),
+ KUNIT_CASE(test_vfsuid_root_in_currentns_nonzero),
+ KUNIT_CASE(test_kuid_root_in_ns_init_ns_uid0),
+ KUNIT_CASE(test_kuid_root_in_ns_init_ns_nonzero),
+ KUNIT_CASE(test_kuid_root_in_ns_with_mapping),
+ KUNIT_CASE(test_kuid_root_in_ns_with_different_mappings),
+ {}
+};
+
+static struct kunit_suite commoncap_test_suite = {
+ .name = "commoncap",
+ .test_cases = commoncap_test_cases,
+};
+
+kunit_test_suite(commoncap_test_suite);
+
+MODULE_LICENSE("GPL");
+
+#endif /* CONFIG_SECURITY_COMMONCAP_KUNIT_TEST */
--
2.43.0
^ permalink raw reply related
* [PATCH v4 4/4] tpm2-sessions: Open code tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-12-04 19:47 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, open list,
Jarkko Sakkinen, Jonathan McDowell, James Bottomley, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <20251204194743.69035-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Open code 'tpm_buf_append_hmac_session_opt' to the call site, as it only
masks a call sequence and does otherwise nothing particularly useful.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
---
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
include/linux/tpm.h | 23 -----------------------
security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
3 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ce0a1c6b0596..3a77be7ebf4a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -282,9 +282,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
do {
tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
- | TPM2_SA_CONTINUE_SESSION,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf,
+ TPM2_SA_ENCRYPT |
+ TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
tpm_buf_append_u16(&buf, num_bytes);
err = tpm_buf_fill_hmac_session(chip, &buf);
if (err) {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index afa51723296a..202da079d500 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -536,29 +536,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
u8 *passphrase, int passphraselen);
-static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
- struct tpm_buf *buf,
- u8 attributes,
- u8 *passphrase,
- int passphraselen)
-{
- struct tpm_header *head;
- int offset;
-
- if (tpm2_chip_auth(chip)) {
- tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
- } else {
- offset = buf->handles * 4 + TPM_HEADER_SIZE;
- head = (struct tpm_header *)buf->data;
-
- /*
- * If the only sessions are optional, the command tag must change to
- * TPM2_ST_NO_SESSIONS.
- */
- if (tpm_buf_length(buf) == offset)
- head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
- }
-}
#ifdef CONFIG_TCG_TPM2_HMAC
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 5b205279584b..a7ea4a1c3bed 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -481,8 +481,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
struct trusted_key_options *options,
u32 blob_handle)
{
+ struct tpm_header *head;
struct tpm_buf buf;
u16 data_len;
+ int offset;
u8 *data;
int rc;
@@ -519,8 +521,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
tpm2_buf_append_auth(&buf, options->policyhandle,
NULL /* nonce */, 0, 0,
options->blobauth, options->blobauth_len);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
}
rc = tpm_buf_fill_hmac_session(chip, &buf);
--
2.52.0
^ permalink raw reply related
* [PATCH v4 1/4] tpm2-sessions: Fix out of range indexing in name_size
From: Jarkko Sakkinen @ 2025-12-04 19:47 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, open list, stable,
Jonathan McDowell, James Bottomley, Mimi Zohar, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, Ard Biesheuvel,
open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <20251204194743.69035-1-jarkko@kernel.org>
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID, which could lead into memory corruption at worst.
Address the issue by only processing known values and returning -EINVAL for
unrecognized values.
Make also 'tpm_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible so
that errors are detected before causing any spurious TPM traffic.
End also the authorization session on failure in both of the functions, as
the session state would be then by definition corrupted.
Cc: stable@vger.kernel.org # v6.10+
Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
Reviewed-by: Jonathan McDowell <noodles@meta.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v4:
- Removed spurious empty line.
v3:
- Fix pr_warn() format string in name_size().
- Fix !CONFIG_TPM2_HMAC compilation.
v2:
- Wrote a better short summary.
- Addressed remarks in https://lore.kernel.org/linux-integrity/aS8TIeviaippVAha@earth.li/
- Use -EIO consistently in tpm2_fill_hmac_session. These are not input value
errors. They could only spun from malformed (kernel) state.
- name_size did not have a proper default-case. Reorganize the
fallback into that.
---
drivers/char/tpm/tpm2-cmd.c | 23 +++-
drivers/char/tpm/tpm2-sessions.c | 132 +++++++++++++++-------
include/linux/tpm.h | 13 ++-
security/keys/trusted-keys/trusted_tpm2.c | 29 ++++-
4 files changed, 142 insertions(+), 55 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index dd502322f499..be4a9c7f2e1a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -199,7 +199,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (!disable_pcr_integrity) {
- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ return rc;
+ }
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -214,8 +218,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
chip->allocated_banks[i].digest_size);
}
- if (!disable_pcr_integrity)
- tpm_buf_fill_hmac_session(chip, &buf);
+ if (!disable_pcr_integrity) {
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ return rc;
+ }
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
if (!disable_pcr_integrity)
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
@@ -273,7 +283,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
| TPM2_SA_CONTINUE_SESSION,
NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
- tpm_buf_fill_hmac_session(chip, &buf);
+ err = tpm_buf_fill_hmac_session(chip, &buf);
+ if (err) {
+ tpm_buf_destroy(&buf);
+ return err;
+ }
+
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..385014dbca39 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -144,16 +144,23 @@ struct tpm2_auth {
/*
* Name Size based on TPM algorithm (assumes no hash bigger than 255)
*/
-static u8 name_size(const u8 *name)
+static int name_size(const u8 *name)
{
- static u8 size_map[] = {
- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
- };
- u16 alg = get_unaligned_be16(name);
- return size_map[alg] + 2;
+ u16 hash_alg = get_unaligned_be16(name);
+
+ switch (hash_alg) {
+ case TPM_ALG_SHA1:
+ return SHA1_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA256:
+ return SHA256_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA384:
+ return SHA384_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA512:
+ return SHA512_DIGEST_SIZE + 2;
+ default:
+ pr_warn("tpm: unsupported name algorithm: 0x%04x\n", hash_alg);
+ return -EINVAL;
+ }
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +168,7 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
struct tpm_header *head = (struct tpm_header *)buf->data;
off_t offset = TPM_HEADER_SIZE;
u32 tot_len = be32_to_cpu(head->length);
+ int ret;
u32 val;
/* we're starting after the header so adjust the length */
@@ -173,8 +181,13 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
offset += val;
/* name */
val = tpm_buf_read_u16(buf, &offset);
- if (val != name_size(&buf->data[offset]))
+ ret = name_size(&buf->data[offset]);
+ if (ret < 0)
+ return ret;
+
+ if (val != ret)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -221,46 +234,72 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* As with most tpm_buf operations, success is assumed because failure
* will be caused by an incorrect programming model and indicated by a
* kernel message.
+ *
+ * Ends the authorization session on failure.
*/
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name)
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name)
{
#ifdef CONFIG_TCG_TPM2_HMAC
enum tpm2_mso_type mso = tpm2_handle_mso(handle);
struct tpm2_auth *auth;
int slot;
+ int ret;
#endif
if (!tpm2_chip_auth(chip)) {
tpm_buf_append_handle(chip, buf, handle);
- return;
+ return 0;
}
#ifdef CONFIG_TCG_TPM2_HMAC
slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
if (slot >= AUTH_MAX_NAMES) {
- dev_err(&chip->dev, "TPM: too many handles\n");
- return;
+ dev_err(&chip->dev, "too many handles\n");
+ ret = -EIO;
+ goto err;
}
auth = chip->auth;
- WARN(auth->session != tpm_buf_length(buf),
- "name added in wrong place\n");
+ if (auth->session != tpm_buf_length(buf)) {
+ dev_err(&chip->dev, "session state malformed");
+ ret = -EIO;
+ goto err;
+ }
tpm_buf_append_u32(buf, handle);
auth->session += 4;
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- if (!name)
- tpm2_read_public(chip, handle, auth->name[slot]);
+ if (!name) {
+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
+ if (ret)
+ goto err;
+ }
} else {
- if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ if (name) {
+ dev_err(&chip->dev, "handle 0x%08x does not use a name\n",
+ handle);
+ ret = -EIO;
+ goto err;
+ }
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ ret = name_size(name);
+ if (ret < 0)
+ goto err;
+
+ memcpy(auth->name[slot], name, ret);
+ }
+#endif
+ return 0;
+
+#ifdef CONFIG_TCG_TPM2_HMAC
+err:
+ tpm2_end_auth_session(chip);
+ return tpm_ret_to_err(ret);
#endif
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -533,11 +572,9 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* encryption key and encrypts the first parameter of the command
* buffer with it.
*
- * As with most tpm_buf operations, success is assumed because failure
- * will be caused by an incorrect programming model and indicated by a
- * kernel message.
+ * Ends the authorization session on failure.
*/
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
{
u32 cc, handles, val;
struct tpm2_auth *auth = chip->auth;
@@ -549,9 +586,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u8 cphash[SHA256_DIGEST_SIZE];
struct sha256_ctx sctx;
struct hmac_sha256_ctx hctx;
+ int ret;
- if (!auth)
- return;
+ if (!auth) {
+ ret = -EIO;
+ goto err;
+ }
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -560,9 +600,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
i = tpm2_find_cc(chip, cc);
if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
+ dev_err(&chip->dev, "command 0x%08x not found\n", cc);
+ ret = -EIO;
+ goto err;
}
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +618,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u32 handle = tpm_buf_read_u32(buf, &offset_s);
if (auth->name_h[i] != handle) {
- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
- i);
- return;
+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
+ ret = -EIO;
+ goto err;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +651,14 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
offset_s += len;
}
if (offset_s != offset_p) {
- dev_err(&chip->dev, "TPM session length is incorrect\n");
- return;
+ dev_err(&chip->dev, "session length is incorrect\n");
+ ret = -EIO;
+ goto err;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ ret = -EIO;
+ goto err;
}
/* encrypt before HMAC */
@@ -646,8 +690,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- sha256_update(&sctx, auth->name[i],
- name_size(auth->name[i]));
+ ret = name_size(auth->name[i]);
+ if (ret < 0)
+ goto err;
+
+ sha256_update(&sctx, auth->name[i], ret);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +715,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
hmac_sha256_update(&hctx, &auth->attrs, 1);
hmac_sha256_final(&hctx, hmac);
+ return 0;
+
+err:
+ tpm2_end_auth_session(chip);
+ return ret;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 3d8f7d1ce2b8..aa816b144ab3 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -529,8 +529,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
#endif
}
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name);
+int tpm_buf_append_name(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);
@@ -563,7 +563,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
#ifdef CONFIG_TCG_TPM2_HMAC
int tpm2_start_auth_session(struct tpm_chip *chip);
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
int rc);
void tpm2_end_auth_session(struct tpm_chip *chip);
@@ -577,10 +577,13 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
static inline void tpm2_end_auth_session(struct tpm_chip *chip)
{
}
-static inline void tpm_buf_fill_hmac_session(struct tpm_chip *chip,
- struct tpm_buf *buf)
+
+static inline int tpm_buf_fill_hmac_session(struct tpm_chip *chip,
+ struct tpm_buf *buf)
{
+ return 0;
}
+
static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
struct tpm_buf *buf,
int rc)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8bc6efa8accb..5b205279584b 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -268,7 +268,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out_put;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -316,7 +319,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -427,7 +433,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -439,7 +448,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (!rc)
@@ -484,7 +496,9 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -509,7 +523,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
NULL, 0);
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v3 1/4] tpm2-sessions: fix out of range indexing in name_size
From: Jarkko Sakkinen @ 2025-12-04 18:47 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, open list, stable,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel,
open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <aTGkno0fzQMHXc7X@earth.li>
On Thu, Dec 04, 2025 at 03:11:26PM +0000, Jonathan McDowell wrote:
> On Thu, Dec 04, 2025 at 12:12:11AM +0200, Jarkko Sakkinen wrote:
> > 'name_size' does not have any range checks, and it just directly indexes
> > with TPM_ALG_ID, which could lead into memory corruption at worst.
> >
> > Address the issue by only processing known values and returning -EINVAL for
> > unrecognized values.
> >
> > Make also 'tpm_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible so
> > that errors are detected before causing any spurious TPM traffic.
> >
> > End also the authorization session on failure in both of the functions, as
> > the session state would be then by definition corrupted.
> >
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> A minor whitespace query below, but:
>
> Reviewed-by: Jonathan McDowell <noodles@meta.com>
Thanks. I updated the commit and removed the extra whitespace.
BR, Jarkko
^ permalink raw reply
* Re: Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: Stephen Smalley @ 2025-12-04 15:43 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Roberto Sassu, Bernd Edlinger, Alexander Viro, Alexey Dobriyan,
Oleg Nesterov, Kees Cook, Andy Lutomirski, Will Drewry,
Christian Brauner, Andrew Morton, Michal Hocko, Serge Hallyn,
James Morris, Randy Dunlap, Suren Baghdasaryan, Yafang Shao,
Helge Deller, Adrian Reber, Thomas Gleixner, Jens Axboe,
Alexei Starovoitov, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest, linux-mm,
linux-security-module, tiozhang, Luis Chamberlain,
Paulo Alcantara (SUSE), Sergey Senozhatsky, Frederic Weisbecker,
YueHaibing, Paul Moore, Aleksa Sarai, Stefan Roesch, Chao Yu,
xu xin, Jeff Layton, Jan Kara, David Hildenbrand, Dave Chinner,
Shuah Khan, Elena Reshetova, David Windsor, Mateusz Guzik,
Ard Biesheuvel, Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <87v7iqtcev.fsf_-_@email.froward.int.ebiederm.org>
On Mon, Dec 1, 2025 at 11:34 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Roberto Sassu <roberto.sassu@huaweicloud.com> writes:
>
> > + Mimi, linux-integrity (would be nice if we are in CC when linux-
> > security-module is in CC).
> >
> > Apologies for not answering earlier, it seems I don't receive the
> > emails from the linux-security-module mailing list (thanks Serge for
> > letting me know!).
> >
> > I see two main effects of this patch. First, the bprm_check_security
> > hook implementations will not see bprm->cred populated. That was a
> > problem before we made this patch:
> >
> > https://patchew.org/linux/20251008113503.2433343-1-roberto.sassu@huaweicloud.com/
>
> Thanks, that is definitely needed.
>
> Does calling process_measurement(CREDS_CHECK) on only the final file
> pass review? Do you know of any cases where that will break things?
>
> As it stands I don't think it should be assumed that any LSM has
> computed it's final creds until bprm_creds_from_file. Not just the
> uid and gid.
>
> If the patch you posted for review works that helps sort that mess out.
>
> > to work around the problem of not calculating the final DAC credentials
> > early enough (well, we actually had to change our CREDS_CHECK hook
> > behavior).
> >
> > The second, I could not check. If I remember well, unlike the
> > capability LSM, SELinux/Apparmor/SMACK calculate the final credentials
> > based on the first file being executed (thus the script, not the
> > interpreter). Is this patch keeping the same behavior despite preparing
> > the credentials when the final binary is found?
>
> The patch I posted was.
>
> My brain is still reeling from the realization that our security modules
> have the implicit assumption that it is safe to calculate their security
> information from shell scripts.
>
> In the first half of the 90's I remember there was lots of effort to try
> and make setuid shell scripts and setuid perl scripts work, and the
> final conclusion was it was a lost cause.
>
> Now I look at security_bprm_creds_for_exec and security_bprm_check which
> both have the implicit assumption that it is indeed safe to compute the
> credentials from a shell script.
>
> When passing a file descriptor to execat we have
> BINPRM_FLAGS_PATH_INACCESSIBLE and use /dev/fd/NNN as the filename
> which reduces some of the races.
>
> However when just plain executing a shell script we pass the filename of
> the shell script as a command line argument, and expect the shell to
> open the filename again. This has been a time of check to time of use
> race for decades, and one of the reasons we don't have setuid shell
> scripts.
>
> Yet the IMA implementation (without the above mentioned patch) assumes
> the final creds will be calculated before security_bprm_check is called,
> and security_bprm_creds_for_exec busily calculate the final creds.
>
> For some of the security modules I believe anyone can set any label they
> want on a file and they remain secure (At which point I don't understand
> the point of having labels on files). I don't believe that is the case
> for selinux, or in general.
>
> So just to remove the TOCTOU race the security_bprm_creds_for_exec
> and security_bprm_check hooks need to be removed, after moving their
> code into something like security_bprm_creds_from_file.
>
> Or am I missing something and even with the TOCTOU race are setuid shell
> scripts somehow safe now?
setuid shell scripts are not safe. But SELinux (and likely AppArmor
and others) have long relied on the ability to transition on shell
scripts to _shed_ permissions. That's a matter of writing your policy
sensibly.
Changing it would break existing userspace and policies.
^ permalink raw reply
* Re: [PATCH v3 1/4] tpm2-sessions: fix out of range indexing in name_size
From: Jonathan McDowell @ 2025-12-04 15:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, open list, stable,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel,
open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <20251203221215.536031-2-jarkko@kernel.org>
On Thu, Dec 04, 2025 at 12:12:11AM +0200, Jarkko Sakkinen wrote:
>'name_size' does not have any range checks, and it just directly indexes
>with TPM_ALG_ID, which could lead into memory corruption at worst.
>
>Address the issue by only processing known values and returning -EINVAL for
>unrecognized values.
>
>Make also 'tpm_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible so
>that errors are detected before causing any spurious TPM traffic.
>
>End also the authorization session on failure in both of the functions, as
>the session state would be then by definition corrupted.
>
>Cc: stable@vger.kernel.org # v6.10+
>Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
>Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
A minor whitespace query below, but:
Reviewed-by: Jonathan McDowell <noodles@meta.com>
>v3:
>- Fix pr_warn() format string in name_size().
>- Fix !CONFIG_TPM2_HMAC compilation.
>v2:
>- Wrote a better short summary.
>- Addressed remarks in https://lore.kernel.org/linux-integrity/aS8TIeviaippVAha@earth.li/
>- Use -EIO consistently in tpm2_fill_hmac_session. These are not input value
> errors. They could only spun from malformed (kernel) state.
>- name_size did not have a proper default-case. Reorganize the
> fallback into that.
>---
> drivers/char/tpm/tpm2-cmd.c | 23 +++-
> drivers/char/tpm/tpm2-sessions.c | 133 +++++++++++++++-------
> include/linux/tpm.h | 13 ++-
> security/keys/trusted-keys/trusted_tpm2.c | 29 ++++-
> 4 files changed, 143 insertions(+), 55 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>index dd502322f499..be4a9c7f2e1a 100644
>--- a/drivers/char/tpm/tpm2-cmd.c
>+++ b/drivers/char/tpm/tpm2-cmd.c
>@@ -199,7 +199,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> }
>
> if (!disable_pcr_integrity) {
>- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
>+ if (rc) {
>+ tpm_buf_destroy(&buf);
>+ return rc;
>+ }
> tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> } else {
> tpm_buf_append_handle(chip, &buf, pcr_idx);
>@@ -214,8 +218,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> chip->allocated_banks[i].digest_size);
> }
>
>- if (!disable_pcr_integrity)
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ if (!disable_pcr_integrity) {
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc) {
>+ tpm_buf_destroy(&buf);
>+ return rc;
>+ }
>+ }
>+
> rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
> if (!disable_pcr_integrity)
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>@@ -273,7 +283,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
> | TPM2_SA_CONTINUE_SESSION,
> NULL, 0);
> tpm_buf_append_u16(&buf, num_bytes);
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ err = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (err) {
>+ tpm_buf_destroy(&buf);
>+ return err;
>+ }
>+
> err = tpm_transmit_cmd(chip, &buf,
> offsetof(struct tpm2_get_random_out,
> buffer),
>diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
>index 6d03c224e6b2..a265e9752a5e 100644
>--- a/drivers/char/tpm/tpm2-sessions.c
>+++ b/drivers/char/tpm/tpm2-sessions.c
>@@ -144,16 +144,23 @@ struct tpm2_auth {
> /*
> * Name Size based on TPM algorithm (assumes no hash bigger than 255)
> */
>-static u8 name_size(const u8 *name)
>+static int name_size(const u8 *name)
> {
>- static u8 size_map[] = {
>- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
>- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
>- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
>- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
>- };
>- u16 alg = get_unaligned_be16(name);
>- return size_map[alg] + 2;
>+ u16 hash_alg = get_unaligned_be16(name);
>+
>+ switch (hash_alg) {
>+ case TPM_ALG_SHA1:
>+ return SHA1_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA256:
>+ return SHA256_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA384:
>+ return SHA384_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA512:
>+ return SHA512_DIGEST_SIZE + 2;
>+ default:
>+ pr_warn("tpm: unsupported name algorithm: 0x%04x\n", hash_alg);
>+ return -EINVAL;
>+ }
> }
>
> static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
>@@ -161,6 +168,7 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
> struct tpm_header *head = (struct tpm_header *)buf->data;
> off_t offset = TPM_HEADER_SIZE;
> u32 tot_len = be32_to_cpu(head->length);
>+ int ret;
> u32 val;
>
> /* we're starting after the header so adjust the length */
>@@ -172,9 +180,15 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
> return -EINVAL;
> offset += val;
> /* name */
>+
Spurious extra blank line? Or meant to be before the comment?
> val = tpm_buf_read_u16(buf, &offset);
>- if (val != name_size(&buf->data[offset]))
>+ ret = name_size(&buf->data[offset]);
>+ if (ret < 0)
>+ return ret;
>+
>+ if (val != ret)
> return -EINVAL;
>+
> memcpy(name, &buf->data[offset], val);
> /* forget the rest */
> return 0;
>@@ -221,46 +235,72 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
> * As with most tpm_buf operations, success is assumed because failure
> * will be caused by an incorrect programming model and indicated by a
> * kernel message.
>+ *
>+ * Ends the authorization session on failure.
> */
>-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>- u32 handle, u8 *name)
>+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>+ u32 handle, u8 *name)
> {
> #ifdef CONFIG_TCG_TPM2_HMAC
> enum tpm2_mso_type mso = tpm2_handle_mso(handle);
> struct tpm2_auth *auth;
> int slot;
>+ int ret;
> #endif
>
> if (!tpm2_chip_auth(chip)) {
> tpm_buf_append_handle(chip, buf, handle);
>- return;
>+ return 0;
> }
>
> #ifdef CONFIG_TCG_TPM2_HMAC
> slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
> if (slot >= AUTH_MAX_NAMES) {
>- dev_err(&chip->dev, "TPM: too many handles\n");
>- return;
>+ dev_err(&chip->dev, "too many handles\n");
>+ ret = -EIO;
>+ goto err;
> }
> auth = chip->auth;
>- WARN(auth->session != tpm_buf_length(buf),
>- "name added in wrong place\n");
>+ if (auth->session != tpm_buf_length(buf)) {
>+ dev_err(&chip->dev, "session state malformed");
>+ ret = -EIO;
>+ goto err;
>+ }
> tpm_buf_append_u32(buf, handle);
> auth->session += 4;
>
> if (mso == TPM2_MSO_PERSISTENT ||
> mso == TPM2_MSO_VOLATILE ||
> mso == TPM2_MSO_NVRAM) {
>- if (!name)
>- tpm2_read_public(chip, handle, auth->name[slot]);
>+ if (!name) {
>+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
>+ if (ret)
>+ goto err;
>+ }
> } else {
>- if (name)
>- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
>+ if (name) {
>+ dev_err(&chip->dev, "handle 0x%08x does not use a name\n",
>+ handle);
>+ ret = -EIO;
>+ goto err;
>+ }
> }
>
> auth->name_h[slot] = handle;
>- if (name)
>- memcpy(auth->name[slot], name, name_size(name));
>+ if (name) {
>+ ret = name_size(name);
>+ if (ret < 0)
>+ goto err;
>+
>+ memcpy(auth->name[slot], name, ret);
>+ }
>+#endif
>+ return 0;
>+
>+#ifdef CONFIG_TCG_TPM2_HMAC
>+err:
>+ tpm2_end_auth_session(chip);
>+ return tpm_ret_to_err(ret);
> #endif
> }
> EXPORT_SYMBOL_GPL(tpm_buf_append_name);
>@@ -533,11 +573,9 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> * encryption key and encrypts the first parameter of the command
> * buffer with it.
> *
>- * As with most tpm_buf operations, success is assumed because failure
>- * will be caused by an incorrect programming model and indicated by a
>- * kernel message.
>+ * Ends the authorization session on failure.
> */
>-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
>+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> {
> u32 cc, handles, val;
> struct tpm2_auth *auth = chip->auth;
>@@ -549,9 +587,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> u8 cphash[SHA256_DIGEST_SIZE];
> struct sha256_ctx sctx;
> struct hmac_sha256_ctx hctx;
>+ int ret;
>
>- if (!auth)
>- return;
>+ if (!auth) {
>+ ret = -EIO;
>+ goto err;
>+ }
>
> /* save the command code in BE format */
> auth->ordinal = head->ordinal;
>@@ -560,9 +601,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
>
> i = tpm2_find_cc(chip, cc);
> if (i < 0) {
>- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
>- return;
>+ dev_err(&chip->dev, "command 0x%08x not found\n", cc);
>+ ret = -EIO;
>+ goto err;
> }
>+
> attrs = chip->cc_attrs_tbl[i];
>
> handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
>@@ -576,9 +619,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> u32 handle = tpm_buf_read_u32(buf, &offset_s);
>
> if (auth->name_h[i] != handle) {
>- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
>- i);
>- return;
>+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
>+ ret = -EIO;
>+ goto err;
> }
> }
> /* point offset_s to the start of the sessions */
>@@ -609,12 +652,14 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> offset_s += len;
> }
> if (offset_s != offset_p) {
>- dev_err(&chip->dev, "TPM session length is incorrect\n");
>- return;
>+ dev_err(&chip->dev, "session length is incorrect\n");
>+ ret = -EIO;
>+ goto err;
> }
> if (!hmac) {
>- dev_err(&chip->dev, "TPM could not find HMAC session\n");
>- return;
>+ dev_err(&chip->dev, "could not find HMAC session\n");
>+ ret = -EIO;
>+ goto err;
> }
>
> /* encrypt before HMAC */
>@@ -646,8 +691,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> if (mso == TPM2_MSO_PERSISTENT ||
> mso == TPM2_MSO_VOLATILE ||
> mso == TPM2_MSO_NVRAM) {
>- sha256_update(&sctx, auth->name[i],
>- name_size(auth->name[i]));
>+ ret = name_size(auth->name[i]);
>+ if (ret < 0)
>+ goto err;
>+
>+ sha256_update(&sctx, auth->name[i], ret);
> } else {
> __be32 h = cpu_to_be32(auth->name_h[i]);
>
>@@ -668,6 +716,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
> hmac_sha256_update(&hctx, &auth->attrs, 1);
> hmac_sha256_final(&hctx, hmac);
>+ return 0;
>+
>+err:
>+ tpm2_end_auth_session(chip);
>+ return ret;
> }
> EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
>
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index 3d8f7d1ce2b8..aa816b144ab3 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -529,8 +529,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
> #endif
> }
>
>-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>- u32 handle, u8 *name);
>+int tpm_buf_append_name(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);
>@@ -563,7 +563,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> #ifdef CONFIG_TCG_TPM2_HMAC
>
> int tpm2_start_auth_session(struct tpm_chip *chip);
>-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
>+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
> int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
> int rc);
> void tpm2_end_auth_session(struct tpm_chip *chip);
>@@ -577,10 +577,13 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
> static inline void tpm2_end_auth_session(struct tpm_chip *chip)
> {
> }
>-static inline void tpm_buf_fill_hmac_session(struct tpm_chip *chip,
>- struct tpm_buf *buf)
>+
>+static inline int tpm_buf_fill_hmac_session(struct tpm_chip *chip,
>+ struct tpm_buf *buf)
> {
>+ return 0;
> }
>+
> static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
> struct tpm_buf *buf,
> int rc)
>diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>index 8bc6efa8accb..5b205279584b 100644
>--- a/security/keys/trusted-keys/trusted_tpm2.c
>+++ b/security/keys/trusted-keys/trusted_tpm2.c
>@@ -268,7 +268,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out_put;
> }
>
>- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>+
> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
> options->keyauth, TPM_DIGEST_SIZE);
>
>@@ -316,7 +319,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out;
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> if (rc)
>@@ -427,7 +433,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> return rc;
> }
>
>- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>+
> tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
> TPM_DIGEST_SIZE);
>
>@@ -439,7 +448,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> goto out;
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> if (!rc)
>@@ -484,7 +496,9 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> return rc;
> }
>
>- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>
> if (!options->policyhandle) {
> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
>@@ -509,7 +523,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> NULL, 0);
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>
>--
>2.52.0
>
J.
--
/-\ | Every program is either trivial or
|@/ Debian GNU/Linux Developer | it contains at least one bug.
\- |
^ permalink raw reply
* Re: Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: Bernd Edlinger @ 2025-12-04 13:03 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, Roberto Sassu, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <20251204054915.GI1712166@ZenIV>
On 12/4/25 06:49, Al Viro wrote:
> On Wed, Dec 03, 2025 at 02:16:29PM +0100, Bernd Edlinger wrote:
>
>> Hmm, yes, that looks like an issue.
>>
>> I would have expected the security engine to look at bprm->filenanme
>> especially in the case, when bprm->interp != bprm->filename,
>> and check that it is not a sym-link with write-access for the
>> current user and of course also that the bprm->file is not a regular file
>> which is writable by the current user, if that is the case I would have expected
>> the secuity engine to enforce non-new-privs on a SUID executable somehow.
>
> Check that _what_ is not a symlink? And while we are at it, what do write
> permissions to any symlinks have to do with anything whatsoever?
When we execve a normal executable, we do open the binary file with deny_write_access
so this might allow the security engine to inspaect the binary, before it is used.
However this behavior has changed recently, now it has some exceptions, where even
this behavior is no longer guaranteed for binary executables, due to
commit 0357ef03c94ef835bd44a0658b8edb672a9dbf51, but why? I have no idea...
But with shell scripts an attack is possible, where a sym-link is executed,
and the SUID bit of the target file is used but a race condition might allow
the attacker to replace the script that is used by the shell:
Consider this:
ln -s /usr/bin/legitimate-suid-sctipt.sh
where legitimate-suid-sctipt.sh starts with "#! /bin/bash -"
and the attack works this way:
./legitmate-suid-script.sh &
ln -f -s do-what-i-want.sh legitimate-suid-script.sh
Bernd.
^ permalink raw reply
* [PATCH] bpf, arm64: Do not audit capability check in do_jit()
From: Ondrej Mosnacek @ 2025-12-04 12:59 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, selinux, linux-security-module, Serge E . Hallyn
Analogically to the x86 commit 881a9c9cb785 ("bpf: Do not audit
capability check in do_jit()"), change the capable() call to
ns_capable_noaudit() in order to avoid spurious SELinux denials in audit
log.
The commit log from that commit applies here as well:
"""
The failure of this check only results in a security mitigation being
applied, slightly affecting performance of the compiled BPF program. It
doesn't result in a failed syscall, an thus auditing a failed LSM
permission check for it is unwanted. For example with SELinux, it causes
a denial to be reported for confined processes running as root, which
tends to be flagged as a problem to be fixed in the policy. Yet
dontauditing or allowing CAP_SYS_ADMIN to the domain may not be
desirable, as it would allow/silence also other checks - either going
against the principle of least privilege or making debugging potentially
harder.
Fix it by changing it from capable() to ns_capable_noaudit(), which
instructs the LSMs to not audit the resulting denials.
"""
Fixes: f300769ead03 ("arm64: bpf: Only mitigate cBPF programs loaded by unprivileged users")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
arch/arm64/net/bpf_jit_comp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index afd05b41ea9e6..5823f2df204d9 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1004,7 +1004,7 @@ static void __maybe_unused build_bhb_mitigation(struct jit_ctx *ctx)
arm64_get_spectre_v2_state() == SPECTRE_VULNERABLE)
return;
- if (capable(CAP_SYS_ADMIN))
+ if (ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN))
return;
if (supports_clearbhb(SCOPE_SYSTEM)) {
--
2.52.0
^ permalink raw reply related
* Re: set_security_override_from_ctx()
From: David Howells @ 2025-12-04 11:39 UTC (permalink / raw)
To: Paul Moore; +Cc: dhowells, Casey Schaufler, LSM List, SElinux list
In-Reply-To: <CAHC9VhQOW8a_pTKS+Mhtu2LEFCPRhEzuhLsJOFHNTNUNUELChg@mail.gmail.com>
Paul Moore <paul@paul-moore.com> wrote:
> I would suggest sending a patch to remove
> set_security_override_from_ctx() since there are no longer any
> callers. Send it to the LSM list and I'll merge it once the merge
> window closes.
Fine by me.
David
^ permalink raw reply
* Re: (subset) [PATCH 00/46] Allow inlining C helpers into Rust when using LTO
From: Christian Brauner @ 2025-12-04 10:05 UTC (permalink / raw)
To: Alice Ryhl
Cc: Christian Brauner, linux-kernel, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Peter Zijlstra, Boqun Feng,
Elle Rhumsaa, Carlos Llamas, Yury Norov, Andreas Hindborg,
linux-block, FUJITA Tomonori, Miguel Ojeda, Michael Turquette,
Stephen Boyd, linux-clk, Benno Lossin, Danilo Krummrich,
Thomas Gleixner, Rafael J. Wysocki, Viresh Kumar, linux-pm,
Paul Moore, Serge Hallyn, linux-security-module, Daniel Almeida,
Abdiel Janulgue, Robin Murphy, Lyude Paul, Alexander Viro,
Jan Kara, linux-fsdevel, Josh Poimboeuf, Jason Baron,
Steven Rostedt, Ard Biesheuvel, Brendan Higgins, David Gow,
linux-kselftest, Andrew Morton, Liam R. Howlett, Andrew Ballance,
maple-tree, linux-mm, Lorenzo Stoakes, Uladzislau Rezki,
Vitaly Wool, Rob Herring, Saravana Kannan, devicetree,
Bjorn Helgaas, Krzysztof Wilczyński, linux-pci,
Remo Senekowitsch, Paul E. McKenney, rcu, Will Deacon,
Fiona Behrens, Gary Guo, Liam Girdwood, Mark Brown,
Alexandre Courbot, Vlastimil Babka, Christoph Lameter,
David Rientjes, Ingo Molnar, Waiman Long, Mitchell Levy,
Frederic Weisbecker, Anna-Maria Behnsen, John Stultz, linux-usb,
Tejun Heo, Lai Jiangshan, Matthew Wilcox, Tamir Duberstein,
Rae Moar, rust-for-linux
In-Reply-To: <20251202-define-rust-helper-v1-0-a2e13cbc17a6@google.com>
On Tue, 02 Dec 2025 19:37:24 +0000, Alice Ryhl wrote:
> This patch series adds __rust_helper to every single rust helper. The
> patches do not depend on each other, so maintainers please go ahead and
> pick up any patches relevant to your subsystem! Or provide your Acked-by
> so that Miguel can pick them up.
>
> These changes were generated by adding __rust_helper and running
> ClangFormat. Unrelated formatting changes were removed manually.
>
> [...]
Applied to the vfs-6.20.rust branch of the vfs/vfs.git tree.
Patches in the vfs-6.20.rust branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.20.rust
[18/46] rust: fs: add __rust_helper to helpers
https://git.kernel.org/vfs/vfs/c/02c444cc60e5
[27/46] rust: pid_namespace: add __rust_helper to helpers
https://git.kernel.org/vfs/vfs/c/f28a178408e4
[29/46] rust: poll: add __rust_helper to helpers
https://git.kernel.org/vfs/vfs/c/de98ed59d678
^ permalink raw reply
* Re: Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: David Laight @ 2025-12-04 9:32 UTC (permalink / raw)
To: Al Viro
Cc: Bernd Edlinger, Eric W. Biederman, Roberto Sassu, Alexey Dobriyan,
Oleg Nesterov, Kees Cook, Andy Lutomirski, Will Drewry,
Christian Brauner, Andrew Morton, Michal Hocko, Serge Hallyn,
James Morris, Randy Dunlap, Suren Baghdasaryan, Yafang Shao,
Helge Deller, Adrian Reber, Thomas Gleixner, Jens Axboe,
Alexei Starovoitov, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest, linux-mm,
linux-security-module, tiozhang, Luis Chamberlain,
Paulo Alcantara (SUSE), Sergey Senozhatsky, Frederic Weisbecker,
YueHaibing, Paul Moore, Aleksa Sarai, Stefan Roesch, Chao Yu,
xu xin, Jeff Layton, Jan Kara, David Hildenbrand, Dave Chinner,
Shuah Khan, Elena Reshetova, David Windsor, Mateusz Guzik,
Ard Biesheuvel, Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <20251204054915.GI1712166@ZenIV>
On Thu, 4 Dec 2025 05:49:15 +0000
Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Dec 03, 2025 at 02:16:29PM +0100, Bernd Edlinger wrote:
>
> > Hmm, yes, that looks like an issue.
> >
> > I would have expected the security engine to look at bprm->filenanme
> > especially in the case, when bprm->interp != bprm->filename,
> > and check that it is not a sym-link with write-access for the
> > current user and of course also that the bprm->file is not a regular file
> > which is writable by the current user, if that is the case I would have expected
> > the secuity engine to enforce non-new-privs on a SUID executable somehow.
>
> Check that _what_ is not a symlink? And while we are at it, what do write
> permissions to any symlinks have to do with anything whatsoever?
>
You'd need to check for write permissions to all the directories in the
full path of the symlink and in all the directories traversed by the symlink.
(and that may not be enough....)
Passing the shell (or whatever) /dev/fd/n doesn't seem (to me) any different
from what happens when the elf interpreter runs a suid program.
You might want to check for non-owner write permissions to the /dev/fd/n entry,
but that is true for any suid executable, not just scripts.
FWIW the SYSV shells normally set the effective uid back the real uid.
So making a script suid didn't work unless the script started "#!/bin/sh -p".
Whether that improved security (rather than being annoying) is another matter.
David
^ permalink raw reply
* Re: Are setuid shell scripts safe? (Implied by security_bprm_creds_for_exec)
From: Al Viro @ 2025-12-04 5:49 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Eric W. Biederman, Roberto Sassu, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, zohar, linux-integrity, Ryan Lee, apparmor
In-Reply-To: <GV2PPF74270EBEE90CDCD964F69E806EF58E4D9A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
On Wed, Dec 03, 2025 at 02:16:29PM +0100, Bernd Edlinger wrote:
> Hmm, yes, that looks like an issue.
>
> I would have expected the security engine to look at bprm->filenanme
> especially in the case, when bprm->interp != bprm->filename,
> and check that it is not a sym-link with write-access for the
> current user and of course also that the bprm->file is not a regular file
> which is writable by the current user, if that is the case I would have expected
> the secuity engine to enforce non-new-privs on a SUID executable somehow.
Check that _what_ is not a symlink? And while we are at it, what do write
permissions to any symlinks have to do with anything whatsoever?
^ permalink raw reply
* Re: set_security_override_from_ctx()
From: Paul Moore @ 2025-12-03 23:32 UTC (permalink / raw)
To: Casey Schaufler, LSM List; +Cc: David Howells, SElinux list
In-Reply-To: <866a132a-b6e2-4e40-aba3-d8b733184672@schaufler-ca.com>
On Wed, Dec 3, 2025 at 5:03 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Adding David, who wrote the code ...
>
> On 12/3/2025 1:32 PM, Casey Schaufler wrote:
> > While trying to ensure sanity in security context processing I
> > discovered set_security_override_from_ctx(), which is not used anywhere
> > in the upstream kernel. Does anyone here know what its purpose is? I
> > would very much like to remove it, but of course wouldn't want to break
> > anything important.
It looks like set_security_override_from_ctx() was first introduced
back in v2.6.29, but I didn't see an in-tree caller until v2.6.30. I
didn't check every kernel release, but doing some spot checks it looks
like cachefiles remained the only user until it dropped the call in
v6.12 with the following commit:
commit e5a8b6446c0d370716f193771ccacf3260a57534
Author: Max Kellermann <max.kellermann@ionos.com>
Date: Fri Dec 13 13:50:05 2024 +0000
cachefiles: Parse the "secctx" immediately
Instead of storing an opaque string, call security_secctx_to_secid()
right in the "secctx" command handler and store only the numeric
"secid". This eliminates an unnecessary string allocation and allows
the daemon to receive errors when writing the "secctx" command instead
of postponing the error to the "bind" command handler. For example,
if the kernel was built without `CONFIG_SECURITY`, "bind" will return
`EOPNOTSUPP`, but the daemon doesn't know why. With this patch, the
"secctx" will instead return `EOPNOTSUPP` which is the right context
for this error.
This patch adds a boolean flag `have_secid` because I'm not sure if we
can safely assume that zero is the special secid value for "not set".
This appears to be true for SELinux, Smack and AppArmor, but since
this attribute is not documented, I'm unable to derive a stable
guarantee for that.
Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/r/20241209141554.638708-1-max.kellermann@ionos
.com/
Link: https://lore.kernel.org/r/20241213135013.2964079-6-dhowells@redhat.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
... which basically just drops the security_secctx_to_secid() from the
code path.
I would suggest sending a patch to remove
set_security_override_from_ctx() since there are no longer any
callers. Send it to the LSM list and I'll merge it once the merge
window closes.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] ipe: remove headers that are included but not used
From: Fan Wu @ 2025-12-03 22:25 UTC (permalink / raw)
To: Yicong Hui
Cc: wufan, paul, jmorris, serge, linux-security-module, linux-kernel
In-Reply-To: <20251203193718.504344-1-yiconghui@gmail.com>
On Wed, Dec 3, 2025 at 11:37 AM Yicong Hui <yiconghui@gmail.com> wrote:
>
> Remove headers that are included but not used in audit.c, audit.c,
> policy.c within the IPE module
>
> Change have been tested through kunit, kernel compiles and passes kunit
> tests
>
> Signed-off-by: Yicong Hui <yiconghui@gmail.com>
> ---
> security/ipe/audit.c | 1 -
> security/ipe/policy.c | 1 -
> security/ipe/policy_fs.c | 1 -
> 3 files changed, 3 deletions(-)
...
Hi Yicong,
Thanks for the patch. This kind of cleanup is appreciated.
Commit message typo: "audit. c, audit.c, policy. c" - audit. c is listed
twice.
I was trying to verify whether ipe.h is really not needed and found
that these files are missing explicit dependencies. policy.c and
policy_fs.c use rcu, mutex, and slab functions but rely on transitive
includes. After removing ipe.h, they still compile because eval.h
also happens to provide these dependencies indirectly.
I'm happy to merge a patch removing unused headers like ipe.h, but
would like to see the implicit dependencies resolved as well. Would
you mind tracing the complete dependencies and adding the explicit
includes in v2?
-Fan
^ permalink raw reply
* [PATCH v3 4/4] tpm2-sessions: Open code tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-12-03 22:12 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Jonathan McDowell, Peter Huewe, Jason Gunthorpe,
open list, Jarkko Sakkinen, Jonathan McDowell, James Bottomley,
Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, open list:KEYS-TRUSTED,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20251203221215.536031-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Open code 'tpm_buf_append_hmac_session_opt' to the call site, as it only
masks a call sequence and does otherwise nothing particularly useful.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
---
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
include/linux/tpm.h | 23 -----------------------
security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
3 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ce0a1c6b0596..3a77be7ebf4a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -282,9 +282,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
do {
tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
- | TPM2_SA_CONTINUE_SESSION,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf,
+ TPM2_SA_ENCRYPT |
+ TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
tpm_buf_append_u16(&buf, num_bytes);
err = tpm_buf_fill_hmac_session(chip, &buf);
if (err) {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index afa51723296a..202da079d500 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -536,29 +536,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
u8 *passphrase, int passphraselen);
-static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
- struct tpm_buf *buf,
- u8 attributes,
- u8 *passphrase,
- int passphraselen)
-{
- struct tpm_header *head;
- int offset;
-
- if (tpm2_chip_auth(chip)) {
- tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
- } else {
- offset = buf->handles * 4 + TPM_HEADER_SIZE;
- head = (struct tpm_header *)buf->data;
-
- /*
- * If the only sessions are optional, the command tag must change to
- * TPM2_ST_NO_SESSIONS.
- */
- if (tpm_buf_length(buf) == offset)
- head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
- }
-}
#ifdef CONFIG_TCG_TPM2_HMAC
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 5b205279584b..a7ea4a1c3bed 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -481,8 +481,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
struct trusted_key_options *options,
u32 blob_handle)
{
+ struct tpm_header *head;
struct tpm_buf buf;
u16 data_len;
+ int offset;
u8 *data;
int rc;
@@ -519,8 +521,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
tpm2_buf_append_auth(&buf, options->policyhandle,
NULL /* nonce */, 0, 0,
options->blobauth, options->blobauth_len);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
}
rc = tpm_buf_fill_hmac_session(chip, &buf);
--
2.52.0
^ permalink raw reply related
* [PATCH v3 1/4] tpm2-sessions: fix out of range indexing in name_size
From: Jarkko Sakkinen @ 2025-12-03 22:12 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, Jonathan McDowell, Peter Huewe, Jason Gunthorpe,
open list, stable, James Bottomley, Mimi Zohar, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, Ard Biesheuvel,
open list:KEYS-TRUSTED, open list:SECURITY SUBSYSTEM
In-Reply-To: <20251203221215.536031-1-jarkko@kernel.org>
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID, which could lead into memory corruption at worst.
Address the issue by only processing known values and returning -EINVAL for
unrecognized values.
Make also 'tpm_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible so
that errors are detected before causing any spurious TPM traffic.
End also the authorization session on failure in both of the functions, as
the session state would be then by definition corrupted.
Cc: stable@vger.kernel.org # v6.10+
Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
v3:
- Fix pr_warn() format string in name_size().
- Fix !CONFIG_TPM2_HMAC compilation.
v2:
- Wrote a better short summary.
- Addressed remarks in https://lore.kernel.org/linux-integrity/aS8TIeviaippVAha@earth.li/
- Use -EIO consistently in tpm2_fill_hmac_session. These are not input value
errors. They could only spun from malformed (kernel) state.
- name_size did not have a proper default-case. Reorganize the
fallback into that.
---
drivers/char/tpm/tpm2-cmd.c | 23 +++-
drivers/char/tpm/tpm2-sessions.c | 133 +++++++++++++++-------
include/linux/tpm.h | 13 ++-
security/keys/trusted-keys/trusted_tpm2.c | 29 ++++-
4 files changed, 143 insertions(+), 55 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index dd502322f499..be4a9c7f2e1a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -199,7 +199,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
if (!disable_pcr_integrity) {
- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ return rc;
+ }
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -214,8 +218,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
chip->allocated_banks[i].digest_size);
}
- if (!disable_pcr_integrity)
- tpm_buf_fill_hmac_session(chip, &buf);
+ if (!disable_pcr_integrity) {
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ return rc;
+ }
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
if (!disable_pcr_integrity)
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
@@ -273,7 +283,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
| TPM2_SA_CONTINUE_SESSION,
NULL, 0);
tpm_buf_append_u16(&buf, num_bytes);
- tpm_buf_fill_hmac_session(chip, &buf);
+ err = tpm_buf_fill_hmac_session(chip, &buf);
+ if (err) {
+ tpm_buf_destroy(&buf);
+ return err;
+ }
+
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..a265e9752a5e 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -144,16 +144,23 @@ struct tpm2_auth {
/*
* Name Size based on TPM algorithm (assumes no hash bigger than 255)
*/
-static u8 name_size(const u8 *name)
+static int name_size(const u8 *name)
{
- static u8 size_map[] = {
- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
- };
- u16 alg = get_unaligned_be16(name);
- return size_map[alg] + 2;
+ u16 hash_alg = get_unaligned_be16(name);
+
+ switch (hash_alg) {
+ case TPM_ALG_SHA1:
+ return SHA1_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA256:
+ return SHA256_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA384:
+ return SHA384_DIGEST_SIZE + 2;
+ case TPM_ALG_SHA512:
+ return SHA512_DIGEST_SIZE + 2;
+ default:
+ pr_warn("tpm: unsupported name algorithm: 0x%04x\n", hash_alg);
+ return -EINVAL;
+ }
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +168,7 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
struct tpm_header *head = (struct tpm_header *)buf->data;
off_t offset = TPM_HEADER_SIZE;
u32 tot_len = be32_to_cpu(head->length);
+ int ret;
u32 val;
/* we're starting after the header so adjust the length */
@@ -172,9 +180,15 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
return -EINVAL;
offset += val;
/* name */
+
val = tpm_buf_read_u16(buf, &offset);
- if (val != name_size(&buf->data[offset]))
+ ret = name_size(&buf->data[offset]);
+ if (ret < 0)
+ return ret;
+
+ if (val != ret)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -221,46 +235,72 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* As with most tpm_buf operations, success is assumed because failure
* will be caused by an incorrect programming model and indicated by a
* kernel message.
+ *
+ * Ends the authorization session on failure.
*/
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name)
+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+ u32 handle, u8 *name)
{
#ifdef CONFIG_TCG_TPM2_HMAC
enum tpm2_mso_type mso = tpm2_handle_mso(handle);
struct tpm2_auth *auth;
int slot;
+ int ret;
#endif
if (!tpm2_chip_auth(chip)) {
tpm_buf_append_handle(chip, buf, handle);
- return;
+ return 0;
}
#ifdef CONFIG_TCG_TPM2_HMAC
slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
if (slot >= AUTH_MAX_NAMES) {
- dev_err(&chip->dev, "TPM: too many handles\n");
- return;
+ dev_err(&chip->dev, "too many handles\n");
+ ret = -EIO;
+ goto err;
}
auth = chip->auth;
- WARN(auth->session != tpm_buf_length(buf),
- "name added in wrong place\n");
+ if (auth->session != tpm_buf_length(buf)) {
+ dev_err(&chip->dev, "session state malformed");
+ ret = -EIO;
+ goto err;
+ }
tpm_buf_append_u32(buf, handle);
auth->session += 4;
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- if (!name)
- tpm2_read_public(chip, handle, auth->name[slot]);
+ if (!name) {
+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
+ if (ret)
+ goto err;
+ }
} else {
- if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ if (name) {
+ dev_err(&chip->dev, "handle 0x%08x does not use a name\n",
+ handle);
+ ret = -EIO;
+ goto err;
+ }
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ ret = name_size(name);
+ if (ret < 0)
+ goto err;
+
+ memcpy(auth->name[slot], name, ret);
+ }
+#endif
+ return 0;
+
+#ifdef CONFIG_TCG_TPM2_HMAC
+err:
+ tpm2_end_auth_session(chip);
+ return tpm_ret_to_err(ret);
#endif
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -533,11 +573,9 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* encryption key and encrypts the first parameter of the command
* buffer with it.
*
- * As with most tpm_buf operations, success is assumed because failure
- * will be caused by an incorrect programming model and indicated by a
- * kernel message.
+ * Ends the authorization session on failure.
*/
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
{
u32 cc, handles, val;
struct tpm2_auth *auth = chip->auth;
@@ -549,9 +587,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u8 cphash[SHA256_DIGEST_SIZE];
struct sha256_ctx sctx;
struct hmac_sha256_ctx hctx;
+ int ret;
- if (!auth)
- return;
+ if (!auth) {
+ ret = -EIO;
+ goto err;
+ }
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -560,9 +601,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
i = tpm2_find_cc(chip, cc);
if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
+ dev_err(&chip->dev, "command 0x%08x not found\n", cc);
+ ret = -EIO;
+ goto err;
}
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +619,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
u32 handle = tpm_buf_read_u32(buf, &offset_s);
if (auth->name_h[i] != handle) {
- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
- i);
- return;
+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
+ ret = -EIO;
+ goto err;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +652,14 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
offset_s += len;
}
if (offset_s != offset_p) {
- dev_err(&chip->dev, "TPM session length is incorrect\n");
- return;
+ dev_err(&chip->dev, "session length is incorrect\n");
+ ret = -EIO;
+ goto err;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ ret = -EIO;
+ goto err;
}
/* encrypt before HMAC */
@@ -646,8 +691,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
if (mso == TPM2_MSO_PERSISTENT ||
mso == TPM2_MSO_VOLATILE ||
mso == TPM2_MSO_NVRAM) {
- sha256_update(&sctx, auth->name[i],
- name_size(auth->name[i]));
+ ret = name_size(auth->name[i]);
+ if (ret < 0)
+ goto err;
+
+ sha256_update(&sctx, auth->name[i], ret);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +716,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
hmac_sha256_update(&hctx, &auth->attrs, 1);
hmac_sha256_final(&hctx, hmac);
+ return 0;
+
+err:
+ tpm2_end_auth_session(chip);
+ return ret;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 3d8f7d1ce2b8..aa816b144ab3 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -529,8 +529,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
#endif
}
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
- u32 handle, u8 *name);
+int tpm_buf_append_name(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);
@@ -563,7 +563,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
#ifdef CONFIG_TCG_TPM2_HMAC
int tpm2_start_auth_session(struct tpm_chip *chip);
-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
int rc);
void tpm2_end_auth_session(struct tpm_chip *chip);
@@ -577,10 +577,13 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
static inline void tpm2_end_auth_session(struct tpm_chip *chip)
{
}
-static inline void tpm_buf_fill_hmac_session(struct tpm_chip *chip,
- struct tpm_buf *buf)
+
+static inline int tpm_buf_fill_hmac_session(struct tpm_chip *chip,
+ struct tpm_buf *buf)
{
+ return 0;
}
+
static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
struct tpm_buf *buf,
int rc)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8bc6efa8accb..5b205279584b 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -268,7 +268,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out_put;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -316,7 +319,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -427,7 +433,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -439,7 +448,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
goto out;
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (!rc)
@@ -484,7 +496,9 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
return rc;
}
- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
+ if (rc)
+ goto out;
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -509,7 +523,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
NULL, 0);
}
- tpm_buf_fill_hmac_session(chip, &buf);
+ rc = tpm_buf_fill_hmac_session(chip, &buf);
+ if (rc)
+ goto out;
+
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
--
2.52.0
^ permalink raw reply related
* Re: set_security_override_from_ctx()
From: Casey Schaufler @ 2025-12-03 22:02 UTC (permalink / raw)
To: LSM List, David Howells; +Cc: SElinux list
In-Reply-To: <cb4293da-41dc-4586-adca-2859944905dc@schaufler-ca.com>
Adding David, who wrote the code ...
On 12/3/2025 1:32 PM, Casey Schaufler wrote:
> While trying to ensure sanity in security context processing I
> discovered set_security_override_from_ctx(), which is not used anywhere
> in the upstream kernel. Does anyone here know what its purpose is? I
> would very much like to remove it, but of course wouldn't want to break
> anything important.
>
>
^ permalink raw reply
* set_security_override_from_ctx()
From: Casey Schaufler @ 2025-12-03 21:32 UTC (permalink / raw)
To: LSM List; +Cc: SElinux list, Casey Schaufler
In-Reply-To: <cb4293da-41dc-4586-adca-2859944905dc.ref@schaufler-ca.com>
While trying to ensure sanity in security context processing I
discovered set_security_override_from_ctx(), which is not used anywhere
in the upstream kernel. Does anyone here know what its purpose is? I
would very much like to remove it, but of course wouldn't want to break
anything important.
^ permalink raw reply
* Re: [PATCH v3 3/5] samples/landlock: Add LANDLOCK_ADD_RULE_NO_INHERIT to landlock-sandboxer
From: Tingmao Wang @ 2025-12-03 21:13 UTC (permalink / raw)
To: Justin Suess
Cc: linux-security-module, Mickaël Salaün,
Günther Noack, Jan Kara, Abhinav Saxena
In-Reply-To: <20251126122039.3832162-4-utilityemal77@gmail.com>
On 11/26/25 12:20, Justin Suess wrote:
> Adds support to landlock-sandboxer with environment variables LL_FS_RO_NO_INHERIT
> and LL_FS_RW_NO_INHERIT. These create the same rulesets as their non-no inherit variants,
> plus the LANDLOCK_ADD_RULE_NO_INHERIT flag.
>
> v2..v3 changes:
>
> * Minor formatting fixes
>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> ---
> samples/landlock/sandboxer.c | 37 +++++++++++++++++++++++++++---------
> 1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index 2d8e3e94b77b..6f6bfc4e5110 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -58,6 +58,8 @@ static inline int landlock_restrict_self(const int ruleset_fd,
>
> #define ENV_FS_RO_NAME "LL_FS_RO"
> #define ENV_FS_RW_NAME "LL_FS_RW"
> +#define ENV_FS_RO_NO_INHERIT_NAME "LL_FS_RO_NO_INHERIT"
> +#define ENV_FS_RW_NO_INHERIT_NAME "LL_FS_RW_NO_INHERIT"
> #define ENV_FS_QUIET_NAME "LL_FS_QUIET"
> #define ENV_FS_QUIET_ACCESS_NAME "LL_FS_QUIET_ACCESS"
> #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> @@ -121,7 +123,8 @@ static int parse_path(char *env_path, const char ***const path_list)
> /* clang-format on */
>
> static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
> - const __u64 allowed_access, bool quiet)
> + const __u64 allowed_access,
> + __u32 add_rule_flags, bool mandatory)
> {
> int num_paths, i, ret = 1;
> char *env_path_name;
> @@ -132,9 +135,13 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
>
> env_path_name = getenv(env_var);
> if (!env_path_name) {
> - /* Prevents users to forget a setting. */
> - fprintf(stderr, "Missing environment variable %s\n", env_var);
> - return 1;
> + if (mandatory) {
> + /* Prevents from forgetting to set necessary env vars. */
> + fprintf(stderr, "Missing environment variable %s\n",
> + env_var);
> + return 1;
> + }
> + return 0;
> }
> env_path_name = strdup(env_path_name);
> unsetenv(env_var);
> @@ -171,8 +178,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
> if (!S_ISDIR(statbuf.st_mode))
> path_beneath.allowed_access &= ACCESS_FILE;
> if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> - &path_beneath,
> - quiet ? LANDLOCK_ADD_RULE_QUIET : 0)) {
> + &path_beneath, add_rule_flags)) {
> fprintf(stderr,
> "Failed to update the ruleset with \"%s\": %s\n",
> path_list[i], strerror(errno));
> @@ -375,6 +381,8 @@ static const char help[] =
> "Optional settings (when not set, their associated access check "
> "is always allowed, which is different from an empty string which "
> "means an empty list):\n"
> + "* " ENV_FS_RO_NO_INHERIT_NAME ": read-only paths without rule inheritance\n"
> + "* " ENV_FS_RW_NO_INHERIT_NAME ": read-write paths without rule inheritance\n"
Would it make more sense to just have one LL_FS_NO_INHERIT env rule, that
will attach the "no inherit" flag without necessarily adding any access?
> "* " ENV_TCP_BIND_NAME ": ports allowed to bind (server)\n"
> "* " ENV_TCP_CONNECT_NAME ": ports allowed to connect (client)\n"
> "* " ENV_SCOPED_NAME ": actions denied on the outside of the landlock domain\n"
> @@ -596,17 +604,28 @@ int main(const int argc, char *const argv[], char *const *const envp)
> }
>
> if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro,
> - false)) {
> + 0, true)) {
> goto err_close_ruleset;
> }
> if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw,
> + 0, true)) {
> + goto err_close_ruleset;
> + }
> + /* Optional no-inherit rules mirror the regular read-only/read-write sets. */
> + if (populate_ruleset_fs(ENV_FS_RO_NO_INHERIT_NAME, ruleset_fd,
> + access_fs_ro, LANDLOCK_ADD_RULE_NO_INHERIT,
> + false)) {
> + goto err_close_ruleset;
> + }
> + if (populate_ruleset_fs(ENV_FS_RW_NO_INHERIT_NAME, ruleset_fd,
> + access_fs_rw, LANDLOCK_ADD_RULE_NO_INHERIT,
These need to be under an ABI version check like the quiet one - this
sandboxer is designed to "downgrade" what it tries to do gracefully if
running on older kernel.
However, there is an argument that maybe if deny rules aren't supported in
the current running kernel, it should just refuse to run the program at
all, otherwise by running the sandboxed program without the deny rules it
might expose the user to risks they might mitigate via some other means.
But in that case we should still have a better error message when running
on an older kernel than "Failed to update the ruleset with ...: Invalid
argument"
> false)) {
> goto err_close_ruleset;
> }
> /* Don't require this env to be present. */
> - if (quiet_supported && getenv(ENV_FS_QUIET_NAME)) {
> + if (quiet_supported) {
> if (populate_ruleset_fs(ENV_FS_QUIET_NAME, ruleset_fd, 0,
> - true)) {
> + LANDLOCK_ADD_RULE_QUIET, false)) {
> goto err_close_ruleset;
> }
> }
^ permalink raw reply
* Re: [PATCH v3 1/5] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT
From: Tingmao Wang @ 2025-12-03 21:12 UTC (permalink / raw)
To: Justin Suess, linux-security-module
Cc: Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün
In-Reply-To: <20251126122039.3832162-2-utilityemal77@gmail.com>
Hi!
I've done a "partial" review of this series (with most of the attention on
this patch). Aside from the comments below, I think we might need to
think a bit more about the implications of things like hard links and bind
mounts, which makes the notion of "parent" non-trivial (as Mickaël also
pointed out on the GitHub thread). However, I think it should mostly be
good. My first pass of reasoning for bind mounts are:
A deny rule does not prevent access through a bind mount if the bind mount
points deep into the denied hierarchy (not to the denied dentry itself),
but does protect it if the bind mount points to the denied dentry itself
or its parent. Therefore, to properly protect a directory that contains
children that might possibly be bind mounted to, a sandboxer just has to
attach deny rules to that directory, plus any bind mounts pointing toward
anything in it, which seems like a reasonable ask given that the sandboxed
application cannot make mounts itself.
Hopefully this review turns out to be useful :)
On 11/26/25 12:20, Justin Suess wrote:
> Implements a flag to prevent access grant inheritance within the filesystem hierarchy
> for landlock rules.
>
> If a landlock rule on an inode has this flag, any access grants on parent inodes will be
> ignored. Moreover, operations that involve altering the direct parent tree of the subject with
I feel like the wording "direct parent tree" is a bit unclear - I think
what you meant is "altering the parent of the subject or its ancestors",
right?
> LANDLOCK_ADD_RULE_NO_INHERIT will be denied up to the mountpoint.
Tbh I'm not entirely clear on why we only deny rename of parents up to the
mountpoint.
>
> Additionally (new in v3) parent flag inheritance is blocked by this flag, allowing fine
> grained access control over LANDLOCK_ADD_RULE_QUIET.
>
> For example, if /a/b/c/ = read only + LANDLOCK_ADD_RULE_NO_INHERIT and / = read write, writes to
> files in /a/b/c will be denied. Moreover, moving /a to /bad, removing /a/b/c, or creating links to
> /a will be prohibited.
>
> And if / has LANDLOCK_ADD_RULE_QUIET, /a/b/c will still audit (handled)
> accesses. This is because LANDLOCK_ADD_RULE_NO_INHERIT also
> suppresses flag inheritance from parent objects.
>
> The parent directory restrictions mitigate sandbox-restart attacks. For example, if a sandboxed program
> is able to move a LANDLOCK_ADD_RULE_NO_INHERIT restricted directory, upon sandbox restart, the policy
> applied naively on the same filenames would be invalid. Preventing these operations mitigates these attacks.
>
> v2..v3 changes:
>
> * Parent directory topology protections now work by lazily
> inserting blank rules on parent inodes if they do not
> exist. This replaces the previous xarray implementation
> with simplified logic.
> * Added an optimization to skip further processing if all layers collected
> no inherit.
> * Added support to block flag inheritance.
>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> ---
> security/landlock/audit.c | 4 +-
> security/landlock/domain.c | 4 +-
> security/landlock/fs.c | 592 +++++++++++++++++++++++++++++++++++-
> security/landlock/ruleset.c | 27 +-
> security/landlock/ruleset.h | 36 ++-
> 5 files changed, 645 insertions(+), 18 deletions(-)
>
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index d51563712325..4da97dd6985c 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -588,7 +588,9 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> subject->domain, &missing, request->layer_masks,
> request->layer_masks_size);
> object_quiet_flag = !!(request->rule_flags.quiet_masks &
> - BIT(youngest_layer));
> + BIT(youngest_layer)) &&
> + !(request->rule_flags.blocked_flag_masks &
> + BIT(youngest_layer));
> } else {
> youngest_layer = get_layer_from_deny_masks(
> &missing, request->all_existing_optional_access,
> diff --git a/security/landlock/domain.c b/security/landlock/domain.c
> index 8caf07250328..5bd83865c87d 100644
> --- a/security/landlock/domain.c
> +++ b/security/landlock/domain.c
> @@ -236,7 +236,9 @@ optional_access_t landlock_get_quiet_optional_accesses(
> BITS_PER_TYPE(access_mask_t)) {
> const u8 layer = (deny_masks >> (access_index * 4)) &
> (LANDLOCK_MAX_NUM_LAYERS - 1);
> - const bool is_quiet = !!(rule_flags.quiet_masks & BIT(layer));
> + const layer_mask_t layer_bit = BIT(layer);
> + const bool is_quiet = !!(rule_flags.quiet_masks & layer_bit) &&
> + !(rule_flags.blocked_flag_masks & layer_bit);
>
> if (is_quiet)
> quiet_optional_accesses |= BIT(access_index);
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 29f10da32141..0a5c73f18f26 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -317,6 +317,206 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
> LANDLOCK_ACCESS_FS_IOCTL_DEV)
> /* clang-format on */
>
> +static const struct landlock_rule *find_rule(const struct landlock_ruleset *const domain,
> + const struct dentry *const dentry);
> +
> +/**
> + * landlock_domain_layers_mask - Build a mask covering all layers of a domain
> + * @domain: The ruleset (domain) to inspect.
> + *
> + * Return a layer mask with a 1 bit for each existing layer of @domain.
> + * If @domain has no layers 0 is returned. If the number of layers is
> + * greater than or equal to the number of bits in layer_mask_t, all bits
> + * are set.
> + */
> +static layer_mask_t landlock_domain_layers_mask(const struct landlock_ruleset
> + *const domain)
> +{
> + if (!domain || !domain->num_layers)
> + return 0;
> +
> + if (domain->num_layers >= sizeof(layer_mask_t) * BITS_PER_BYTE)
> + return (layer_mask_t)~0ULL;
> +
> + return GENMASK_ULL(domain->num_layers - 1, 0);
> +}
> +
> +/**
> + * rule_blocks_all_layers_no_inherit - check whether a rule disables inheritance
> + * @domain_layers_mask: Mask describing the domain's active layers.
> + * @rule: Rule to inspect.
> + *
> + * Return true if every layer present in @rule has its no_inherit flag set
> + * and the set of layers covered by the rule equals @domain_layers_mask.
> + * This indicates that the rule prevents inheritance on all layers of the
> + * domain and thus further walking for inheritance checks can stop.
> + */
> +static bool rule_blocks_all_layers_no_inherit(const layer_mask_t domain_layers_mask,
> + const struct landlock_rule *const rule)
> +{
> + layer_mask_t rule_layers = 0;
> + u32 layer_index;
> +
> + if (!domain_layers_mask || !rule)
> + return false;
> +
> + for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
> + const struct landlock_layer *const layer =
> + &rule->layers[layer_index];
> + const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
> +
> + if (!layer->flags.no_inherit)
> + return false;
> +
> + rule_layers |= layer_bit;
> + }
> +
> + return rule_layers && rule_layers == domain_layers_mask;
> +}
> +
> +/**
> + * landlock_collect_no_inherit_layers - Collects layers with no_inherit flags
> + */
Note likely misplaced comment here
> +/**
> + * landlock_collect_no_inherit_layers - collect effective no_inherit layers
> + * @ruleset: Ruleset to consult.
> + * @dentry: Dentry used as a starting point for the upward walk.
> + *
> + * Walk upwards from @dentry and return a layer mask containing the layers
> + * for which either a rule on the visited dentry has the no_inherit flag set
> + * or where an ancestor was previously marked as having a descendant with
> + * a no_inherit rule. The search prefers the closest matching dentry and
> + * stops once any relevant layer bits are found or the root is reached.
> + *
> + * Returns a layer_mask_t where each set bit corresponds to a layer with an
> + * effective no_inherit influence for @dentry. Returns 0 if none apply or if
> + * inputs are invalid.
> + */
> +static layer_mask_t landlock_collect_no_inherit_layers(const struct landlock_ruleset
> + *const ruleset,
> + struct dentry *const dentry)
> +{
> + struct dentry *cursor, *parent;
> + layer_mask_t layers = 0;
> + bool include_descendants = true;
> +
> + if (!ruleset || !dentry || d_is_negative(dentry))
> + return 0;
> +
> + cursor = dget(dentry);
> + while (true) {
> + const struct landlock_rule *rule;
> + u32 layer_index;
> +
> + rule = find_rule(ruleset, cursor);
> + if (rule) {
> + for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
> + const struct landlock_layer *layer = &rule->layers[layer_index];
> +
> + if (layer->flags.no_inherit ||
> + (include_descendants &&
> + layer->flags.has_no_inherit_descendant))
> + layers |= BIT_ULL((layer->level ?
> + layer->level : layer_index + 1) - 1);
> + }
> + }
> +
> + if (layers) {
> + dput(cursor);
> + return layers;
> + }
> +
> + if (IS_ROOT(cursor)) {
> + dput(cursor);
> + break;
> + }
> +
> + parent = dget_parent(cursor);
> + dput(cursor);
> + if (!parent)
> + break;
> +
> + cursor = parent;
> + include_descendants = false;
> + }
> + return 0;
> +}
> +
> +static int mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
> + struct dentry *dentry,
> + layer_mask_t descendant_layers);
Wouldn't you be able to avoid this declaration by moving the definition
for ensure_rule_for_dentry and mark_no_inherit_ancestors up a bit, before
mask_no_inherit_descendant_layers?
> +
> +/**
> + * mask_no_inherit_descendant_layers - apply descendant no_inherit masking
> + * @domain: The ruleset (domain) to consult.
> + * @dentry: The dentry whose descendants are considered.
> + * @child_layers: Layers present on the child that may be subject to masking.
> + * @access_request: Accesses being requested (bitmask).
> + * @layer_masks: Per-access layer masks to be modified in-place.
> + * @rule_flags: Collected flags which will be updated accordingly.
> + *
> + * If descendant dentries have no_inherit, clear that
> + * layer's bit from @layer_masks. Also updates @rule_flags to reflect
> + * which layers were blocked. Returns true if any of the @layer_masks were
> + * modified, false otherwise.
> + */
> +static bool mask_no_inherit_descendant_layers(const struct landlock_ruleset
> + *const domain,
> + struct dentry *const dentry,
> + layer_mask_t child_layers,
> + const access_mask_t access_request,
> + layer_mask_t
> + (*const layer_masks)
> + [LANDLOCK_NUM_ACCESS_FS],
> + struct collected_rule_flags
> + *const rule_flags)
> +{
> + layer_mask_t descendant_layers;
> + const unsigned long access_req = access_request;
> + unsigned long access_bit;
> + bool changed = false;
> +
> + if (!access_request || !layer_masks || !rule_flags || !dentry)
> + return false;
> + if (d_is_negative(dentry))
> + return false;
> +
> + descendant_layers = landlock_collect_no_inherit_layers(domain, dentry);
> + {
> + layer_mask_t shared_layers = descendant_layers & child_layers;
> +
> + if (shared_layers) {
> + rule_flags->no_inherit_masks |= shared_layers;
> + rule_flags->no_inherit_desc_masks |= shared_layers;
> + rule_flags->blocked_flag_masks |= shared_layers;
> + }
> + }
> + descendant_layers &= ~child_layers;
> + descendant_layers &= ~rule_flags->no_inherit_masks;
> + if (!descendant_layers)
> + return false;
> +
> + rule_flags->blocked_flag_masks |= descendant_layers;
> +
> + for_each_set_bit(access_bit, &access_req,
> + ARRAY_SIZE(*layer_masks)) {
> + layer_mask_t *const layer_mask = &(*layer_masks)[access_bit];
> +
> + if (*layer_mask & descendant_layers) {
> + *layer_mask &= ~descendant_layers;
> + changed = true;
> + }
> + }
> +
> + if (!changed)
> + return false;
> +
> + rule_flags->no_inherit_masks |= descendant_layers;
> + rule_flags->no_inherit_desc_masks |= descendant_layers;
> +
> + return true;
> +}
> +
> /*
> * @path: Should have been checked by get_path_from_fd().
> */
> @@ -325,12 +525,13 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> access_mask_t access_rights, const int flags)
> {
> int err;
> + const bool is_dir = d_is_dir(path->dentry);
> struct landlock_id id = {
> .type = LANDLOCK_KEY_INODE,
> };
>
> /* Files only get access rights that make sense. */
> - if (!d_is_dir(path->dentry) &&
> + if (!is_dir &&
> (access_rights | ACCESS_FILE) != ACCESS_FILE)
> return -EINVAL;
> if (WARN_ON_ONCE(ruleset->num_layers != 1))
> @@ -344,13 +545,43 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> return PTR_ERR(id.key.object);
> mutex_lock(&ruleset->lock);
> err = landlock_insert_rule(ruleset, id, access_rights, flags);
> + if (!err && (flags & LANDLOCK_ADD_RULE_NO_INHERIT)) {
> + const struct landlock_rule *rule;
> + layer_mask_t descendant_layers = 0;
> + u32 layer_index;
> +
> + rule = find_rule(ruleset, path->dentry);
> + if (rule) {
> + for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
This function is only called to add rules to an "unmerged" ruleset, which
will always only have one layer. So probably this can just be a
err = landlock_insert_rule(ruleset, id, access_rights, flags);
if (err)
goto out_unlock;
if (flags & LANDLOCK_ADD_RULE_NO_INHERIT) {
err = mark_no_inherit_ancestors(ruleset, path->dentry);
if (err)
goto out_unlock;
}
And for a similar reason, you don't have to do a for(layer_index) in
mark_no_inherit_ancestors either. (Basically I think you would just set
rule->layers[0].flags.has_no_inherit_descendant to true)
It might be helpful to validate / document this reasoning by adding
WARN_ONCE(rule->num_layers != 1, "unmerged rulesets should only have one
layer") either here or in mark_no_inherit_ancestors.
> + const struct landlock_layer *layer =
> + &rule->layers[layer_index];
> +
> + if (layer->flags.no_inherit ||
> + layer->flags.has_no_inherit_descendant)
> + descendant_layers |=
> + BIT_ULL((layer->level ?
> + layer->level : layer_index + 1) - 1);
> + }
> + if (descendant_layers) {
> + err = mark_no_inherit_ancestors(ruleset, path->dentry,
> + descendant_layers);
> + if (err)
> + goto out_unlock;
> + }
> + }
> + }
> mutex_unlock(&ruleset->lock);
> +out:
> /*
> * No need to check for an error because landlock_insert_rule()
> * increments the refcount for the new object if needed.
> */
> landlock_put_object(id.key.object);
> return err;
> +
> +out_unlock:
> + mutex_unlock(&ruleset->lock);
> + goto out;
> }
>
> /* Access-control management */
> @@ -382,6 +613,134 @@ find_rule(const struct landlock_ruleset *const domain,
> return rule;
> }
>
> +/**
> + * ensure_rule_for_dentry - ensure a ruleset contains a rule entry for dentry,
> + * inserting a blank rule if needed.
> + * @ruleset: Ruleset to modify/inspect. Caller must hold @ruleset->lock.
> + * @dentry: Dentry to ensure a rule exists for.
> + *
> + * If no rule is currently associated with @dentry, insert an empty rule
> + * (with zero access) tied to the backing inode. Returns a pointer to the
> + * rule associated with @dentry on success, NULL when @dentry is negative, or
> + * an ERR_PTR()-encoded error if the rule cannot be created.
> + *
> + * This is useful for LANDLOCK_ADD_RULE_NO_INHERIT processing, where a rule
> + * may need to be created for an ancestor dentry that does not yet have one
> + * to properly track no_inherit flags.
> + *
> + * The flags are set to zero if a rule is newly created, and the caller
> + * is responsible for setting them appropriately.
> + *
> + * The returned rule pointer's lifetime is tied to @ruleset.
> + */
> +static const struct landlock_rule *
> +ensure_rule_for_dentry(struct landlock_ruleset *const ruleset,
> + struct dentry *const dentry)
> +{
> + struct landlock_id id = {
> + .type = LANDLOCK_KEY_INODE,
> + };
> + const struct landlock_rule *rule;
> + int err;
> +
> + if (!ruleset || !dentry || d_is_negative(dentry))
> + return NULL;
> +
> + lockdep_assert_held(&ruleset->lock);
> +
> + rule = find_rule(ruleset, dentry);
> + if (rule)
> + return rule;
> +
> + id.key.object = get_inode_object(d_backing_inode(dentry));
> + if (IS_ERR(id.key.object))
> + return ERR_CAST(id.key.object);
> +
> + err = landlock_insert_rule(ruleset, id, 0, 0);
> + landlock_put_object(id.key.object);
> + if (err)
> + return ERR_PTR(err);
> +
> + rule = find_rule(ruleset, dentry);
> + return rule ? rule : ERR_PTR(-ENOENT);
I feel like this deserves a WARN_ON_ONCE(!rule) before this line - we
don't really expect to not find a rule right after adding it.
On the other hand, the only reason why we need to re-lookup the rule seems
to be because landlock_insert_rule() does not return the newly added rule.
We could change it to do so, and not have to do this extra lookup.
This also nicely solves the constness issue - landlock_insert_rule could
return the mutable pointer.
> +}
> +
> +/**
> + * mark_no_inherit_ancestors - mark ancestors as having no_inherit descendants
> + * @ruleset: Ruleset to modify. Caller must hold @ruleset->lock.
> + * @dentry: Dentry representing the descendant that carries no_inherit bits.
> + * @descendant_layers: Mask of layers from the descendant that should be
> + * advertised to ancestors via has_no_inherit_descendant.
> + *
> + * Walks upward from @dentry and ensures that any ancestor rule contains the
> + * has_no_inherit_descendant marker for the specified @descendant_layers so
> + * parent lookups can quickly detect descendant no_inherit influence.
> + *
> + * Returns 0 on success or a negative errno if ancestor bookkeeping fails.
> + */
> +static int mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
> + struct dentry *dentry,
> + layer_mask_t descendant_layers)
> +{
> + struct dentry *cursor;
> + u32 layer_index;
> + int err = 0;
> +
> + if (!ruleset || !dentry || !descendant_layers)
> + return -EINVAL;
> +
> + lockdep_assert_held(&ruleset->lock);
> +
> + cursor = dget(dentry);
> + while (cursor) {
> + struct dentry *parent;
> +
> + if (IS_ROOT(cursor)) {
> + dput(cursor);
> + break;
> + }
> +
> + parent = dget_parent(cursor);
> + dput(cursor);
> + if (!parent)
> + break;
> +
> + if (!d_is_negative(parent)) {
My understanding is that if the child is not negative (which is required
for us to actually get here), as long as we always have a reference to it,
none of its parents should be negative as well. This should probably be a
WARN_ON_ONCE(d_is_negative(parent)).
I think some of the other d_is_negative() checks in this patch also have a
similar argument (i.e. turn into WARN_ON_ONCE or be removed if not
necessary), but I've not looked at them all.
> + const struct landlock_rule *rule;
> + /* Ensures a rule exists for the parent dentry,
> + * inserting a blank one if needed
> + */
> + rule = ensure_rule_for_dentry(ruleset, parent);
> + if (IS_ERR(rule)) {
> + err = PTR_ERR(rule);
> + dput(parent);
> + cursor = NULL;
> + break;
> + }
> + if (rule) {
> + struct landlock_rule *mutable_rule =
> + (struct landlock_rule *)rule;
> +
> + for (layer_index = 0;
> + layer_index < mutable_rule->num_layers;
> + layer_index++) {
> + struct landlock_layer *layer =
> + &mutable_rule->layers[layer_index];
> + layer_mask_t layer_bit =
> + BIT_ULL((layer->level ?
> + layer->level : layer_index + 1) - 1);
> +
> + if (descendant_layers & layer_bit)
> + layer->flags.has_no_inherit_descendant = true;
> + }
> + }
> + }
> +
> + cursor = parent;
> + }
> + return err;
> +}
> +
> /*
> * Allows access to pseudo filesystems that will never be mountable (e.g.
> * sockfs, pipefs), but can still be reachable through
> @@ -764,6 +1123,8 @@ static bool is_access_to_paths_allowed(
> struct landlock_request *const log_request_parent2,
> struct dentry *const dentry_child2)
> {
> + const layer_mask_t domain_layers_mask =
> + landlock_domain_layers_mask(domain);
> bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check,
> is_dom_check_bkp, child1_is_directory = true,
> child2_is_directory = true;
> @@ -778,6 +1139,13 @@ static bool is_access_to_paths_allowed(
> struct collected_rule_flags *rule_flags_parent1 = &log_request_parent1->rule_flags;
> struct collected_rule_flags *rule_flags_parent2 = &log_request_parent2->rule_flags;
> struct collected_rule_flags _rule_flag_parent1_bkp, _rule_flag_parent2_bkp;
> + layer_mask_t child1_layers = 0;
> + layer_mask_t child2_layers = 0;
> +
> + if (dentry_child1)
> + child1_layers = landlock_collect_no_inherit_layers(domain, dentry_child1);
> + if (dentry_child2)
> + child2_layers = landlock_collect_no_inherit_layers(domain, dentry_child2);
>
> if (!access_request_parent1 && !access_request_parent2)
> return true;
> @@ -931,6 +1299,10 @@ static bool is_access_to_paths_allowed(
> ARRAY_SIZE(*layer_masks_parent2),
> rule_flags_parent2);
>
> + if (rule &&
> + rule_blocks_all_layers_no_inherit(domain_layers_mask, rule))
> + break;
> +
> /* Stops when a rule from each layer grants access. */
> if (allowed_parent1 && allowed_parent2) {
> /*
> @@ -976,8 +1348,13 @@ static bool is_access_to_paths_allowed(
> memcpy(&_rule_flag_parent2_bkp,
> rule_flags_parent2,
> sizeof(_rule_flag_parent2_bkp));
> - is_dom_check_bkp = is_dom_check;
> }
> + is_dom_check_bkp = is_dom_check;
> + child1_layers = landlock_collect_no_inherit_layers(domain,
> + walker_path
> + .dentry);
> + if (layer_masks_parent2)
> + child2_layers = child1_layers;
>
> /* Ignores hidden mount points. */
> goto jump_up;
> @@ -1001,15 +1378,50 @@ static bool is_access_to_paths_allowed(
> break;
> }
>
> - /*
> - * We reached a disconnected root directory from a bind mount, and
> - * we need to reset the walk to the current mount root.
> - */
> - goto reset_to_mount_root;
> - }
> - parent_dentry = dget_parent(walker_path.dentry);
> - dput(walker_path.dentry);
> - walker_path.dentry = parent_dentry;
> + /*
> + * We reached a disconnected root directory from a bind mount, and
> + * we need to reset the walk to the current mount root.
> + */
Accidental change of indentation?
> + goto reset_to_mount_root;
> + }
> + if (likely(!d_is_negative(walker_path.dentry))) {
> + child1_layers = landlock_collect_no_inherit_layers(domain,
> + walker_path.dentry);
> + if (layer_masks_parent2)
> + child2_layers = child1_layers;
> + } else {
> + child1_layers = 0;
> + if (layer_masks_parent2)
> + child2_layers = 0;
> + }
> + parent_dentry = dget_parent(walker_path.dentry);
> + dput(walker_path.dentry);
> + walker_path.dentry = parent_dentry;
> + /*
> + * Apply descendant no-inherit masking now that we've moved to the
> + * parent. This ensures the parent respects any no-inherit rules from
> + * the child we just left. Only applies to refer operations (rename/link).
> + */
> + if (unlikely(layer_masks_parent2)) {
> + if (mask_no_inherit_descendant_layers(domain, walker_path.dentry,
> + child1_layers,
> + access_masked_parent1,
> + layer_masks_parent1,
> + rule_flags_parent1))
> + allowed_parent1 =
> + allowed_parent1 ||
> + is_layer_masks_allowed(layer_masks_parent1);
> +
> + if (rule_flags_parent2 &&
> + mask_no_inherit_descendant_layers(domain, walker_path.dentry,
> + child2_layers,
> + access_masked_parent2,
> + layer_masks_parent2,
> + rule_flags_parent2))
> + allowed_parent2 =
> + allowed_parent2 ||
> + is_layer_masks_allowed(layer_masks_parent2);
> + }
Maybe I'm missing something, but I can't tell what's the purpose of this
block, or in fact what mask_no_inherit_descendant_layers and
landlock_collect_no_inherit_layers is for. The doc comment for
mask_no_inherit_descendant_layers seems to suggest that it's supposed to
walk descendents, but landlock_collect_no_inherit_layers is actually
walking ancestors. Removing these checks doesn't seem to break any tests,
and sandboxer still seems to work as expected wrt. no_inherit rules and
denial of renaming denied dentries and its parents.
Note that the special "reverting" style disconnected directory handling
has been removed in Mickaël's next branch (i.e. the "backup" logic is
removed), which should hopefully simplify reasoning about this.
> continue;
>
> reset_to_mount_root:
> @@ -1057,6 +1469,10 @@ static bool is_access_to_paths_allowed(
> dput(walker_path.dentry);
> walker_path.dentry = walker_path.mnt->mnt_root;
> dget(walker_path.dentry);
> + child1_layers = landlock_collect_no_inherit_layers(domain,
> + walker_path.dentry);
> + if (layer_masks_parent2)
> + child2_layers = child1_layers;
> }
> path_put(&walker_path);
>
> @@ -1172,6 +1588,8 @@ static bool collect_domain_accesses(
> struct collected_rule_flags *const rule_flags)
> {
> access_mask_t access_dom;
> + const layer_mask_t domain_layers_mask =
> + landlock_domain_layers_mask(domain);
> bool ret = false;
>
> if (WARN_ON_ONCE(!domain || !mnt_dir || !dir || !layer_masks_dom))
> @@ -1187,9 +1605,11 @@ static bool collect_domain_accesses(
> while (true) {
> struct dentry *parent_dentry;
>
> + const struct landlock_rule *rule = find_rule(domain, dir);
> +
> /* Gets all layers allowing all domain accesses. */
> if (landlock_unmask_layers(
> - find_rule(domain, dir), access_dom, layer_masks_dom,
> + rule, access_dom, layer_masks_dom,
> ARRAY_SIZE(*layer_masks_dom), rule_flags)) {
> /*
> * Before allowing this side of the access request, checks that the
> @@ -1206,6 +1626,10 @@ static bool collect_domain_accesses(
> break;
> }
>
> + if (rule &&
> + rule_blocks_all_layers_no_inherit(domain_layers_mask, rule))
> + break;
> +
> /* Stops at the mount point. */
> if (dir == mnt_dir->dentry)
> break;
> @@ -1232,6 +1656,121 @@ static bool collect_domain_accesses(
> return ret;
> }
>
> +/**
> + * collect_topology_sealed_layers - collect layers sealed against topology changes
> + * @domain: Ruleset to consult.
> + * @dentry: Starting dentry for the upward walk.
> + * @override_layers: Optional out parameter filled with layers that are
> + * present on ancestors but considered overrides (not
> + * sealing the topology for descendants).
> + *
> + * Walk upwards from @dentry and return a mask of layers where either the
> + * visited dentry contains a no_inherit rule or ancestors were previously
> + * marked as having a descendant with no_inherit. @override_layers, if not
> + * NULL, is filled with layers that would normally be overridden by more
> + * specific descendant rules.
> + *
> + * Returns a layer mask where set bits indicate layers that are "sealed"
> + * (topology changes like rename/rmdir are denied) for the subtree rooted at
> + * @dentry.
> + *
> + * Useful for LANDLOCK_ADD_RULE_NO_INHERIT parent directory enforcement to ensure
> + * that topology changes do not violate the no_inherit constraints.
> + */
> +static layer_mask_t
> +collect_topology_sealed_layers(const struct landlock_ruleset *const domain,
> + struct dentry *dentry,
> + layer_mask_t *const override_layers)
> +{
> + struct dentry *cursor, *parent;
> + bool include_descendants = true;
> + layer_mask_t sealed_layers = 0;
> +
> + if (override_layers)
> + *override_layers = 0;
> +
> + if (!domain || !dentry || d_is_negative(dentry))
> + return 0;
> +
> + cursor = dget(dentry);
> + while (cursor) {
> + const struct landlock_rule *rule;
> + u32 layer_index;
> +
> + rule = find_rule(domain, cursor);
> + if (rule) {
> + for (layer_index = 0; layer_index < rule->num_layers;
> + layer_index++) {
> + const struct landlock_layer *layer =
> + &rule->layers[layer_index];
> + const int level = layer->level ? layer->level :
> + layer_index + 1;
Wouldn't layer->level always be >= 1 here? Using layer_index doesn't make
sense since layer_index is just the index that the struct landlock_layer
happened to be in that rule's array.
> + layer_mask_t layer_bit = BIT_ULL(level - 1);
> +
> + if (include_descendants &&
> + (layer->flags.no_inherit ||
> + layer->flags.has_no_inherit_descendant)) {
> + sealed_layers |= layer_bit;
> + } else if (override_layers) {
> + *override_layers |= layer_bit;
> + }
> + }
> + }
> +
> + if (sealed_layers || IS_ROOT(cursor))
> + break;
> +
> + parent = dget_parent(cursor);
> + dput(cursor);
> + if (!parent)
> + return sealed_layers;
> +
> + cursor = parent;
> + include_descendants = false;
> + }
> + dput(cursor);
> + return sealed_layers;
> +}
> +
> +/**
> + * deny_no_inherit_topology_change - deny topology changes on sealed layers
> + * @subject: Subject performing the operation (contains the domain).
> + * @dentry: Dentry that is the target of the topology modification.
> + *
> + * Checks whether any domain layers are sealed against topology changes at
> + * @dentry (via collect_topology_sealed_layers). If so, emit an audit record
> + * and return -EACCES. Otherwise return 0.
> + */
> +static int deny_no_inherit_topology_change(const struct landlock_cred_security
> + *subject,
> + struct dentry *dentry)
> +{
> + layer_mask_t sealed_layers;
> + layer_mask_t override_layers;
> + unsigned long layer_index;
> +
> + if (!subject || !dentry || d_is_negative(dentry))
> + return 0;
> + sealed_layers = collect_topology_sealed_layers(subject->domain,
> + dentry, &override_layers);
> + sealed_layers &= ~override_layers;
> +
> + if (!sealed_layers)
> + return 0;
> +
> + layer_index = __ffs((unsigned long)sealed_layers);
> + landlock_log_denial(subject, &(struct landlock_request) {
> + .type = LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY,
> + .audit = {
> + .type = LSM_AUDIT_DATA_DENTRY,
> + .u.dentry = dentry,
> + },
> + .layer_plus_one = layer_index + 1,
> + });
> +
> + return -EACCES;
> +}
> +
> /**
> * current_check_refer_path - Check if a rename or link action is allowed
> *
> @@ -1316,6 +1855,16 @@ static int current_check_refer_path(struct dentry *const old_dentry,
> access_request_parent2 =
> get_mode_access(d_backing_inode(old_dentry)->i_mode);
> if (removable) {
> + int err;
> +
> + err = deny_no_inherit_topology_change(subject, old_dentry);
> + if (err)
> + return err;
> + if (exchange) {
> + err = deny_no_inherit_topology_change(subject, new_dentry);
> + if (err)
> + return err;
> + }
> access_request_parent1 |= maybe_remove(old_dentry);
> access_request_parent2 |= maybe_remove(new_dentry);
> }
> @@ -1707,12 +2256,31 @@ static int hook_path_symlink(const struct path *const dir,
> static int hook_path_unlink(const struct path *const dir,
> struct dentry *const dentry)
> {
> + const struct landlock_cred_security *const subject =
> + landlock_get_applicable_subject(current_cred(), any_fs, NULL);
> + int err;
> +
> + if (subject) {
> + err = deny_no_inherit_topology_change(subject, dentry);
> + if (err)
> + return err;
> + }
> return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE);
> }
>
> static int hook_path_rmdir(const struct path *const dir,
> struct dentry *const dentry)
> {
> + const struct landlock_cred_security *const subject =
> + landlock_get_applicable_subject(current_cred(), any_fs, NULL);
> + int err;
> +
> + if (subject) {
> + err = deny_no_inherit_topology_change(subject, dentry);
> + if (err)
> + return err;
> + }
> +
> return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> }
>
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 750a444e1983..f7b6a48bbf39 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -255,8 +255,13 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
> return -EINVAL;
> if (WARN_ON_ONCE(this->layers[0].level != 0))
> return -EINVAL;
> + /* Merge the flags into the rules */
> this->layers[0].access |= (*layers)[0].access;
> this->layers[0].flags.quiet |= (*layers)[0].flags.quiet;
> + this->layers[0].flags.no_inherit |=
> + (*layers)[0].flags.no_inherit;
> + this->layers[0].flags.has_no_inherit_descendant |=
> + (*layers)[0].flags.has_no_inherit_descendant;
> return 0;
> }
>
> @@ -315,7 +320,10 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
> .level = 0,
> .flags = {
> .quiet = !!(flags & LANDLOCK_ADD_RULE_QUIET),
> - },
> + .no_inherit = !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
> + .has_no_inherit_descendant =
> + !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
> + }
> } };
>
> build_check_layer();
> @@ -662,9 +670,22 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
> unsigned long access_bit;
> bool is_empty;
>
> - /* Collect rule flags for each layer. */
> - if (rule_flags && layer->flags.quiet)
> + /* Skip layers that already have no inherit flags. */
> + if (rule_flags &&
> + (rule_flags->no_inherit_masks & layer_bit))
> + continue;
> +
> + /* Collect rule flags for each layer.
> + * We block flag inheritance if needed
> + * because of a no_inherit rule.
> + */
> + if (rule_flags && layer->flags.quiet &&
> + !(rule_flags->blocked_flag_masks & layer_bit))
I don't quite understand the purpose of blocked_flag_masks - wouldn't the
"continue;" above naturally prevent flag inheritance?
> rule_flags->quiet_masks |= layer_bit;
> + if (rule_flags && layer->flags.no_inherit)
> + rule_flags->no_inherit_masks |= layer_bit;
> + if (rule_flags && layer->flags.has_no_inherit_descendant)
> + rule_flags->no_inherit_desc_masks |= layer_bit;
>
> /*
> * Records in @layer_masks which layer grants access to each requested
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index eb60db646422..8b46ab14e995 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -40,6 +40,21 @@ struct landlock_layer {
> * down the file hierarchy.
> */
> bool quiet:1;
> + /**
> + * @no_inherit: Prevents this rule from being inherited by
> + * descendant directories in the filesystem layer. Only used
> + * for filesystem rules.
> + */
> + bool no_inherit:1;
> + /**
> + * @has_no_inherit_descendant: Marker to indicate that this layer
> + * has at least one descendant directory with a rule having the
> + * no_inherit flag. Only used for filesystem rules.
> + * This "flag" is not set by the user, but by Landlock on
> + * parent directories of rules when the child rule has
> + * a rule with the no_inherit flag.
> + */
> + bool has_no_inherit_descendant:1;
> } flags;
> /**
> * @access: Bitfield of allowed actions on the kernel object. They are
> @@ -49,13 +64,32 @@ struct landlock_layer {
> };
>
> /**
> - * struct collected_rule_flags - Hold accumulated flags for each layer.
> + * struct collected_rule_flags - Hold accumulated flags and their markers for each layer.
> */
> struct collected_rule_flags {
> /**
> * @quiet_masks: Layers for which the quiet flag is effective.
> */
> layer_mask_t quiet_masks;
> + /**
> + * @no_inherit_masks: Layers for which the no_inherit flag is effective.
> + */
> + layer_mask_t no_inherit_masks;
> + /**
> + * @no_inherit_desc_masks: Layers for which the
> + * has_no_inherit_descendant tag is effective.
> + * This is not a flag itself, but a marker set on ancestors
> + * of rules with the no_inherit flag to deny topology changes
> + * in the direct parent path.
> + */
> + layer_mask_t no_inherit_desc_masks;
> + /**
> + * @blocked_flag_masks: Layers where flag inheritance must be blocked
> + * because of a no_inherit rule. This is not a flag itself, but a marker
> + * for layers that have their flags blocked due to no_inherit rule
> + * propagation.
> + */
> + layer_mask_t blocked_flag_masks;
> };
>
> /**
^ permalink raw reply
* Re: [PATCH v2] security,fs,nfs,net: update security_inode_listsecurity() interface
From: Stephen Smalley @ 2025-12-03 20:01 UTC (permalink / raw)
To: Paul Moore
Cc: Anna Schumaker, Trond Myklebust, Anna Schumaker, Jakub Kicinski,
Casey Schaufler, Alexander Viro, Christian Brauner, Jan Kara,
James Morris, Serge E. Hallyn, Eric Dumazet, Paolo Abeni,
Willem de Bruijn, David S. Miller, Simon Horman, Ondrej Mosnacek,
linux-nfs, linux-kernel, linux-fsdevel, linux-security-module,
netdev, selinux
In-Reply-To: <CAEjxPJ6e8z__=MP5NfdUxkOMQ=EnUFSjWFofP4YPwHqK=Ki5nw@mail.gmail.com>
On Wed, Dec 3, 2025 at 1:08 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Dec 3, 2025 at 10:55 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Dec 3, 2025 at 10:35 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > On Wed, Jul 23, 2025 at 10:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Thu, Jun 19, 2025 at 5:18 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Tue, May 27, 2025 at 5:03 PM Anna Schumaker
> > > > > <anna.schumaker@oracle.com> wrote:
> > > > > > On 5/20/25 5:31 PM, Paul Moore wrote:
> > > > > > > On Tue, Apr 29, 2025 at 7:34 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > >> On Mon, Apr 28, 2025 at 4:15 PM Stephen Smalley
> > > > > > >> <stephen.smalley.work@gmail.com> wrote:
> > > > > > >>>
> > > > > > >>> Update the security_inode_listsecurity() interface to allow
> > > > > > >>> use of the xattr_list_one() helper and update the hook
> > > > > > >>> implementations.
> > > > > > >>>
> > > > > > >>> Link: https://lore.kernel.org/selinux/20250424152822.2719-1-stephen.smalley.work@gmail.com/
> > > > > > >>>
> > > > > > >>> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > > > >>> ---
> > > > > > >>> This patch is relative to the one linked above, which in theory is on
> > > > > > >>> vfs.fixes but doesn't appear to have been pushed when I looked.
> > > > > > >>>
> > > > > > >>> fs/nfs/nfs4proc.c | 10 ++++++----
> > > > > > >>> fs/xattr.c | 19 +++++++------------
> > > > > > >>> include/linux/lsm_hook_defs.h | 4 ++--
> > > > > > >>> include/linux/security.h | 5 +++--
> > > > > > >>> net/socket.c | 17 +++++++----------
> > > > > > >>> security/security.c | 16 ++++++++--------
> > > > > > >>> security/selinux/hooks.c | 10 +++-------
> > > > > > >>> security/smack/smack_lsm.c | 13 ++++---------
> > > > > > >>> 8 files changed, 40 insertions(+), 54 deletions(-)
> > > > > > >>
> > > > > > >> Thanks Stephen. Once we get ACKs from the NFS, netdev, and Smack
> > > > > > >> folks I can pull this into the LSM tree.
> > > > > > >
> > > > > > > Gentle ping for Trond, Anna, Jakub, and Casey ... can I get some ACKs
> > > > > > > on this patch? It's a little late for the upcoming merge window, but
> > > > > > > I'd like to merge this via the LSM tree after the merge window closes.
> > > > > >
> > > > > > For the NFS change:
> > > > > > Acked-by: Anna Schumaker <anna.schumaker@oracle.com>
> > > > >
> > > > > Hi Anna,
> > > > >
> > > > > Thanks for reviewing the patch. Unfortunately when merging the patch
> > > > > today and fixing up some merge conflicts I bumped into an odd case in
> > > > > the NFS space and I wanted to check with you on how you would like to
> > > > > resolve it.
> > > > >
> > > > > Commit 243fea134633 ("NFSv4.2: fix listxattr to return selinux
> > > > > security label")[1] adds a direct call to
> > > > > security_inode_listsecurity() in nfs4_listxattr(), despite the
> > > > > existing nfs4_listxattr_nfs4_label() call which calls into the same
> > > > > LSM hook, although that call is conditional on the server supporting
> > > > > NFS_CAP_SECURITY_LABEL. Based on a quick search, it appears the only
> > > > > caller for nfs4_listxattr_nfs4_label() is nfs4_listxattr() so I'm
> > > > > wondering if there isn't some room for improvement here.
> > > > >
> > > > > I think there are two obvious options, and I'm curious about your
> > > > > thoughts on which of these you would prefer, or if there is another
> > > > > third option that you would like to see merged.
> > > > >
> > > > > Option #1:
> > > > > Essentially back out commit 243fea134633, removing the direct LSM call
> > > > > in nfs4_listxattr() and relying on the nfs4_listxattr_nfs4_label() for
> > > > > the LSM/SELinux xattrs. I think we would want to remove the
> > > > > NFS_CAP_SECURITY_LABEL check and build nfs4_listxattr_nfs4_label()
> > > > > regardless of CONFIG_NFS_V4_SECURITY_LABEL.
> > > > >
> > > > > Option #2:
> > > > > Remove nfs4_listxattr_nfs4_label() entirely and keep the direct LSM
> > > > > call in nfs4_listxattr(), with the required changes for this patch.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > [1] https://lore.kernel.org/all/20250425180921.86702-1-okorniev@redhat.com/
> > > >
> > > > A gentle ping on the question above for the NFS folks. If I don't
> > > > hear anything I'll hack up something and send it out for review, but I
> > > > thought it would nice if we could sort out the proper fix first.
> > >
> > > Raising this thread back up again to see if the NFS folks have a
> > > preference on option #1 or #2 above, or
> > > something else altogether. Should returning of the security.selinux
> > > xattr name from listxattr() be dependent on
> > > NFS_CAP_SECURITY_LABEL being set by the server and should it be
> > > dependent on CONFIG_NFS_V4_SECURITY_LABEL?
> >
> > Thanks for bringing this back up Stephen, it would be good to get this resolved.
>
> On second look, I realized that commit 243fea134633 ("NFSv4.2: fix
> listxattr to return selinux security label") was likely motivated by
> the same issue as commit 8b0ba61df5a1c44e2b3cf6 ("fs/xattr.c: fix
> simple_xattr_list to always include security.* xattrs"), i.e. the
> coreutils change that switched ls -Z from unconditionally calling
> getxattr("security.selinux") (via libselinux getfilecon(3)) to only
> doing so if listxattr() returns the "security.selinux" xattr name.
> Hence, we want the call to security_inode_listsecurity() to be
> unconditional, which favors option #2. My only residual question
> though is that commit 243fea134633 put the call _after_ fetching the
> user.* xattr names, whereas the nfs4_listxattr_nfs4_label() returns it
> _before_ any user.* xattrs are appended. I'd be inclined to move up
> the security_inode_listsecurity() call to replace the
> nfs4_listxattr_nfs4_label() call along with option #2.
I've made an attempt to unify the two security_inode_listsecurity()
hook calls in the nfs4 code into a single, unconditional call from
nfs4_listxattr(), which can be found here:
https://lore.kernel.org/selinux/20251203195728.8592-1-stephen.smalley.work@gmail.com/T/#u
If this is deemed acceptable by the NFS folks, then I can re-base this
patch on top of that one.
^ 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