* [PATCH v2 8/9] tpm-buf: Remove chip parameeter from tpm_buf_append_handle
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Remove chip parameter from tpm_buf_append_handle() in order to maintain
decoupled state with tpm-buf. This is mandatory change in order to re-use
the module in early boot code of Trenchboot, and the binding itself brings
no benefit. Use WARN like in other functions, as the error condition can
happen only as a net effect of a trivial programming mistake.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 6 ++----
drivers/char/tpm/tpm2-cmd.c | 2 +-
drivers/char/tpm/tpm2-sessions.c | 2 +-
include/linux/tpm.h | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 5526f548b4de..c2bf7556cb23 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -147,21 +147,19 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
/**
* tpm_buf_append_handle() - Add a handle
- * @chip: &tpm_chip instance
* @buf: &tpm_buf instance
* @handle: a TPM object handle
*
* Add a handle to the buffer, and increase the count tracking the number of
* handles in the command buffer. Works only for command buffers.
*/
-void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
+void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle)
{
if (buf->flags & TPM_BUF_INVALID)
return;
if (buf->flags & TPM_BUF_TPM2B) {
- buf->flags |= TPM_BUF_INVALID;
- dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+ WARN(1, "tpm-buf: invalid type: TPM2B\n");
return;
}
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index eef324e61308..4248e59265aa 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -190,7 +190,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
- tpm_buf_append_handle(chip, &buf, pcr_idx);
+ tpm_buf_append_handle(&buf, pcr_idx);
tpm_buf_append_auth(chip, &buf, NULL, 0);
}
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 13f019d1312a..bbc05f0997a8 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -232,7 +232,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
#endif
if (!tpm2_chip_auth(chip)) {
- tpm_buf_append_handle(chip, buf, handle);
+ tpm_buf_append_handle(buf, handle);
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5283f32781c4..b2d89df70c18 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -423,7 +423,7 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
-void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle);
+void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle);
/*
* Check if TPM device is in the firmware upgrade mode.
--
2.39.5
^ permalink raw reply related
* [PATCH v2 7/9] tpm-buf: check for corruption in tpm_buf_append_handle()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW into TPM_BUF_INVALID
flag because semantically they are identical.
Test and set TPM_BUF_INVALID in tpm_buf_append_handle() following the
pattern from other functions in tpm-buf.c.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 14 ++++++++------
include/linux/tpm.h | 8 +++-----
security/keys/trusted-keys/trusted_tpm2.c | 6 +++---
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index dc882fc9fa9e..5526f548b4de 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -104,13 +104,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
*/
void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
{
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
+ if (buf->flags & TPM_BUF_INVALID)
return;
if ((buf->length + new_length) > PAGE_SIZE) {
WARN(1, "tpm_buf: write overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
@@ -157,7 +156,11 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
*/
void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
{
+ if (buf->flags & TPM_BUF_INVALID)
+ return;
+
if (buf->flags & TPM_BUF_TPM2B) {
+ buf->flags |= TPM_BUF_INVALID;
dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
return;
}
@@ -177,14 +180,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
{
off_t next_offset;
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
+ if (buf->flags & TPM_BUF_INVALID)
return;
next_offset = *offset + count;
if (next_offset > buf->length) {
WARN(1, "tpm_buf: read out of boundary\n");
- buf->flags |= TPM_BUF_BOUNDARY_ERROR;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index e72e7657faa2..5283f32781c4 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -366,12 +366,10 @@ struct tpm_header {
} __packed;
enum tpm_buf_flags {
- /* the capacity exceeded: */
- TPM_BUF_OVERFLOW = BIT(0),
/* TPM2B format: */
- TPM_BUF_TPM2B = BIT(1),
- /* read out of boundary: */
- TPM_BUF_BOUNDARY_ERROR = BIT(2),
+ TPM_BUF_TPM2B = BIT(0),
+ /* The buffer is in invalid and unusable state: */
+ TPM_BUF_INVALID = BIT(1),
};
/*
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8e3b283a59b2..119d5152c0db 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -295,7 +295,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
/* creation PCR */
tpm_buf_append_u32(&buf, 0);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
@@ -308,7 +308,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
blob_len = tpm_buf_read_u32(&buf, &offset);
- if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
+ if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
goto out;
}
@@ -414,7 +414,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
tpm_buf_append(&buf, blob, blob_len);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
--
2.39.5
^ permalink raw reply related
* [PATCH v2 6/9] KEYS: trusted: Open code tpm2_buf_append()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Jonathan McDowell,
David Howells, Jarkko Sakkinen, Paul Moore, James Morris,
Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
tpm2_buf_append_auth() has only single call site and most of its parameters
are redundant. Open code it to the call site. Remove illegit FIXME comment
as there is no categorized bug and replace it with more sane comment about
implementation (i.e. "non-opionated inline comment").
Reviewed-by: Jonathan McDowell <noodles@earth.li>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- No changes.
---
security/keys/trusted-keys/trusted_tpm2.c | 51 ++++-------------------
1 file changed, 9 insertions(+), 42 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c414a7006d78..8e3b283a59b2 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -198,36 +198,6 @@ int tpm2_key_priv(void *context, size_t hdrlen,
return 0;
}
-/**
- * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
- *
- * @buf: an allocated tpm_buf instance
- * @session_handle: session handle
- * @nonce: the session nonce, may be NULL if not used
- * @nonce_len: the session nonce length, may be 0 if not used
- * @attributes: the session attributes
- * @hmac: the session HMAC or password, may be NULL if not used
- * @hmac_len: the session HMAC or password length, maybe 0 if not used
- */
-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
- const u8 *nonce, u16 nonce_len,
- u8 attributes,
- const u8 *hmac, u16 hmac_len)
-{
- tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
- tpm_buf_append_u32(buf, session_handle);
- tpm_buf_append_u16(buf, nonce_len);
-
- if (nonce && nonce_len)
- tpm_buf_append(buf, nonce, nonce_len);
-
- tpm_buf_append_u8(buf, attributes);
- tpm_buf_append_u16(buf, hmac_len);
-
- if (hmac && hmac_len)
- tpm_buf_append(buf, hmac, hmac_len);
-}
-
/**
* tpm2_seal_trusted() - seal the payload of a trusted key
*
@@ -507,19 +477,16 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
options->blobauth_len);
} else {
/*
- * FIXME: The policy session was generated outside the
- * kernel so we don't known the nonce and thus can't
- * calculate a HMAC on it. Therefore, the user can
- * only really use TPM2_PolicyPassword and we must
- * send down the plain text password, which could be
- * intercepted. We can still encrypt the returned
- * key, but that's small comfort since the interposer
- * could repeat our actions with the exfiltrated
- * password.
+ * The policy session is generated outside the kernel, and thus
+ * the password will end up being unencrypted on the bus, as
+ * HMAC nonce cannot be calculated for it.
*/
- tpm2_buf_append_auth(&buf, options->policyhandle,
- NULL /* nonce */, 0, 0,
- options->blobauth, options->blobauth_len);
+ tpm_buf_append_u32(&buf, 9 + options->blobauth_len);
+ tpm_buf_append_u32(&buf, options->policyhandle);
+ tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u8(&buf, 0);
+ tpm_buf_append_u16(&buf, options->blobauth_len);
+ tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
if (tpm2_chip_auth(chip)) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
} else {
--
2.39.5
^ permalink raw reply related
* [PATCH v2 5/9] tpm2-sessions: Umask tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Open code tpm_buf_append_hmac_session_opt() in order to unmask the code
paths in the call sites of tpm_buf_append_hmac_session().
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Uncorrupt the patch.
---
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 c182a07b70de..eef324e61308 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -257,9 +257,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);
tpm_buf_fill_hmac_session(chip, &buf);
err = tpm_transmit_cmd(chip, &buf,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 1fa02e18e688..e72e7657faa2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -532,29 +532,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 e165b117bbca..c414a7006d78 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -482,8 +482,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;
@@ -518,8 +520,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);
+ }
}
tpm_buf_fill_hmac_session(chip, &buf);
--
2.39.5
^ permalink raw reply related
* [PATCH v2 4/9] tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Roberto Sassu, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
In a previous bug fix, 'attributes' was added by mistake to
tpm_buf_append_auth(). Remove the parameter.
Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Uncorrupt the patch.
---
drivers/char/tpm/tpm2-cmd.c | 2 +-
drivers/char/tpm/tpm2-sessions.c | 5 ++---
include/linux/tpm.h | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e416cc8705e3..c182a07b70de 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -191,7 +191,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
- tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+ tpm_buf_append_auth(chip, &buf, NULL, 0);
}
tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..13f019d1312a 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -266,7 +266,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
- u8 attributes, u8 *passphrase, int passphrase_len)
+ u8 *passphrase, int passphrase_len)
{
/* offset tells us where the sessions area begins */
int offset = buf->handles * 4 + TPM_HEADER_SIZE;
@@ -327,8 +327,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
#endif
if (!tpm2_chip_auth(chip)) {
- tpm_buf_append_auth(chip, buf, attributes, passphrase,
- passphrase_len);
+ tpm_buf_append_auth(chip, buf, passphrase, passphrase_len);
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 51846317d662..1fa02e18e688 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -531,7 +531,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
u8 attributes, u8 *passphrase,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
- u8 attributes, u8 *passphrase, int passphraselen);
+ u8 *passphrase, int passphraselen);
static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
struct tpm_buf *buf,
u8 attributes,
--
2.39.5
^ permalink raw reply related
* [PATCH v2 3/9] KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, David Howells,
Jarkko Sakkinen, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Use tpm_ret_to_err() to transmute TPM return codes in trusted_tpm2.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- New patch split out from the fix.
---
security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 024be262702f..e165b117bbca 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -348,25 +348,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
}
blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
+ if (blob_len < 0)
+ rc = blob_len;
out:
tpm_buf_destroy(&sized);
tpm_buf_destroy(&buf);
- if (rc > 0) {
- if (tpm2_rc_value(rc) == TPM2_RC_HASH)
- rc = -EINVAL;
- else
- rc = -EPERM;
- }
- if (blob_len < 0)
- rc = blob_len;
- else
+ if (!rc)
payload->blob_len = blob_len;
out_put:
tpm_put_ops(chip);
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -468,10 +462,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
kfree(blob);
tpm_buf_destroy(&buf);
- if (rc > 0)
- rc = -EPERM;
-
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -534,8 +525,6 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
tpm_buf_fill_hmac_session(chip, &buf);
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
- if (rc > 0)
- rc = -EPERM;
if (!rc) {
data_len = be16_to_cpup(
@@ -568,7 +557,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
out:
tpm_buf_destroy(&buf);
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -600,6 +589,5 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
out:
tpm_put_ops(chip);
-
- return rc;
+ return tpm_ret_to_err(rc);
}
--
2.39.5
^ permalink raw reply related
* [PATCH v2 2/9] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Stefano Garzarella, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Using -EFAULT as the tpm_ret_to_err() fallback error code causes makes it
incompatible on how trusted keys transmute TPM return codes.
Change the fallback as -EPERM in order to gain compatibility with trusted
keys. In addition, map TPM_RC_HASH to -EINVAL in order to be compatible
with tpm2_seal_trusted() return values.
Fixes: 539fbab37881 ("tpm: Mask TPM RC in tpm2_start_auth_session()")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Split trusted_tpm2 change to a separate patch.
---
include/linux/tpm.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index fc7df87dfb9a..51846317d662 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -453,8 +453,10 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
return 0;
case TPM2_RC_SESSION_MEMORY:
return -ENOMEM;
+ case TPM2_RC_HASH:
+ return -EINVAL;
default:
- return -EFAULT;
+ return -EPERM;
}
}
--
2.39.5
^ permalink raw reply related
* [PATCH v2 1/9] tpm: cap PCR bank in tpm2_get_pcr_allocations()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Roberto Sassu,
Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
tpm2_get_pcr_allocation() does not cap any upper limit for the number of
banks received from external hardware device. This could lead into resource
over-consumption with a fauly TPM device.
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-chip.c | 13 +++++++++----
drivers/char/tpm/tpm.h | 1 -
drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
drivers/char/tpm/tpm2-cmd.c | 8 +++-----
include/linux/tpm.h | 18 ++++++++----------
5 files changed, 20 insertions(+), 45 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 687f6d8cd601..9a6538f76f50 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
static int tpm_get_pcr_allocation(struct tpm_chip *chip)
{
- int rc;
+ int rc = 0;
if (tpm_is_firmware_upgrade(chip))
return 0;
- rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
- tpm2_get_pcr_allocation(chip) :
- tpm1_get_pcr_allocation(chip);
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
+ chip->nr_allocated_banks = 1;
+ } else {
+ rc = tpm2_get_pcr_allocation(chip);
+ }
if (rc > 0)
return -ENODEV;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 57ef8589f5f5..769fa6b00c54 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
-int tpm1_get_pcr_allocation(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index cf64c7385105..5c49bdff33de 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
return rc;
}
-
-/**
- * tpm1_get_pcr_allocation() - initialize the allocated bank
- * @chip: TPM chip to use.
- *
- * The function initializes the SHA1 allocated bank to extend PCR
- *
- * Return:
- * * 0 on success,
- * * < 0 on error.
- */
-int tpm1_get_pcr_allocation(struct tpm_chip *chip)
-{
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks)
- return -ENOMEM;
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
- return 0;
-}
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 7d77f6fbc152..e416cc8705e3 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
nr_possible_banks = be32_to_cpup(
(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
-
- chip->allocated_banks = kcalloc(nr_possible_banks,
- sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
+ if (nr_possible_banks > TPM2_MAX_BANKS) {
+ pr_err("tpm:: unexpected large number of banks: %u > %u",
+ nr_possible_banks, TPM2_MAX_BANKS);
rc = -ENOMEM;
goto out;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 900c81a2bc41..fc7df87dfb9a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -27,7 +27,12 @@
#include <crypto/aes.h>
#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
-#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
+#define TPM_HEADER_SIZE 10
+
+#define TPM2_PLATFORM_PCR 24
+#define TPM2_PCR_SELECT_MIN 3
+#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
+#define TPM2_MAX_BANKS 4
struct tpm_chip;
struct trusted_key_payload;
@@ -69,7 +74,7 @@ enum tpm2_curves {
struct tpm_digest {
u16 alg_id;
- u8 digest[TPM_MAX_DIGEST_SIZE];
+ u8 digest[TPM2_MAX_DIGEST_SIZE];
} __packed;
struct tpm_bank_info {
@@ -190,7 +195,7 @@ struct tpm_chip {
unsigned int groups_cnt;
u32 nr_allocated_banks;
- struct tpm_bank_info *allocated_banks;
+ struct tpm_bank_info allocated_banks[TPM2_MAX_BANKS];
#ifdef CONFIG_ACPI
acpi_handle acpi_dev_handle;
char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -217,13 +222,6 @@ struct tpm_chip {
#endif
};
-#define TPM_HEADER_SIZE 10
-
-enum tpm2_const {
- TPM2_PLATFORM_PCR = 24,
- TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
-};
-
enum tpm2_timeouts {
TPM2_TIMEOUT_A = 750,
TPM2_TIMEOUT_B = 4000,
--
2.39.5
^ permalink raw reply related
* [PATCH v2 0/9] tpm: Decouple PCR extend from driver
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, David Howells,
Paul Moore, James Morris, Serge E. Hallyn,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
Decouple tpm2-sessions enough from implementation so that building for PCR
extend commands can be decoupled from rest of the implementation. This is
a mandatory for Trenchboot series, and including all these changes for
that series would over-complicate it.
This is first part of refactorizations for make grounds for Trenchboot,
and still aimed for 6.18. The second part includes robustness updates
for tpm-buf.
v2:
- While including fixes from v1, this patch set has a refocus in order to
do minimal changes to make code base more compatible Trenchboot.
Jarkko Sakkinen (9):
tpm: cap PCR bank in tpm2_get_pcr_allocations()
tpm: Use -EPERM as fallback error code in tpm_ret_to_err
KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
tpm2-sessions: Umask tpm_buf_append_hmac_session()
KEYS: trusted: Open code tpm2_buf_append()
tpm-buf: check for corruption in tpm_buf_append_handle()
tpm-buf: Remove chip parameeter from tpm_buf_append_handle
tpm-buf: Build PCR extend commands
drivers/char/tpm/tpm-buf.c | 85 +++++++++++++++++---
drivers/char/tpm/tpm-chip.c | 13 +++-
drivers/char/tpm/tpm.h | 1 -
drivers/char/tpm/tpm1-cmd.c | 40 ++--------
drivers/char/tpm/tpm2-cmd.c | 39 ++++++----
drivers/char/tpm/tpm2-sessions.c | 7 +-
include/linux/tpm.h | 61 +++++----------
include/linux/tpm_command.h | 5 +-
security/keys/trusted-keys/trusted_tpm2.c | 95 +++++++----------------
9 files changed, 170 insertions(+), 176 deletions(-)
--
2.39.5
^ permalink raw reply
* Re: [RFC PATCH v2] lsm,selinux: introduce LSM_ATTR_UNSHARE and wire it up for SELinux
From: Serge E. Hallyn @ 2025-09-29 3:35 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-security-module, selinux, paul, omosnace, john.johansen,
serge, casey
In-Reply-To: <20250918135904.9997-2-stephen.smalley.work@gmail.com>
On Thu, Sep 18, 2025 at 09:59:05AM -0400, Stephen Smalley wrote:
> RFC-only, will ultimately split the LSM-only changes to their own
> patch for submission. I have now tested this with the corresponding
> selinux userspace change that you can find at
> https://lore.kernel.org/selinux/20250918135118.9896-2-stephen.smalley.work@gmail.com/
> and also verified that my modified systemd-nspawn still works when
> starting containers with their own SELinux namespace.
>
> This defines a new LSM_ATTR_UNSHARE attribute for the
> lsm_set_self_attr(2) system call and wires it up for SELinux to invoke
> the underlying function for unsharing the SELinux namespace. As with
> the selinuxfs interface, this immediately unshares the SELinux
> namespace of the current process just like an unshare(2) system call
> would do for other namespaces. I have not yet explored the
> alternatives of deferring the unshare to the next unshare(2),
> clone(2), or execve(2) call and would want to first confirm that doing
> so does not introduce any issues in the kernel or make it harder to
> integrate with existing container runtimes.
Doing it immediately seems like the right thing to do. So that
the container runtime can keep the umount/remount of selinuxfs
with the unshare, instead of having to defer that until after
a later syscall.
> Differences between this syscall interface and the selinuxfs interface
> that need discussion before moving forward:
>
> 1. The syscall interface does not currently check any Linux capability
> or DAC permissions, whereas the selinuxfs interface can only be set by
> uid-0 or CAP_DAC_OVERRIDE processes. We need to decide what if any
> capability or DAC check should apply to this syscall interface and if
> any, add the checks to either the LSM framework code or to the SELinux
> hook function.
I think this should be done by the SELinux hook. And I suspect you
do want to require those privs, but I could be wrong.
> Pros: Checking a capability or DAC permissions prevents misuse of this
> interface by unprivileged processes, particularly on systems with
> policies that do not yet define any of the new SELinux permissions
> introduced for controlling this operation. This is a potential concern
> on Linux distributions that do not tightly coordinate kernel updates
> with policy updates (or where users may choose to deploy upstream
> kernels on their own), but not on Android.
Hm, that's an interesting problem.
> Cons: Checking a capability or DAC permissions requires any process
> that uses this facility to have the corresponding capability or
> permissions, which might otherwise be unnecessary and create
> additional risks. This is less likely if we use a capability already
> required by container runtimes and similar components that might
> leverage this facility for unsharing SELinux namespaces.
>
> 2. The syscall interface checks a new SELinux unshare_selinuxns
> permission in the process2 class between the task SID and itself,
> similar to other checks for setting process attributes. This means
> that:
> allow domain self:process2 *; -or-
> allow domain self:process2 ~anything-other-than-unshare_selinuxns; -or-
> allow domain self:process2 unshare_selinuxns;
> would allow a process to unshare its SELinux namespace.
>
> The selinuxfs interface checks a new unshare permission in the
> security class between the task SID and the security initial SID,
> likewise similar to other checks for setting selinuxfs attributes.
> This means that:
> allow domain security_t:security *; -or-
> allow domain security_t:security ~anything-other-than-unshare; -or-
> allow domain security_t:security unshare;
> would allow a process to unshare its SELinux namespace.
>
> Technically, the selinuxfs interface also currently requires open and
> write access to the selinuxfs node; hence:
> allow domain security_t:file { open write };
> is also required for the selinuxfs interface.
>
> We need to decide what we want the SELinux check(s) to be for the
> syscall and whether it should be more like the former (process
> attributes) or more like the latter (security policy settings). Note
> that the permission name itself is unimportant here and only differs
> because it seemed less evident in the process2 class that we are
> talking about a SELinux namespace otherwise.
>
> Regardless, either form of allow rule can be prohibited in policies
> via neverallow rules on systems that enforce their usage
> (e.g. Android, not necessarily on Linux distributions).
>
> 3. The selinuxfs interface currently offers more functionality than I
> have implemented here for the sycall interface, including:
>
> a) the ability to read the selinuxfs node to see if your namespace has
> been unshared, which should be easily implementable via
> lsm_get_self_attr(2). However, questions remain as to when that
> should return 1 versus 0 (currently returns 1 whenever your namespace
> is NOT the initial SELinux namespace, useful for the testsuite to
> detect it is in a child, but could instead be reset to 0 by a
> subsequent policy load to indicate completion of the setup of the
> namespace, thus hiding from child processes that they are in a child
> namespace once its policy has been loaded).
maybe 'unshare' means that an unshare is in progress, and add an
'unshared' which is incremented on every unshare (and never
decremented) for use by the testsuite?
> b) the abilities to get and set the maximum number of SELinux
> namespaces (via a /sys/fs/selinux/maxns node) and to get and set the
> maximum depth for SELinux namespaces (via a /sys/fs/selinux/maxnsdepth
> node). These could be left in selinuxfs or migrated to some other LSM
> management APIs since they are global in scope, not per-process
> attributes.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 fixes a typo (PROCESS->PROCESS2) and is now tested.
>
> include/uapi/linux/lsm.h | 1 +
> security/selinux/hooks.c | 8 ++++++++
> security/selinux/include/classmap.h | 4 +++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index 938593dfd5da..fb1b4a8aa639 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -83,6 +83,7 @@ struct lsm_ctx {
> #define LSM_ATTR_KEYCREATE 103
> #define LSM_ATTR_PREV 104
> #define LSM_ATTR_SOCKCREATE 105
> +#define LSM_ATTR_UNSHARE 106
>
> /*
> * LSM_FLAG_XXX definitions identify special handling instructions
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f48483383d6e..1e34a16b7954 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6816,6 +6816,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETCURRENT, NULL);
> break;
> + case LSM_ATTR_UNSHARE:
> + error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS2,
> + PROCESS2__UNSHARE_SELINUXNS, NULL);
> + break;
> default:
> error = -EOPNOTSUPP;
> break;
> @@ -6927,6 +6931,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> }
>
> tsec->sid = sid;
> + } else if (attr == LSM_ATTR_UNSHARE) {
> + error = selinux_state_create(new);
> + if (error)
> + goto abort_change;
> } else {
> error = -EINVAL;
> goto abort_change;
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index be52ebb6b94a..07fe316308cd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -60,7 +60,9 @@ const struct security_class_mapping secclass_map[] = {
> "siginh", "setrlimit", "rlimitinh", "dyntransition",
> "setcurrent", "execmem", "execstack", "execheap",
> "setkeycreate", "setsockcreate", "getrlimit", NULL } },
> - { "process2", { "nnp_transition", "nosuid_transition", NULL } },
> + { "process2",
> + { "nnp_transition", "nosuid_transition", "unshare_selinuxns",
> + NULL } },
> { "system",
> { "ipc_info", "syslog_read", "syslog_mod", "syslog_console",
> "module_request", "module_load", "firmware_load",
> --
> 2.50.1
^ permalink raw reply
* [PATCH] Documentation/landlock: Make docs in cred.h and domain.h visible.
From: Tingmao Wang @ 2025-09-28 23:49 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, linux-security-module
Currently even though the structures in these files have documentation,
they are not shown in the "Landlock LSM: kernel documentation" page.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Documentation/security/landlock.rst | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst
index e0fc54aff09e..5066f8c3091e 100644
--- a/Documentation/security/landlock.rst
+++ b/Documentation/security/landlock.rst
@@ -110,6 +110,12 @@ Filesystem
.. kernel-doc:: security/landlock/fs.h
:identifiers:
+Process credential
+------------------
+
+.. kernel-doc:: security/landlock/cred.h
+ :identifiers:
+
Ruleset and domain
------------------
@@ -128,6 +134,9 @@ makes the reasoning much easier and helps avoid pitfalls.
.. kernel-doc:: security/landlock/ruleset.h
:identifiers:
+.. kernel-doc:: security/landlock/domain.h
+ :identifiers:
+
Additional documentation
========================
base-commit: f83ec76bf285bea5727f478a68b894f5543ca76e
--
2.51.0
^ permalink raw reply related
* Re: [PATCH RFC 0/4] landlock: add LANDLOCK_SCOPE_MEMFD_EXEC execution
From: Abhinav Saxena @ 2025-09-28 23:37 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Paul Moore, James Morris, Serge E. Hallyn,
Shuah Khan, Nathan Chancellor, Nick Desaulniers, Bill Wendling,
Justin Stitt, linux-security-module, linux-kernel,
linux-kselftest, llvm, Daniel Verkamp, Jeff Xu,
Thiébaud Weksteen, Stephen Smalley
In-Reply-To: <20250918.io7too8ain7A@digikod.net>
[-- Attachment #1: Type: text/plain, Size: 8093 bytes --]
Thanks for the detailed reply Mickaël!
Mickaël Salaün <mic@digikod.net> writes:
> Thanks for this patch series Abhinav! The code looks good overall, but
> we should clarify the design. Sorry for the delayed response, it is on
> my radar now.
>
> CCing Jeff and Daniel
>
> On Sat, Jul 19, 2025 at 05:13:10AM -0600, Abhinav Saxena wrote:
>> This patch series introduces LANDLOCK_SCOPE_MEMFD_EXEC, a new Landlock
>> scoping mechanism that restricts execution of anonymous memory file
>> descriptors (memfd) created via memfd_create(2). This addresses security
>> gaps where processes can bypass W^X policies and execute arbitrary code
>> through anonymous memory objects.
>>
>> Fixes: <https://github.com/landlock-lsm/linux/issues/37>
>>
>> SECURITY PROBLEM
>> `=============='
>>
>> Current Landlock filesystem restrictions do not cover memfd objects,
>> allowing processes to:
>>
>> 1. Read-to-execute bypass: Create writable memfd, inject code,
>> then execute via mmap(PROT_EXEC) or direct execve()
>> 2. Anonymous execution: Execute code without touching the filesystem via
>> execve(“/proc/self/fd/N”) where N is a memfd descriptor
>
>> 3. Cross-domain access violations: Pass memfd between processes to
>> bypass domain restrictions
>
> Landlock only restricts access at open time, which is a useful property.
> This enables to create more restricted sandboxes but still get access to
> outside resources via trusted processes. If the process passing the FDs
> is not trusted, the sandboxed process could just ask to execute
> arbitrary code outside the sandbox anyway.
>
> However, the Landlock scopes are designed to block IPC from within a
> sandbox to outside the sandbox. We could have a new scope to forbid a
> sandbox process to receive or inherit file descriptors, but that would
> be a different and generic feature. For compatibility reasons, this
> might not be easy to implement and I think there are more important
> features to implement before that.
>
> Thinking more about it, restricting memfd should not be a “scoped” flag
> because the semantic is not the same, but we should have a new ruleset
> property instead, something like “ruleset.denied” with a related
> LANDLOCK_DENY_EXECUTE_MEMFD flag. This flag will only have an impact on
> newly created memfd from a sandboxed process with this restriction at
> creation time. This could be implemented with hook_file_alloc_security()
> by checking if the file is indeed a memfd and checking inode->i_mode for
> executability bits (which would imply MFD_NOEXEC_SEAL).
>
Thanks for the clarification! So if I understood correctly we are
proposing adding a `denied` field to the `landlock_ruleset_attr` struct
struct landlock_ruleset_attr {
__u64 handled_access_fs;
__u64 handled_access_net;
__u64 scoped;
__u64 denied; /* New field */
};
which allows memfd_create() to be allowed by default unless
LANDLOCK_DENY_EXECUTE_MEMFD bit is set. Also it seems Thiébaud
Weksteen’s patch[1] will land, and maybe we can use
security_inode_init_security_anon instead? What do you think?
Apologies for my ignorance, do we have to wait till his patch has
landed into Linus’s tree?
>>
>> These scenarios can occur in sandboxed environments where filesystem
>> access is restricted but memfd creation remains possible.
>>
>> IMPLEMENTATION
>> `============'
>>
>> The implementation adds hierarchical execution control through domain
>> scoping:
>>
>> Core Components:
>> - is_memfd_file(): Reliable memfd detection via “memfd:” dentry prefix
>> - domain_is_scoped(): Cross-domain hierarchy checking (moved to domain.c)
>> - LSM hooks: mmap_file, file_mprotect, bprm_creds_for_exec
>> - Creation-time restrictions: hook_file_alloc_security
>>
>> Security Matrix:
>> Execution decisions follow domain hierarchy rules preventing both
>> same-domain bypass attempts and cross-domain access violations while
>> preserving legitimate hierarchical access patterns.
>>
>> Domain Hierarchy with LANDLOCK_SCOPE_MEMFD_EXEC:
>> `============================================='
>>
>> Root (no domain) - No restrictions
>> |
>> +– Domain A [SCOPE_MEMFD_EXEC] Layer 1
>> | +– memfd_A (tagged with Domain A as creator)
>> | |
>> | +– Domain A1 (child) [NO SCOPE] Layer 2
>> | | +– Inherits Layer 1 restrictions from parent
>> | | +– memfd_A1 (can create, inherits restrictions)
>> | | +– Domain A1a [SCOPE_MEMFD_EXEC] Layer 3
>> | | +– memfd_A1a (tagged with Domain A1a)
>> | |
>> | +– Domain A2 (child) [SCOPE_MEMFD_EXEC] Layer 2
>> | +– memfd_A2 (tagged with Domain A2 as creator)
>> | +– CANNOT access memfd_A1 (different subtree)
>> |
>> +– Domain B [SCOPE_MEMFD_EXEC] Layer 1
>> +– memfd_B (tagged with Domain B as creator)
>> +– CANNOT access ANY memfd from Domain A subtree
>>
>> Execution Decision Matrix:
>> `======================'
>> Executor-> | A | A1 | A1a | A2 | B | Root
>> Creator | | | | | |
>> ————|—–|—-|—–|—-|—-|—–
>> Domain A | X | X | X | X | X | Y
>> Domain A1 | Y | X | X | X | X | Y
>> Domain A1a | Y | Y | X | X | X | Y
>> Domain A2 | Y | X | X | X | X | Y
>> Domain B | X | X | X | X | X | Y
>> Root | Y | Y | Y | Y | Y | Y
>>
>> Legend: Y = Execution allowed, X = Execution denied
>
> Because checks should not be related to scopes, this will be much
> simpler.
>
>>
>> Scenarios Covered:
>> - Direct mmap(PROT_EXEC) on memfd files
>> - Two-stage mmap(PROT_READ) + mprotect(PROT_EXEC) bypass attempts
>> - execve("/proc/self/fd/N") anonymous execution
>> - execveat() and fexecve() file descriptor execution
>> - Cross-process memfd inheritance and IPC passing
>>
>> TESTING
>> `====='
>>
>> All patches have been validated with:
>> - scripts/checkpatch.pl –strict (clean)
>> - Selftests covering same-domain restrictions, cross-domain
>> hierarchy enforcement, and regular file isolation
>> - KUnit tests for memfd detection edge cases
>
> Thanks for all these tests!
>
>>
>> DISCLAIMER
>> `========'
>>
>> My understanding of Landlock scoping semantics may be limited, but this
>> implementation reflects my current understanding based on available
>> documentation and code analysis. I welcome feedback and corrections
>> regarding the scoping logic and domain hierarchy enforcement.
>>
>> Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
>> —
>> Abhinav Saxena (4):
>> landlock: add LANDLOCK_SCOPE_MEMFD_EXEC scope
>> landlock: implement memfd detection
>> landlock: add memfd exec LSM hooks and scoping
>> selftests/landlock: add memfd execution tests
>>
>> include/uapi/linux/landlock.h | 5 +
>> security/landlock/.kunitconfig | 1 +
>> security/landlock/audit.c | 4 +
>> security/landlock/audit.h | 1 +
>> security/landlock/cred.c | 14 -
>> security/landlock/domain.c | 67 ++++
>> security/landlock/domain.h | 4 +
>> security/landlock/fs.c | 405 ++++++++++++++++++++-
>> security/landlock/limits.h | 2 +-
>> security/landlock/task.c | 67 —-
>> …/selftests/landlock/scoped_memfd_exec_test.c | 325 +++++++++++++++++
>> 11 files changed, 812 insertions(+), 83 deletions(-)
>> —
>> base-commit: 5b74b2eff1eeefe43584e5b7b348c8cd3b723d38
>> change-id: 20250716-memfd-exec-ac0d582018c3
>>
>> Best regards,
>> –
>> Abhinav Saxena <xandfury@gmail.com>
>>
>>
Best,
Abhinav
[1] - <https://lore.kernel.org/all/20250918020434.1612137-1-tweek@google.com/>
^ permalink raw reply
* Re: [PATCH v3 01/14] ACPI: APEI: Remove redundant rcu_read_lock/unlock() in spin_lock
From: Rafael J. Wysocki @ 2025-09-28 10:33 UTC (permalink / raw)
To: Hanjun Guo, pengdonglin
Cc: tj, tony.luck, jani.nikula, ap420073, jv, freude, bcrl, trondmy,
longman, kees, bigeasy, hdanton, paulmck, linux-kernel,
linux-rt-devel, linux-nfs, linux-aio, linux-fsdevel,
linux-security-module, netdev, intel-gfx, linux-wireless,
linux-acpi, linux-s390, cgroups, Rafael J. Wysocki, pengdonglin
In-Reply-To: <03ad08d9-4510-19fb-bbad-652159308119@huawei.com>
On Sat, Sep 27, 2025 at 5:22 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> On 2025/9/16 12:47, pengdonglin wrote:
> > From: pengdonglin <pengdonglin@xiaomi.com>
> >
> > Since commit a8bb74acd8efe ("rcu: Consolidate RCU-sched update-side function definitions")
> > there is no difference between rcu_read_lock(), rcu_read_lock_bh() and
> > rcu_read_lock_sched() in terms of RCU read section and the relevant grace
> > period. That means that spin_lock(), which implies rcu_read_lock_sched(),
> > also implies rcu_read_lock().
> >
> > There is no need no explicitly start a RCU read section if one has already
> > been started implicitly by spin_lock().
> >
> > Simplify the code and remove the inner rcu_read_lock() invocation.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Hanjun Guo <guohanjun@huawei.com>
> > Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
> > Signed-off-by: pengdonglin <dolinux.peng@gmail.com>
> > ---
> > drivers/acpi/apei/ghes.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index a0d54993edb3..97ee19f2cae0 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -1207,12 +1207,10 @@ static int ghes_notify_hed(struct notifier_block *this, unsigned long event,
> > int ret = NOTIFY_DONE;
> >
> > spin_lock_irqsave(&ghes_notify_lock_irq, flags);
> > - rcu_read_lock();
> > list_for_each_entry_rcu(ghes, &ghes_hed, list) {
> > if (!ghes_proc(ghes))
> > ret = NOTIFY_OK;
> > }
> > - rcu_read_unlock();
> > spin_unlock_irqrestore(&ghes_notify_lock_irq, flags);
> >
> > return ret;
>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Applied as 6.18 material, thanks!
^ permalink raw reply
* [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-09-28 3:03 UTC (permalink / raw)
To: linux-integrity
Cc: Dmitry Torokhov, Karel Srot, Mimi Zohar, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
Currently, for any IMA policy that requires appraisal for kernel modules
e.g. ima_policy=secure_boot, PowerPC architecture specific policy,
booting will fail because IMA will reject a kernel module which will
be decompressed in the kernel space and then have its signature
verified.
This happens because when in-kernel module decompression
(CONFIG_MODULE_DECOMPRESS) is enabled, kmod will use finit_module
syscall instead of init_module to load a module. And IMA mandates IMA
xattr verification for finit_module unless appraise_type=imasig|modsig
is specified in the rule. However currently initramfs doesn't support
xattr. And IMA rule "func=MODULE_CHECK appraise_type=imasig|modsig"
doesn't work either because IMA will treat to-be-decompressed kernel
module as not having module signature as it can't decompress kernel
module to check if signature exists.
So fall back to default kernel module signature verification when we have
no way to verify IMA xattr.
Reported-by: Karel Srot <ksrot@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
Another approach will be to make IMA decompress the kernel module to
check the signature. This requires refactoring kernel module code to
make the in-kernel module decompressing feature modular and seemingly
more efforts are needed. A second disadvantage is it feels
counter-intuitive to verify the same kernel module signature twice. And
we still need to make ima_policy=secure_boot allow verifying appended
module signature.
Anyways, I'm open to suggestions and can try the latter approach if
there are some benefits I'm not aware of or a better approach.
security/integrity/ima/ima_appraise.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f435eff4667f..fcc75dd1486f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -502,9 +502,10 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
int rc = xattr_len;
bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig;
+ bool enforce_module_sig = iint->flags & IMA_DIGSIG_REQUIRED && func == MODULE_CHECK;
- /* If not appraising a modsig, we need an xattr. */
- if (!(inode->i_opflags & IOP_XATTR) && !try_modsig)
+ /* If not appraising a modsig or using default module verification, we need an xattr. */
+ if (!(inode->i_opflags & IOP_XATTR) && !try_modsig && !enforce_module_sig)
return INTEGRITY_UNKNOWN;
/*
@@ -517,8 +518,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
if (is_bprm_creds_for_exec(func, file))
audit_msgno = AUDIT_INTEGRITY_USERSPACE;
- /* If reading the xattr failed and there's no modsig, error out. */
- if (rc <= 0 && !try_modsig) {
+ /* If reading the xattr failed and there's no modsig or module verification, error out. */
+ if (rc <= 0 && !try_modsig && !enforce_module_sig) {
if (rc && rc != -ENODATA)
goto out;
@@ -549,8 +550,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
case INTEGRITY_UNKNOWN:
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
- /* It's fine not to have xattrs when using a modsig. */
- if (try_modsig)
+ /* Fine to not have xattrs when using a modsig or default module verification. */
+ if (try_modsig || enforce_module_sig)
break;
fallthrough;
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
@@ -580,6 +581,18 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
rc == -ENOKEY))
rc = modsig_verify(func, modsig, &status, &cause);
+ /* Fall back to default kernel module signature verification */
+ if (rc && enforce_module_sig) {
+ rc = 0;
+ set_module_sig_enforced();
+ /* CONFIG_MODULE_SIG may be disabled */
+ if (is_module_sig_enforced()) {
+ rc = 0;
+ status = INTEGRITY_PASS;
+ pr_debug("Fall back to default kernel module verification for %s\n", filename);
+ }
+ }
+
out:
/*
* File signatures on some filesystems can not be properly verified.
base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
--
2.51.0
^ permalink raw reply related
* Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules
From: Tingmao Wang @ 2025-09-27 23:12 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Jan Kara, linux-security-module
In-Reply-To: <20250927.ivat2tho0Uom@digikod.net>
On 9/27/25 20:00, Mickaël Salaün wrote:
> On Sat, Sep 27, 2025 at 04:43:50PM +0100, Tingmao Wang wrote:
>> [..]
>>
>> Also, do we want to consider calling this something else instead, like
>> "suppress_audit"?
>
> Quiet is simpler (similar to the LANDLOCK_RESTRICT_SELF_LOG_* flags) and
> if we get other ways to log actions this will also be used. For the
> supervisor case, that would be useful to not forward a request to the
> supervisor. The *_LOG_* flags could be used the same way too (even if
> "LOG" may be a subset of the supervisor capabilities). Do you think
> that would be OK? Dedicated flags would be more flexible but also a bit
> more complex. Is it worth it? In any case, the semantic and need
> should be quite similar.
I don't think we need a dedicated flag, I was just wondering if "QUIET" is
the right name, but I guess I don't have a better suggestion either. On
second thought SUPPRESS_AUDIT would no longer be accurate if we later use
it to control supervisor forwarding (it would be doing more than just
suppressing audit logs).
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Eric Biggers @ 2025-09-27 23:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, bpf, LSM List, Blaise Boscaccy, Paul Moore,
K. Y. Srinivasan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
In-Reply-To: <CAADnVQ+t6JJ9YgH_xgicbzvvP2WvEJWxi+hioQtFKrR6BLTsCg@mail.gmail.com>
On Sat, Sep 27, 2025 at 11:33:12PM +0100, Alexei Starovoitov wrote:
> On Sat, Sep 27, 2025 at 10:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> > > Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> > > used for the loader generation code to embed the metadata hash in the
> > > loader program and also by the bpf_map__make_exclusive API to calculate
> > > the hash of the program the map is exclusive to.
> > >
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> >
> > Nacked-by: Eric Biggers <ebiggers@kernel.org>
> >
> > No more users of AF_ALG, please. It's a huge mistake and has been
> > incredibly problematic over the years.
>
> Lol. True, but good luck with that. AF_ALG is uapi and it will be removed
> only when the last user retires many years from now.
Many Linux systems never enabled AF_ALG in the first place, and those
that have it enabled often only have a few users of it or even none at
all. Sure, AF_ALG support will remain in-tree for a very long time or
even forever. But many systems can keep it disabled, or can disable it,
if new users are not introduced and existing users continue to be fixed.
Let's do the right thing here, instead of making the situation even
worse and also adding undocumented kconfig dependencies to libbpf.
> > If you don't want to depend on a library, then just include some basic
> > SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
> > https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
> > I'd even be glad to write the patch for you, if you want.
>
> Yes. Please. If you can craft sha256 without external dependencies
> we can certainly use it.
> Certainly agree that it would be better than AF_ALG.
Sure, I'll do that.
- Eric
^ permalink raw reply
* Re: [PATCH v2 0/7] fs/9p: Reuse inode based on path (in addition to qid)
From: Tingmao Wang @ 2025-09-27 22:53 UTC (permalink / raw)
To: Mickaël Salaün, Greg Kurz
Cc: Christian Schoenebeck, Dominique Martinet, Eric Van Hensbergen,
Latchesar Ionkov, v9fs, Günther Noack, linux-security-module,
Jan Kara, Amir Goldstein, Matthew Bobrowski, Al Viro,
Christian Brauner, linux-fsdevel, qemu-devel
In-Reply-To: <20250927.ahGhiiy0koo0@digikod.net>
On 9/27/25 19:27, Mickaël Salaün wrote:
> Adding Greg Kurz too.
>
> On Sun, Sep 21, 2025 at 05:24:49PM +0100, Tingmao Wang wrote:
>> On 9/17/25 16:00, Mickaël Salaün wrote:
>>> [...]
>>
>> Alternatively if we believe this to be a QEMU issue, maybe
>> Landlock don't need to work around it and should just hold fids (and use
>> QIDs to key the rules) anyway despite server quirks like these. This can
>> perhaps then be fixed in QEMU?
>
> Yes, I think it would make sense for Landlock to open and keep open a
> fid (and hopefully the related remote file). However, the v9fs umount
> should be handled gracefully the same way Landlock tag inodes are
> handled. This should come with a QEMU patch to fix the consistency
> issue.
>
>>
>> (I guess the fact that QEMU is doing path tracking in the first place does
>> gives more precedent for justifying doing path tracking in v9fs as well,
>> but maybe that's the wrong way to think about it)
>
> Anyway, if QEMU does it, wouldn't it be the same for Landlock to just
> rely on fid?
The fid can't be relied on because it's just a handle. The client can
open multiple fids pointing to the same file (and in fact this is what
v9fs does - new fid for each open())
> If QEMU uses FD+O_PATH, then Landlock would work even for
> server-moved files.
(With this new approach, Landlock would have to key the rules based on
qid, but it also needs to hold an open fid to prevent that qid from being
reused (due to ext4 inode number reuse, etc))
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Alexei Starovoitov @ 2025-09-27 22:33 UTC (permalink / raw)
To: Eric Biggers
Cc: KP Singh, bpf, LSM List, Blaise Boscaccy, Paul Moore,
K. Y. Srinivasan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
In-Reply-To: <20250927210345.GE9798@quark>
On Sat, Sep 27, 2025 at 10:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> > Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> > used for the loader generation code to embed the metadata hash in the
> > loader program and also by the bpf_map__make_exclusive API to calculate
> > the hash of the program the map is exclusive to.
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
>
> Nacked-by: Eric Biggers <ebiggers@kernel.org>
>
> No more users of AF_ALG, please. It's a huge mistake and has been
> incredibly problematic over the years.
Lol. True, but good luck with that. AF_ALG is uapi and it will be removed
only when the last user retires many years from now.
> If you don't want to depend on a library, then just include some basic
> SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
> https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
> I'd even be glad to write the patch for you, if you want.
Yes. Please. If you can craft sha256 without external dependencies
we can certainly use it.
Certainly agree that it would be better than AF_ALG.
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Eric Biggers @ 2025-09-27 21:03 UTC (permalink / raw)
To: KP Singh
Cc: bpf, linux-security-module, bboscaccy, paul, kys, ast, daniel,
andrii
In-Reply-To: <20250921133133.82062-4-kpsingh@kernel.org>
On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> used for the loader generation code to embed the metadata hash in the
> loader program and also by the bpf_map__make_exclusive API to calculate
> the hash of the program the map is exclusive to.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
Nacked-by: Eric Biggers <ebiggers@kernel.org>
No more users of AF_ALG, please. It's a huge mistake and has been
incredibly problematic over the years.
If you don't want to depend on a library, then just include some basic
SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
I'd even be glad to write the patch for you, if you want.
- Eric
^ permalink raw reply
* [syzbot] Monthly lsm report (Sep 2025)
From: syzbot @ 2025-09-27 20:43 UTC (permalink / raw)
To: linux-kernel, linux-security-module, syzkaller-bugs
Hello lsm maintainers/developers,
This is a 31-day syzbot report for the lsm subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/lsm
During the period, 0 new issues were detected and 0 were fixed.
In total, 6 issues are still open and 41 have already been fixed.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 45 No possible deadlock in process_measurement (5)
https://syzkaller.appspot.com/bug?extid=6529afa25091aee8536c
<2> 43 Yes INFO: task hung in process_measurement (3)
https://syzkaller.appspot.com/bug?extid=cb9e66807bcb882cd0c5
<3> 13 Yes INFO: task hung in ima_file_free (4)
https://syzkaller.appspot.com/bug?extid=8036326eebe7d0140944
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH v2 0/7] fs/9p: Reuse inode based on path (in addition to qid)
From: Mickaël Salaün @ 2025-09-27 18:27 UTC (permalink / raw)
To: Tingmao Wang, Greg Kurz
Cc: Christian Schoenebeck, Dominique Martinet, Eric Van Hensbergen,
Latchesar Ionkov, v9fs, Günther Noack, linux-security-module,
Jan Kara, Amir Goldstein, Matthew Bobrowski, Al Viro,
Christian Brauner, linux-fsdevel, qemu-devel
In-Reply-To: <f1228978-dac0-4d1a-a820-5ac9562675d0@maowtm.org>
Adding Greg Kurz too.
On Sun, Sep 21, 2025 at 05:24:49PM +0100, Tingmao Wang wrote:
> On 9/17/25 16:00, Mickaël Salaün wrote:
> > On Wed, Sep 17, 2025 at 11:52:35AM +0200, Christian Schoenebeck wrote:
> >> On Wednesday, September 17, 2025 1:59:21 AM CEST Tingmao Wang wrote:
> >>> On 9/16/25 20:22, Christian Schoenebeck wrote:
> >>>> On Tuesday, September 16, 2025 4:01:40 PM CEST Tingmao Wang wrote:
> >> [...]
> >>>> I see that you are proposing an option for your proposed qid based
> >>>> re-using of dentries. I don't think it should be on by default though,
> >>>> considering what we already discussed (e.g. inodes recycled by ext4, but
> >>>> also not all 9p servers handling inode collisions).
> >>>
> >>> Just to be clear, this approach (Landlock holding a fid reference, then
> >>> using the qid as a key to search for rules when a Landlocked process
> >>> accesses the previously remembered file, possibly after the file has been
> >>> moved on the server) would only be in Landlock, and would only affect
> >>> Landlock, not 9pfs (so not sure what you meant by "re-using of dentries").
> >>>
> >>> The idea behind holding a fid reference within Landlock is that, because
> >>> we have the file open, the inode would not get recycled in ext4, and thus
> >>> no other file will reuse the qid, until we close that reference (when the
> >>> Landlock domain terminates, or when the 9p filesystem is unmounted)
> >>
> >> So far I only had a glimpse on your kernel patches and had the impression that
> >> they are changing behaviour for all users, since you are touching dentry
> >> lookup.
> >
> > I think we should not hold dentries because:
> > - they reference other dentries (i.e. a file hierarchy),
> > - they block umount and I'm convinced the VFS (and users) are not going
> > to like long-lived dentries,
> > - Landlock and inotify don't need dentries, just inodes.
> >
> > I'm wondering why fid are referenced by dentries instead of inodes.
> >
> > The need for Landlock is to be able to match an inode with a previously
> > seen one. Not all LSM hooks (nor VFS internals) always have access to
> > dentries, but they do have access to inodes.
> >
> >>
> >>>> For all open FIDs QEMU retains a descriptor to the file/directory.
> >>>>
> >>>> Which 9p message do you see sent to server, Trename or Trenameat?
> >>>>
> >>>> Does this always happen to you or just sometimes, i.e. under heavy load?
> >>>
> >>> Always happen, see log: (no Trename since the rename is done on the host)
> >> [...]
> >>> Somehow if I rename in the guest, it all works, even though it's using the
> >>> same fid 2 (and it didn't ask QEMU to walk the new path)
> >>
> >> Got it. Even though QEMU *should* hold a file descriptor (or a DIR* stream,
> >
> > It's reasonable to assume that QEMU and other should hold opened fid In
> > practice, this might not always be the case, but let's move on and
> > consider that a 9p server bug.
> >
> > Landlock and fanotify need some guarantees on opened files, and we
> > cannot consider every server bug. For Landlock, inode may get an
> > "ephemeral tag" (with the Landlock object mechanism) to match previously
> > seen inodes. In a perfect world, Landlock could keep a reference on 9p
> > inodes (as for other filesystems) and these inodes would always match
> > the same file. In practice this is not the case, but the 9p client
> > requirements and the Landlock requirements are not exactly the same.
> >
> > A 9p client (the kernel) wants to safely deal with duplicated qid, which
> > should not happen but still happen in practice as explained before.
> > On the other side, Landlock wants to not deny access to allowed files
> > (currently identified by their inodes), but I think it would be
> > reasonable to allow access theoretically denied (i.e. not allowed to be
> > precise, because of the denied by default mechanism) files because of a
> > 9p server bug mishandling qid (e.g. mapping them to recycled ext4
> > inodes).
> >
> > All that to say that it looks reasonable for Landlock to trust the
> > filesystem, and by that I mean all its dependencies, including the 9p
> > server, to not have bugs.
> >
> > Another advantage to rely on qid and server-side opened files is that we
> > get (in theory) the same semantic as when Landlock is used with local
> > filesystems (e.g. files moved on the server should still be correctly
> > identified by Landlock on the client).
> >
> >> which should imply a file descriptor), there is still a path string stored at
> >> V9fsFidState and that path being processed at some places, probably because
> >> there are path based and FID based variants (e.g Trename vs. Trenameat). Maybe
> >> that clashes somewhere, not sure. So I fear you would need to debug this.
> >
> > Good to know that it is not a legitimate behavior for a 9p client.
>
> So I did some quick debugging and realized that I had a wrong
> understanding of how fids relates to opened files on the host, under QEMU.
> It turns out that in QEMU's 9p server implementation, a fid does not
> actually correspond to any opened file descriptors - it merely represents
> a (string-based) path that QEMU stores internally. It only opens the
> actual file if the client actually does an T(l)open, which is in fact
> separate from acquiring the fid with T(l)walk. The reason why renaming
> file/dirs from the client doesn't break those fids is because QEMU will
> actually fix those paths when a rename request is processed - c.f.
> v9fs_fix_fid_paths [1].
>
> It turns out that even if a guest process opens the file with O_PATH, that
> file descriptor does not cause an actual Topen, and therefore QEMU does
> not open the file on the host, and later on reopening that fd with another
> mode (via e.g. open("/proc/self/fd/...", O_RDONLY)) will fail if the file
> has moved on the host without QEMU's knowledge. Also, openat will fail if
> provided with a dir fd that "points" to a moved directory, regardless of
> whether the fd is opened with O_PATH or not, since path walk in QEMU is
> completely string-based and does not actually issue openat on the host fs
> [2].
>
> I'm not sure if this was is intentional in QEMU - it would seem to me that
> a fid should translate to a fd (maybe opened with just O_PATH) on the
> host, and path walks based on that fid should be done via openat with this
> fd, which will also "automatically" handle renames without QEMU needing to
> fixup the string paths?
I agree, it would make sense for QEMU to map fid to FD+O_PATH. That
would avoid the kind of issues you mentioned.
Christian, Greg, what do you think?
>
> In any case, this probably means that even if Landlock were to hold a fid
> reference, and QEMU does qid remapping, that's still not enough to
> guarantees that we won't have a different, unrelated file ending up with
> the same qid, at least under ext4.
>
> I'm not sure what's the way forward - would Landlock need to actually
> "open" the files (or do something that will cause a Topen to be issued by
> v9fs)?
> Alternatively if we believe this to be a QEMU issue, maybe
> Landlock don't need to work around it and should just hold fids (and use
> QIDs to key the rules) anyway despite server quirks like these. This can
> perhaps then be fixed in QEMU?
Yes, I think it would make sense for Landlock to open and keep open a
fid (and hopefully the related remote file). However, the v9fs umount
should be handled gracefully the same way Landlock tag inodes are
handled. This should come with a QEMU patch to fix the consistency
issue.
>
> (I guess the fact that QEMU is doing path tracking in the first place does
> gives more precedent for justifying doing path tracking in v9fs as well,
> but maybe that's the wrong way to think about it)
Anyway, if QEMU does it, wouldn't it be the same for Landlock to just
rely on fid? If QEMU uses FD+O_PATH, then Landlock would work even for
server-moved files.
>
> Test programs: openat.c [3], open_procselffd.c [4]
>
>
> [1]: https://gitlab.com/qemu-project/qemu/-/blob/44f51c1a3cf435daa82eb757740b59b1fd4fe71c/hw/9pfs/9p.c#L3403
> [2]: https://gitlab.com/qemu-project/qemu/-/blob/371a269ff8ce561c28e4fa03bb49e4940f990637/hw/9pfs/9p-local.c#L1243
> [3]: https://fileshare.maowtm.org/9pfs-landlock-fix/20250921/openat.c
> [4]: https://fileshare.maowtm.org/9pfs-landlock-fix/20250921/open_procselffd.c
>
^ permalink raw reply
* Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules
From: Mickaël Salaün @ 2025-09-27 19:00 UTC (permalink / raw)
To: Tingmao Wang; +Cc: Günther Noack, Jan Kara, linux-security-module
In-Reply-To: <090e272e-64b4-4c56-9928-0e0be0288e64@maowtm.org>
On Sat, Sep 27, 2025 at 04:43:50PM +0100, Tingmao Wang wrote:
> On 9/24/25 10:20, Mickaël Salaün wrote:
> > On Mon, Sep 22, 2025 at 12:52:19AM +0100, Tingmao Wang wrote:
> >> [...]
> >> "accumulative". Therefore if this (different "quietness" for different
> >> access bits) becomes a strong need, we should probably consider some way
> >> of implementing it, rather than expecting a sandboxer to do this two-layer
> >> hack. (But implementing this does have the potential to result in needing
> >> to have a (number of access bits) x (number of layers) matrix...)
> >
> > Yes, that will indeed increase the size of rules, which is why I'm not
> > sure if it worth it.
>
> In addition to the size of each rule, another concern is that we would
> need another layer_mask_t[LANDLOCK_NUM_ACCESS_FS] matrix on the stack in
> order to correctly accumulate quiet bits when multiple access bits are
> requested, and the quiet bits are only set on some of them / spread out
> between different objects. For example, for this 1 layer domain:
>
> / quiet: r
> /etc quiet: w
>
> request: /etc/file rw
> # This should not audit log, but if we only keep one accumulated bit
> # per layer, we would not be able to tell that all access bits are
> # eventually "covered" by quiet flags.
>
> The alternative approach you suggested below would get rid of this
> situation as well.
>
> >
> > The alternative I was thinking about is to only increase the size of
> > struct landlock_ruleset (which would be negligible) to include sets of
> > quiet access rights. A request to such access right *on* a quiet object
> > would never be logged. I think this approach is flexible enough and
> > doesn't increase much the complexity. This would also be useful to not
> > log access rights that don't have associated rules (e.g. scopes), and
> > then no identified objects. To avoid the kind of hack you pointed out,
> > this feature should probably be part of this patch series though. What
> > do you think?
>
> This seems reasonable to me, especially if we don't think that having
> separate quiet access bit controls for each object would be a common need.
> Although if for some reason such control is needed, one might still be
> tempted to use the kind of two layer hack I mentioned. Maybe some program
> would like to quiet reads globally but only quiet writes to /run...?
>
> But maybe later on when we get to have supervised domains, the supervisor
> can tell Landlock whether to audit log or not for individual denied
> requests, or do such logging itself, therefore offering fine-grained
> control without hacks, and could potentially be more flexible e.g. only
> log once per request per process, etc.
>
> I think the most obvious way to implement this is to add a field to struct
> landlock_ruleset_attr, and landlock_create_ruleset would use the passed in
> size to determine if the quiet access bits field should be read or not?
Yes
>
> Also, do we want to consider calling this something else instead, like
> "suppress_audit"?
Quiet is simpler (similar to the LANDLOCK_RESTRICT_SELF_LOG_* flags) and
if we get other ways to log actions this will also be used. For the
supervisor case, that would be useful to not forward a request to the
supervisor. The *_LOG_* flags could be used the same way too (even if
"LOG" may be a subset of the supervisor capabilities). Do you think
that would be OK? Dedicated flags would be more flexible but also a bit
more complex. Is it worth it? In any case, the semantic and need
should be quite similar.
^ permalink raw reply
* Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules
From: Tingmao Wang @ 2025-09-27 15:43 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Jan Kara, linux-security-module
In-Reply-To: <20250924.wu8Ieku8aiph@digikod.net>
On 9/24/25 10:20, Mickaël Salaün wrote:
> On Mon, Sep 22, 2025 at 12:52:19AM +0100, Tingmao Wang wrote:
>> [...]
>> "accumulative". Therefore if this (different "quietness" for different
>> access bits) becomes a strong need, we should probably consider some way
>> of implementing it, rather than expecting a sandboxer to do this two-layer
>> hack. (But implementing this does have the potential to result in needing
>> to have a (number of access bits) x (number of layers) matrix...)
>
> Yes, that will indeed increase the size of rules, which is why I'm not
> sure if it worth it.
In addition to the size of each rule, another concern is that we would
need another layer_mask_t[LANDLOCK_NUM_ACCESS_FS] matrix on the stack in
order to correctly accumulate quiet bits when multiple access bits are
requested, and the quiet bits are only set on some of them / spread out
between different objects. For example, for this 1 layer domain:
/ quiet: r
/etc quiet: w
request: /etc/file rw
# This should not audit log, but if we only keep one accumulated bit
# per layer, we would not be able to tell that all access bits are
# eventually "covered" by quiet flags.
The alternative approach you suggested below would get rid of this
situation as well.
>
> The alternative I was thinking about is to only increase the size of
> struct landlock_ruleset (which would be negligible) to include sets of
> quiet access rights. A request to such access right *on* a quiet object
> would never be logged. I think this approach is flexible enough and
> doesn't increase much the complexity. This would also be useful to not
> log access rights that don't have associated rules (e.g. scopes), and
> then no identified objects. To avoid the kind of hack you pointed out,
> this feature should probably be part of this patch series though. What
> do you think?
This seems reasonable to me, especially if we don't think that having
separate quiet access bit controls for each object would be a common need.
Although if for some reason such control is needed, one might still be
tempted to use the kind of two layer hack I mentioned. Maybe some program
would like to quiet reads globally but only quiet writes to /run...?
But maybe later on when we get to have supervised domains, the supervisor
can tell Landlock whether to audit log or not for individual denied
requests, or do such logging itself, therefore offering fine-grained
control without hacks, and could potentially be more flexible e.g. only
log once per request per process, etc.
I think the most obvious way to implement this is to add a field to struct
landlock_ruleset_attr, and landlock_create_ruleset would use the passed in
size to determine if the quiet access bits field should be read or not?
Also, do we want to consider calling this something else instead, like
"suppress_audit"?
^ permalink raw reply
* [syzbot ci] Re: BPF signature hash chains
From: syzbot ci @ 2025-09-27 5:47 UTC (permalink / raw)
To: andrii, ast, bboscaccy, bpf, daniel, james.bottomley, kpsingh,
kys, linux-security-module, paul, wufan
Cc: syzbot, syzkaller-bugs
In-Reply-To: <20250926203111.1305999-1-bboscaccy@linux.microsoft.com>
syzbot ci has tested the following series
[v1] BPF signature hash chains
https://lore.kernel.org/all/20250926203111.1305999-1-bboscaccy@linux.microsoft.com
* [PATCH bpf-next 1/2] bpf: Add hash chain signature support for arbitrary maps
* [PATCH bpf-next 2/2] selftests/bpf: Enable map verification for some lskel tests
and found the following issue:
general protection fault in bpf_prog_verify_signature
Full report is available here:
https://ci.syzbot.org/series/9d93d1cd-d2bd-4967-a631-f3e84ee6fb41
***
general protection fault in bpf_prog_verify_signature
tree: bpf-next
URL: https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf-next.git
base: d43029ff7d1b7183dc0cf11b6cc2c12a0b810ad8
arch: amd64
compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
config: https://ci.syzbot.org/builds/a6b64e3c-6f08-4a6d-b484-ece99910a3f0/config
C repro: https://ci.syzbot.org/findings/a6c59e03-c4f7-4752-8d4f-526ccf945b7c/c_repro
syz repro: https://ci.syzbot.org/findings/a6c59e03-c4f7-4752-8d4f-526ccf945b7c/syz_repro
Oops: general protection fault, probably for non-canonical address 0xdffffc00020244a9: 0000 [#1] SMP KASAN PTI
KASAN: probably user-memory-access in range [0x0000000010122548-0x000000001012254f]
CPU: 1 UID: 0 PID: 6002 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:bpf_prog_verify_signature+0x610/0xc40 kernel/bpf/syscall.c:2873
Code: f7 e8 04 73 52 00 4d 8b 2e 89 d8 25 ff ff ff 7f 48 8b 4c 24 10 4c 8d 34 81 4c 89 f0 48 c1 e8 03 48 b9 00 00 00 00 00 fc ff df <0f> b6 04 08 84 c0 0f 85 c9 03 00 00 49 63 06 4c 8d 34 85 00 00 00
RSP: 0018:ffffc90002bff920 EFLAGS: 00010206
RAX: 00000000020244a9 RBX: 0000000004048956 RCX: dffffc0000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc90002bffb30 R08: ffffffff8fa3b537 R09: 1ffffffff1f476a6
R10: dffffc0000000000 R11: fffffbfff1f476a7 R12: 1ffff9200057ff34
R13: 0000000000000000 R14: 000000001012254a R15: 0000000080000000
FS: 0000555591f35500(0000) GS:ffff8881a3c11000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000011081a000 CR4: 00000000000006f0
Call Trace:
<TASK>
bpf_prog_load+0xcc7/0x19e0 kernel/bpf/syscall.c:3072
__sys_bpf+0x4ff/0x850 kernel/bpf/syscall.c:6199
__do_sys_bpf kernel/bpf/syscall.c:6309 [inline]
__se_sys_bpf kernel/bpf/syscall.c:6307 [inline]
__x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:6307
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7feeb318ec29
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffd613f7e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
RAX: ffffffffffffffda RBX: 00007feeb33d5fa0 RCX: 00007feeb318ec29
RDX: 00000000000000b9 RSI: 0000200000000100 RDI: 0000000000000005
RBP: 00007feeb3211e41 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007feeb33d5fa0 R14: 00007feeb33d5fa0 R15: 0000000000000003
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:bpf_prog_verify_signature+0x610/0xc40 kernel/bpf/syscall.c:2873
Code: f7 e8 04 73 52 00 4d 8b 2e 89 d8 25 ff ff ff 7f 48 8b 4c 24 10 4c 8d 34 81 4c 89 f0 48 c1 e8 03 48 b9 00 00 00 00 00 fc ff df <0f> b6 04 08 84 c0 0f 85 c9 03 00 00 49 63 06 4c 8d 34 85 00 00 00
RSP: 0018:ffffc90002bff920 EFLAGS: 00010206
RAX: 00000000020244a9 RBX: 0000000004048956 RCX: dffffc0000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc90002bffb30 R08: ffffffff8fa3b537 R09: 1ffffffff1f476a6
R10: dffffc0000000000 R11: fffffbfff1f476a7 R12: 1ffff9200057ff34
R13: 0000000000000000 R14: 000000001012254a R15: 0000000080000000
FS: 0000555591f35500(0000) GS:ffff8881a3c11000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000011081a000 CR4: 00000000000006f0
----------------
Code disassembly (best guess):
0: f7 e8 imul %eax
2: 04 73 add $0x73,%al
4: 52 push %rdx
5: 00 4d 8b add %cl,-0x75(%rbp)
8: 2e 89 d8 cs mov %ebx,%eax
b: 25 ff ff ff 7f and $0x7fffffff,%eax
10: 48 8b 4c 24 10 mov 0x10(%rsp),%rcx
15: 4c 8d 34 81 lea (%rcx,%rax,4),%r14
19: 4c 89 f0 mov %r14,%rax
1c: 48 c1 e8 03 shr $0x3,%rax
20: 48 b9 00 00 00 00 00 movabs $0xdffffc0000000000,%rcx
27: fc ff df
* 2a: 0f b6 04 08 movzbl (%rax,%rcx,1),%eax <-- trapping instruction
2e: 84 c0 test %al,%al
30: 0f 85 c9 03 00 00 jne 0x3ff
36: 49 63 06 movslq (%r14),%rax
39: 4c rex.WR
3a: 8d .byte 0x8d
3b: 34 85 xor $0x85,%al
3d: 00 00 add %al,(%rax)
***
If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
Tested-by: syzbot@syzkaller.appspotmail.com
---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.
^ permalink raw reply
* Re: [PATCH v3 01/14] ACPI: APEI: Remove redundant rcu_read_lock/unlock() in spin_lock
From: Hanjun Guo @ 2025-09-27 3:22 UTC (permalink / raw)
To: pengdonglin, tj, tony.luck, jani.nikula, ap420073, jv, freude,
bcrl, trondmy, longman, kees
Cc: bigeasy, hdanton, paulmck, linux-kernel, linux-rt-devel,
linux-nfs, linux-aio, linux-fsdevel, linux-security-module,
netdev, intel-gfx, linux-wireless, linux-acpi, linux-s390,
cgroups, Rafael J. Wysocki, pengdonglin
In-Reply-To: <20250916044735.2316171-2-dolinux.peng@gmail.com>
On 2025/9/16 12:47, pengdonglin wrote:
> From: pengdonglin <pengdonglin@xiaomi.com>
>
> Since commit a8bb74acd8efe ("rcu: Consolidate RCU-sched update-side function definitions")
> there is no difference between rcu_read_lock(), rcu_read_lock_bh() and
> rcu_read_lock_sched() in terms of RCU read section and the relevant grace
> period. That means that spin_lock(), which implies rcu_read_lock_sched(),
> also implies rcu_read_lock().
>
> There is no need no explicitly start a RCU read section if one has already
> been started implicitly by spin_lock().
>
> Simplify the code and remove the inner rcu_read_lock() invocation.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
> Signed-off-by: pengdonglin <dolinux.peng@gmail.com>
> ---
> drivers/acpi/apei/ghes.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a0d54993edb3..97ee19f2cae0 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1207,12 +1207,10 @@ static int ghes_notify_hed(struct notifier_block *this, unsigned long event,
> int ret = NOTIFY_DONE;
>
> spin_lock_irqsave(&ghes_notify_lock_irq, flags);
> - rcu_read_lock();
> list_for_each_entry_rcu(ghes, &ghes_hed, list) {
> if (!ghes_proc(ghes))
> ret = NOTIFY_OK;
> }
> - rcu_read_unlock();
> spin_unlock_irqrestore(&ghes_notify_lock_irq, flags);
>
> return ret;
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Thanks
Hanjun
^ 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