* [PATCH] fuse: fix conversion of fuse_reverse_inval_entry() to start_removing()
From: NeilBrown @ 2025-11-30 22:06 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Val Packett
Cc: Amir Goldstein, Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason,
David Sterba, David Howells, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Tyler Hicks, Miklos Szeredi,
Chuck Lever, Olga Kornievskaia, Dai Ngo, Namjae Jeon,
Steve French, Sergey Senozhatsky, Carlos Maiolino, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Ondrej Mosnacek, Mateusz Guzik, Lorenzo Stoakes, Stefan Berger,
Darrick J. Wong, linux-kernel, netfs, ecryptfs, linux-nfs,
linux-unionfs, linux-cifs, linux-xfs, linux-security-module,
selinux
In-Reply-To: <6713ea38-b583-4c86-b74a-bea55652851d@packett.cool>
From: NeilBrown <neil@brown.name>
The recent conversion of fuse_reverse_inval_entry() to use
start_removing() was wrong.
As Val Packett points out the original code did not call ->lookup
while the new code does. This can lead to a deadlock.
Rather than using full_name_hash() and d_lookup() as the old code
did, we can use try_lookup_noperm() which combines these. Then
the result can be given to start_removing_dentry() to get the required
locks for removal. We then double check that the name hasn't
changed.
As 'dir' needs to be used several times now, we load the dput() until
the end, and initialise to NULL so dput() is always safe.
Reported-by: Val Packett <val@packett.cool>
Closes: https://lore.kernel.org/all/6713ea38-b583-4c86-b74a-bea55652851d@packett.cool
Fixes: c9ba789dad15 ("VFS: introduce start_creating_noperm() and start_removing_noperm()")
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/fuse/dir.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index a0d5b302bcc2..8384fa96cf53 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1390,8 +1390,8 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
{
int err = -ENOTDIR;
struct inode *parent;
- struct dentry *dir;
- struct dentry *entry;
+ struct dentry *dir = NULL;
+ struct dentry *entry = NULL;
parent = fuse_ilookup(fc, parent_nodeid, NULL);
if (!parent)
@@ -1404,11 +1404,19 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
dir = d_find_alias(parent);
if (!dir)
goto put_parent;
-
- entry = start_removing_noperm(dir, name);
- dput(dir);
- if (IS_ERR(entry))
- goto put_parent;
+ while (!entry) {
+ struct dentry *child = try_lookup_noperm(name, dir);
+ if (!child || IS_ERR(child))
+ goto put_parent;
+ entry = start_removing_dentry(dir, child);
+ dput(child);
+ if (IS_ERR(entry))
+ goto put_parent;
+ if (!d_same_name(entry, dir, name)) {
+ end_removing(entry);
+ entry = NULL;
+ }
+ }
fuse_dir_changed(parent);
if (!(flags & FUSE_EXPIRE_ONLY))
@@ -1446,6 +1454,7 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
end_removing(entry);
put_parent:
+ dput(dir);
iput(parent);
return err;
}
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH v2] tpm2-sessions: address out-of-range indexing
From: Jarkko Sakkinen @ 2025-11-30 21:35 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, stable, Peter Huewe, Jason Gunthorpe,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel, linux-kernel,
keyrings, linux-security-module
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID.
Address the issue by:
1. Rename 'name_size' as 'tpm2_name_size' so that it is bit easier to
recognize and make it a fallible function.
2. Check for only known algorithms in 'tpm2_name_size'. Return -EINVAL for
unrecognized algorithms.
3. In order to correctly propagate possible errors make also
'tpm2_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible and
address their possible errors at the call sites.
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>
---
v2:
There was spurious extra field added to tpm2_hash by mistake.
drivers/char/tpm/tpm2-cmd.c | 23 ++++-
drivers/char/tpm/tpm2-sessions.c | 108 ++++++++++++++--------
include/linux/tpm.h | 6 +-
security/keys/trusted-keys/trusted_tpm2.c | 38 ++++++--
4 files changed, 124 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 5b6ccf901623..e63254135a74 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,7 +187,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -202,8 +207,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) {
+ tpm2_end_auth_session(chip);
+ 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);
@@ -261,7 +272,11 @@ 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)
+ goto out;
+
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..82b9d9096fd1 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -141,19 +141,28 @@ struct tpm2_auth {
};
#ifdef CONFIG_TCG_TPM2_HMAC
+
/*
- * Name Size based on TPM algorithm (assumes no hash bigger than 255)
+ * Calculate size of the TPMT_HA payload of TPM2B_NAME.
*/
-static u8 name_size(const u8 *name)
+static int tpm2_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;
+ case TPM_ALG_SM3_256:
+ return SM3256_DIGEST_SIZE + 2;
+ }
+
+ return -EINVAL;
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +170,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 name_size_alg;
u32 val;
/* we're starting after the header so adjust the length */
@@ -172,9 +182,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]))
+ name_size_alg = tpm2_name_size(&buf->data[offset]);
+ if (name_size_alg < 0)
+ return name_size_alg;
+
+ if (val != name_size_alg)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -222,46 +238,59 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-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 name_size;
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");
+ return -ENOMEM;
}
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");
+ return -EIO;
+ }
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)
+ return tpm_ret_to_err(ret);
+ }
} else {
if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ return -EINVAL;
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ name_size = tpm2_name_size(name);
+ if (name_size < 0)
+ return name_size;
+
+ memcpy(auth->name[slot], name, name_size);
+ }
#endif
+ return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -537,7 +566,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-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 +578,10 @@ 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 name_size;
if (!auth)
- return;
+ return -EINVAL;
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -559,10 +589,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
cc = be32_to_cpu(head->ordinal);
i = tpm2_find_cc(chip, cc);
- if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
- }
+ if (i < 0)
+ return -EINVAL;
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +605,8 @@ 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);
+ return -EINVAL;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +637,12 @@ 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");
+ return -EINVAL;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ return -EINVAL;
}
/* encrypt before HMAC */
@@ -646,8 +674,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]));
+ name_size = tpm2_name_size(auth->name[i]);
+ if (name_size < 0)
+ return name_size;
+
+ sha256_update(&sctx, auth->name[i], name_size);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +699,7 @@ 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;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0e9e043f728c..1a59f0190eb3 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -528,8 +528,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);
@@ -562,7 +562,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);
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index e165b117bbca..33544b6bc105 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -283,7 +283,13 @@ 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) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(chip);
+ goto out_put;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -331,7 +337,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ goto out;
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -438,7 +449,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -450,7 +466,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)
@@ -497,7 +516,11 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -522,7 +545,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
* [PATCH] tpm2-sessions: address out-of-range indexing
From: Jarkko Sakkinen @ 2025-11-30 21:18 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, stable, Peter Huewe, Jason Gunthorpe,
James Bottomley, Mimi Zohar, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Ard Biesheuvel, linux-kernel,
keyrings, linux-security-module
'name_size' does not have any range checks, and it just directly indexes
with TPM_ALG_ID.
Address the issue by:
1. Rename 'name_size' as 'tpm2_name_size' so that it is bit easier to
recognize and make it a fallible function.
2. Check for only known algorithms in 'tpm2_name_size'. Return -EINVAL for
unrecognized algorithms.
3. In order to correctly propagate possible errors make also
'tpm2_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible and
address their possible errors at the call sites.
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>
---
drivers/char/tpm/tpm2-cmd.c | 23 ++++-
drivers/char/tpm/tpm2-sessions.c | 108 ++++++++++++++--------
include/linux/tpm.h | 7 +-
security/keys/trusted-keys/trusted_tpm2.c | 38 ++++++--
4 files changed, 125 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 5b6ccf901623..e63254135a74 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,7 +187,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
@@ -202,8 +207,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) {
+ tpm2_end_auth_session(chip);
+ 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);
@@ -261,7 +272,11 @@ 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)
+ goto out;
+
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..82b9d9096fd1 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -141,19 +141,28 @@ struct tpm2_auth {
};
#ifdef CONFIG_TCG_TPM2_HMAC
+
/*
- * Name Size based on TPM algorithm (assumes no hash bigger than 255)
+ * Calculate size of the TPMT_HA payload of TPM2B_NAME.
*/
-static u8 name_size(const u8 *name)
+static int tpm2_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;
+ case TPM_ALG_SM3_256:
+ return SM3256_DIGEST_SIZE + 2;
+ }
+
+ return -EINVAL;
}
static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
@@ -161,6 +170,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 name_size_alg;
u32 val;
/* we're starting after the header so adjust the length */
@@ -172,9 +182,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]))
+ name_size_alg = tpm2_name_size(&buf->data[offset]);
+ if (name_size_alg < 0)
+ return name_size_alg;
+
+ if (val != name_size_alg)
return -EINVAL;
+
memcpy(name, &buf->data[offset], val);
/* forget the rest */
return 0;
@@ -222,46 +238,59 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-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 name_size;
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");
+ return -ENOMEM;
}
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");
+ return -EIO;
+ }
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)
+ return tpm_ret_to_err(ret);
+ }
} else {
if (name)
- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+ return -EINVAL;
}
auth->name_h[slot] = handle;
- if (name)
- memcpy(auth->name[slot], name, name_size(name));
+ if (name) {
+ name_size = tpm2_name_size(name);
+ if (name_size < 0)
+ return name_size;
+
+ memcpy(auth->name[slot], name, name_size);
+ }
#endif
+ return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
@@ -537,7 +566,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
* will be caused by an incorrect programming model and indicated by a
* kernel message.
*/
-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 +578,10 @@ 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 name_size;
if (!auth)
- return;
+ return -EINVAL;
/* save the command code in BE format */
auth->ordinal = head->ordinal;
@@ -559,10 +589,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
cc = be32_to_cpu(head->ordinal);
i = tpm2_find_cc(chip, cc);
- if (i < 0) {
- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
- return;
- }
+ if (i < 0)
+ return -EINVAL;
+
attrs = chip->cc_attrs_tbl[i];
handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
@@ -576,9 +605,8 @@ 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);
+ return -EINVAL;
}
}
/* point offset_s to the start of the sessions */
@@ -609,12 +637,12 @@ 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");
+ return -EINVAL;
}
if (!hmac) {
- dev_err(&chip->dev, "TPM could not find HMAC session\n");
- return;
+ dev_err(&chip->dev, "could not find HMAC session\n");
+ return -EINVAL;
}
/* encrypt before HMAC */
@@ -646,8 +674,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]));
+ name_size = tpm2_name_size(auth->name[i]);
+ if (name_size < 0)
+ return name_size;
+
+ sha256_update(&sctx, auth->name[i], name_size);
} else {
__be32 h = cpu_to_be32(auth->name_h[i]);
@@ -668,6 +699,7 @@ 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;
}
EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0e9e043f728c..f168c547abae 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -413,6 +413,7 @@ enum tpm2_session_attributes {
struct tpm2_hash {
unsigned int crypto_id;
unsigned int tpm_id;
+ unsigned int hash_size;
};
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
@@ -528,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);
@@ -562,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);
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index e165b117bbca..33544b6bc105 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -283,7 +283,13 @@ 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) {
+ tpm_buf_destroy(&buf);
+ tpm2_end_auth_session(chip);
+ goto out_put;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
options->keyauth, TPM_DIGEST_SIZE);
@@ -331,7 +337,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ goto out;
+ }
+
rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
if (rc)
@@ -438,7 +449,12 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
+
tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
TPM_DIGEST_SIZE);
@@ -450,7 +466,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)
@@ -497,7 +516,11 @@ 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) {
+ tpm2_end_auth_session(chip);
+ return rc;
+ }
if (!options->policyhandle) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
@@ -522,7 +545,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 bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Al Viro @ 2025-11-30 6:46 UTC (permalink / raw)
To: Song Liu
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge, Shervin Oloumi
In-Reply-To: <CAPhsuW69nUeMf+89vwsBrwo4sv3P8xOypSfhafEu12HJKqAb+w@mail.gmail.com>
On Sat, Nov 29, 2025 at 09:57:43PM -0800, Song Liu wrote:
> > Your primitive is a walking TOCTOU bug - it's impossible to use safely.
>
> Good point. AFAICT, the sample TOCTOU bug applies to other LSMs that
> care about dev_name in sb_mount, namely, aa_bind_mount() for apparmor
> and tomoyo_mount_acl() for tomoyo.
sb_mount needs to be taken out of its misery; it makes very little sense
and it's certainly rife with TOCTOU issues.
What to replace it with is an interesting question, especially considering
how easy it is to bypass the damn thing with fsopen(), open_tree() and friends.
It certainly won't be a single hook; multiplexing thing aside, if
you look at e.g. loopback you'll see that there are two separate
operations involved - one is cloning a tree (that's where dev_name is
parsed in old API; the corresponding spot in the new one is open_tree()
with OPEN_TREE_CLONE in flags) and another - attaching that tree to
destination (move_mount(2) in the new API).
The former is "what", the latter - "where". And in open_tree()/move_mount()
it literally could be done by different processes - there's no problem
with open_tree() in one process, passing the resulting descriptor to
another process that will attach it.
Any checks you do sb_mount (or in your mount_loopback) would have
to have equivalent counterparts in those, or you get an easy way to
bypass them.
That's a very unpleasant can of worms; if you want to open it, be my
guest, but I would seriously suggest doing that after the end of merge
window - and going over the existing LSMs to see what they are trying to
do in that area before starting that thread. And yes, that's an example
of the reasons why I'm very sceptical about out-of-tree modules in
that area - with API in that state, we have no realistic way to promise
any kind of stability, with obvious consequences for everyone we can't
even see.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Song Liu @ 2025-11-30 5:57 UTC (permalink / raw)
To: Al Viro
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge, Shervin Oloumi
In-Reply-To: <20251130042357.GP3538@ZenIV>
On Sat, Nov 29, 2025 at 8:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Nov 26, 2025 at 04:50:06PM -0800, Song Liu wrote:
> > Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
> > by BPF LSM programs:
> >
> > bpf_kern_path():
> > - Resolves a pathname string to a struct path
>
> > These kfuncs enable BPF LSM programs to resolve pathnames provided by
> > hook arguments (e.g., dev_name from sb_mount) and validate or inspect
> > the resolved paths. The verifier enforces proper resource management
> > through acquire/release tracking.
>
> Oh, *brilliant*. Thank you for giving a wonderful example of the reasons
> why this is fundamentally worthless.
>
> OK, your "BPF LSM" has been called and it got that dev_name. You decide
> that you want to know what it resolves to (which, BTW, requries a really
> non-trivial amount of parsing other arguments - just to figure out whether
> it *is* a pathname of some sort). Thanks to your shiny new kfuncs you
> can do that! You are a proud holder of mount/dentry pair. You stare at
> those and decide whether it's OK to go on. Then you... drop that pair
> and let mount(2) proceed towards the point where it will (if you parsed
> the arguments correctly) repeat that pathname resolution and get a mount/dentry
> pair of its own, that may very well be different from what you've got the
> first time around.
>
> Your primitive is a walking TOCTOU bug - it's impossible to use safely.
Good point. AFAICT, the sample TOCTOU bug applies to other LSMs that
care about dev_name in sb_mount, namely, aa_bind_mount() for apparmor
and tomoyo_mount_acl() for tomoyo.
What would you recommend to do this properly? How about we add a new
LSM hook that works on the actual mount/dentry pair? Something like:
diff --git i/fs/namespace.c w/fs/namespace.c
index d82910f33dc4..3d5dc167f15f 100644
--- i/fs/namespace.c
+++ w/fs/namespace.c
@@ -2984,6 +2984,10 @@ static int do_loopback(const struct path *path,
const char *old_name,
if (err)
return err;
+ err = security_mount_loopback(old_path, path, recurse);
+ if (err)
+ return err;
+
if (mnt_ns_loop(old_path.dentry))
return -EINVAL;
(Or s/security_mount_loopback/some_other_name).
In other words, do you think we should go [1] by Shervin Oloumi?
CCing Shervin here.
We will also need something similar to cover move mount operation
via path_mount()=>do_move_mount_old()=>do_move_mount().
Thanks,
Song
[1] https://lore.kernel.org/linux-security-module/20250110021008.2704246-1-enlightened@chromium.org/
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Al Viro @ 2025-11-30 4:23 UTC (permalink / raw)
To: Song Liu
Cc: bpf, linux-fsdevel, linux-security-module, ast, daniel, andrii,
kernel-team, brauner, jack, paul, jmorris, serge
In-Reply-To: <20251127005011.1872209-3-song@kernel.org>
On Wed, Nov 26, 2025 at 04:50:06PM -0800, Song Liu wrote:
> Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
> by BPF LSM programs:
>
> bpf_kern_path():
> - Resolves a pathname string to a struct path
> These kfuncs enable BPF LSM programs to resolve pathnames provided by
> hook arguments (e.g., dev_name from sb_mount) and validate or inspect
> the resolved paths. The verifier enforces proper resource management
> through acquire/release tracking.
Oh, *brilliant*. Thank you for giving a wonderful example of the reasons
why this is fundamentally worthless.
OK, your "BPF LSM" has been called and it got that dev_name. You decide
that you want to know what it resolves to (which, BTW, requries a really
non-trivial amount of parsing other arguments - just to figure out whether
it *is* a pathname of some sort). Thanks to your shiny new kfuncs you
can do that! You are a proud holder of mount/dentry pair. You stare at
those and decide whether it's OK to go on. Then you... drop that pair
and let mount(2) proceed towards the point where it will (if you parsed
the arguments correctly) repeat that pathname resolution and get a mount/dentry
pair of its own, that may very well be different from what you've got the
first time around.
Your primitive is a walking TOCTOU bug - it's impossible to use safely.
NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
^ permalink raw reply
* Re: [PATCH v6 06/15] VFS: introduce start_creating_noperm() and start_removing_noperm()
From: Al Viro @ 2025-11-30 0:19 UTC (permalink / raw)
To: Val Packett
Cc: NeilBrown, Christian Brauner, Amir Goldstein, Jan Kara,
linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <6713ea38-b583-4c86-b74a-bea55652851d@packett.cool>
On Sat, Nov 29, 2025 at 09:01:05PM -0300, Val Packett wrote:
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 316922d5dd13..a0d5b302bcc2 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1397,27 +1397,25 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> > if (!parent)
> > return -ENOENT;
> > - inode_lock_nested(parent, I_MUTEX_PARENT);
> > if (!S_ISDIR(parent->i_mode))
> > - goto unlock;
> > + goto put_parent;
> > err = -ENOENT;
> > dir = d_find_alias(parent);
> > if (!dir)
> > - goto unlock;
> > + goto put_parent;
> > - name->hash = full_name_hash(dir, name->name, name->len);
> > - entry = d_lookup(dir, name);
> > + entry = start_removing_noperm(dir, name);
> > dput(dir);
> > - if (!entry)
> > - goto unlock;
> > + if (IS_ERR(entry))
> > + goto put_parent;
>
> This broke xdg-document-portal (and potentially other FUSE filesystems) by
> introducing a massive deadlock.
ACK. That chunk needs to be reverted - this is *not* "remove an object by
parent and name", it's "invalidate stuff under that parent with this
first name component" and I would like to understand what FUSE_EXPIRE_ONLY
thing is about.
Miklos, could you give some details on that thing? This chunk definitely
needs to go, the question is what that code is trying to do other than
d_invalidate()...
^ permalink raw reply
* Re: [PATCH v6 06/15] VFS: introduce start_creating_noperm() and start_removing_noperm()
From: Val Packett @ 2025-11-30 0:01 UTC (permalink / raw)
To: NeilBrown, Alexander Viro, Christian Brauner, Amir Goldstein
Cc: Jan Kara, linux-fsdevel, Jeff Layton, Chris Mason, David Sterba,
David Howells, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Tyler Hicks, Miklos Szeredi, Chuck Lever,
Olga Kornievskaia, Dai Ngo, Namjae Jeon, Steve French,
Sergey Senozhatsky, Carlos Maiolino, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek,
Mateusz Guzik, Lorenzo Stoakes, Stefan Berger, Darrick J. Wong,
linux-kernel, netfs, ecryptfs, linux-nfs, linux-unionfs,
linux-cifs, linux-xfs, linux-security-module, selinux
In-Reply-To: <20251113002050.676694-7-neilb@ownmail.net>
Hi,
On 11/12/25 9:18 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> xfs, fuse, ipc/mqueue need variants of start_creating or start_removing
> which do not check permissions.
> This patch adds _noperm versions of these functions.
> [..]
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 316922d5dd13..a0d5b302bcc2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1397,27 +1397,25 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
> if (!parent)
> return -ENOENT;
>
> - inode_lock_nested(parent, I_MUTEX_PARENT);
> if (!S_ISDIR(parent->i_mode))
> - goto unlock;
> + goto put_parent;
>
> err = -ENOENT;
> dir = d_find_alias(parent);
> if (!dir)
> - goto unlock;
> + goto put_parent;
>
> - name->hash = full_name_hash(dir, name->name, name->len);
> - entry = d_lookup(dir, name);
> + entry = start_removing_noperm(dir, name);
> dput(dir);
> - if (!entry)
> - goto unlock;
> + if (IS_ERR(entry))
> + goto put_parent;
This broke xdg-document-portal (and potentially other FUSE filesystems)
by introducing a massive deadlock.
❯ doas cat /proc/40751/stack # main thread
[<0>] __fuse_simple_request+0x37c/0x5c0 [fuse]
[<0>] fuse_lookup_name+0x12c/0x2a0 [fuse]
[<0>] fuse_lookup+0x9c/0x1e8 [fuse]
[<0>] lookup_one_qstr_excl+0xd4/0x160
[<0>] start_removing_noperm+0x5c/0x90
[<0>] fuse_reverse_inval_entry+0x64/0x1e0 [fuse]
[<0>] fuse_dev_do_write+0x13a8/0x16a8 [fuse]
[<0>] fuse_dev_write+0x64/0xa8 [fuse]
[<0>] do_iter_readv_writev+0x170/0x1d0
[<0>] vfs_writev+0x100/0x2d0
[<0>] do_writev+0x88/0x130
d_lookup which was previously used here —from what I could understand by
reading it— is cache-only and does not call into the FS's lookup at all.
This new start_removing_noperm calls start_dirop which calls
lookup_one_qstr_excl which according to its own comment is the "one and
only case when ->lookup() gets called on non in-lookup dentries". Well,
->lookup() is the request back to the userspace FUSE server.. but the
FUSE server is waiting for the write() to the FUSE device that invokes
this operation to return! We cannot reenter the FUSE server
from fuse_reverse_inval_entry.
x-d-p issue link: https://github.com/flatpak/xdg-desktop-portal/issues/1871
Reverting the fuse/dir.c changes has fixed that for me.
Thanks,
~val
^ permalink raw reply
* [GIT PULL] KEYS: trusted: keys-trusted-next-rc1
From: Jarkko Sakkinen @ 2025-11-29 21:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, James Bottomley, Mimi Zohar, keyrings,
linux-integrity, linux-security-module
The following changes since commit e1afacb68573c3cd0a3785c6b0508876cd3423bc:
Merge tag 'ceph-for-6.18-rc8' of https://github.com/ceph/ceph-client (2025-11-27 11:11:03 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git tags/keys-trusted-next-rc1
for you to fetch changes up to 62cd5d480b9762ce70d720a81fa5b373052ae05f:
KEYS: trusted: Fix a memory leak in tpm2_load_cmd (2025-11-29 22:57:30 +0200)
----------------------------------------------------------------
Hi,
This pull request includes couple of updates for trusted keys:
1. Remove duplicate 'tpm2_hash_map' and use the one in the drive via new
function 'tpm2_find_hash_alg'.
2. Fix a memory leak on failure paths of 'tpm2_load_cmd'.
BR, Jarkko
----------------------------------------------------------------
Jarkko Sakkinen (2):
KEYS: trusted: Replace a redundant instance of tpm2_hash_map
KEYS: trusted: Fix a memory leak in tpm2_load_cmd
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++++-
include/linux/tpm.h | 1 +
security/keys/trusted-keys/trusted_tpm2.c | 29 ++++++++---------------------
3 files changed, 22 insertions(+), 22 deletions(-)
^ permalink raw reply
* [GIT PULL] Smack patches for 6.19
From: Casey Schaufler @ 2025-11-29 20:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: LSM List, Linux kernel mailing list, Casey Schaufler,
Konstantin Andreev
In-Reply-To: <80229bac-c3d9-4c99-9cca-dade23ef7421.ref@schaufler-ca.com>
Hello Linus,
Here is the Smack pull request for v6.19.
It fairly large as Smack pulls go. There are fixes for several
cases where labels are treated inconsistently when imported
from user space. The assignment of extended attributes has
been cleaned up. There are also some documentation improvements.
The following changes since commit e04c78d86a9699d136910cfc0bdcf01087e3267e:
Linux 6.16-rc2 (2025-06-15 13:49:41 -0700)
are available in the Git repository at:
https://github.com/cschaufler/smack-next tags/Smack-for-6.19
for you to fetch changes up to 29c701f90b9341f1f9c1854a9c22b71c2318457d:
Smack: function parameter 'gfp' not described (2025-11-11 12:00:18 -0800)
----------------------------------------------------------------
Patches for 6.19
----------------------------------------------------------------
Casey Schaufler (1):
Smack: function parameter 'gfp' not described
Konstantin Andreev (8):
smack: deduplicate "does access rule request transmutation"
smack: fix bug: SMACK64TRANSMUTE set on non-directory
smack: deduplicate xattr setting in smack_inode_init_security()
smack: always "instantiate" inode in smack_inode_init_security()
smack: fix bug: invalid label of unix socket file
smack: fix bug: unprivileged task can create labels
smack: fix bug: setting task label silently ignores input garbage
smack: fix kernel-doc warnings for smk_import_valid_label()
Documentation/admin-guide/LSM/Smack.rst | 16 +-
security/smack/smack.h | 3 +
security/smack/smack_access.c | 96 ++++++++---
security/smack/smack_lsm.c | 279 +++++++++++++++++++++-----------
4 files changed, 275 insertions(+), 119 deletions(-)
^ permalink raw reply
* Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach
From: Bernd Edlinger @ 2025-11-29 15:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Alexander Viro, Alexey Dobriyan, Kees Cook,
Andy Lutomirski, Will Drewry, Andrew Morton, Michal Hocko,
Serge Hallyn, James Morris, Randy Dunlap, Suren Baghdasaryan,
Yafang Shao, Helge Deller, Eric W. Biederman, 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
In-Reply-To: <aSNTNZxiQ0txISJx@redhat.com>
On 11/23/25 19:32, Oleg Nesterov wrote:
> Hi Bernd,
>
> sorry for delay, I am on PTO, didn't read emails this week...
>
> On 11/17, Bernd Edlinger wrote:
>>
>> On 11/17/25 16:01, Oleg Nesterov wrote:
>>> On 11/17, Bernd Edlinger wrote:
>>>>
>>>> On 11/11/25 10:21, Christian Brauner wrote:
>>>>> On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
>>>>
>>>>>> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
>>>>>> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
>>>>>> unconditionally.
>>>>>>
>>>>
>>>> I would not like to drop the mutex when no absolutely necessary for performance reasons.
>>>
>>> OK, I won't insist... But I don't really understand how this can help to
>>> improve the performance. If nothing else, this adds another for_other_threads()
>>> loop.
>>>
>>
>> If no dead-lock is possible it is better to complete the de_thread without
>> releasing the mutex. For the debugger it is also the better experience,
>> no matter when the ptrace_attack happens it will succeed rather quickly either
>> before the execve or after the execve.
>
> I still disagree, I still don't understand the "performance reasons", but since I can't
> convince you I won't really argue.
>
>>>>>>> + if (unlikely(unsafe_execve_in_progress)) {
>>>>>>> + spin_unlock_irq(lock);
>>>>>>> + sig->exec_bprm = bprm;
>>>>>>> + mutex_unlock(&sig->cred_guard_mutex);
>>>>>>> + spin_lock_irq(lock);
>>>>>>
>>>>>> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
>>>>>>
>>>>
>>>> Since the spin lock was acquired while holding the mutex, both should be
>>>> unlocked in reverse sequence and the spin lock re-acquired after releasing
>>>> the mutex.
>>>
>>> Why?
>>>
>>
>> It is generally more safe when each thread acquires its mutexes in order and
>> releases them in reverse order.
>> Consider this:
>> Thread A:
>> holds spin_lock_irq(siglock);
>> does mutes_unlock(cred_guard_mutex); with irq disabled.
>> task switch happens to Thread B which has irq enabled.
>> and is waiting for cred_guard_mutex.
>> Thrad B:
>> does mutex_lock(cred_guard_mutex);
>> but is interrupted this point and the interrupt handler I executes
>> now iterrupt handler I wants to take siglock and is blocked,
>> because the system one single CPU core.
>
> I don't follow. Do you mean PREEMPT_RT ?
>
> If yes. In this case spin_lock_irq() is rt_spin_lock() which doesn't disable irqs,
> it does rt_lock_lock() (takes rt_mutex) + migrate_disable().
>
> I do think that spin/mutex/whatever_unlock() is always safe. In any order, and
> regardless of RT.
>
Well, based on my experience with other embedded real-time O/S-es, I would
expect that something named spin_lock_irq locks the task-specific IRQ, and
prevents task switches due to time-slicing, while something called
mutes_unlock may cause an explicit task switch, when another task is waiting
for the mutex.
It is hard to follow how linux implements that spin_lock_irq exactly, but
to me it looks like it is done this way:
include/linux/spinlock_api_smp.h:static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
include/linux/spinlock_api_smp.h-{
include/linux/spinlock_api_smp.h- local_irq_disable();
include/linux/spinlock_api_smp.h- preempt_disable();
include/linux/spinlock_api_smp.h- spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
include/linux/spinlock_api_smp.h- LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
include/linux/spinlock_api_smp.h-}
so an explicit task switch while locka_irq_disable looks
very dangerous to me. Do you know other places where such
a code pattern is used?
I do just ask, because a close look at those might reveal
some serious bugs, WDYT?
Thanks
Bernd.
^ permalink raw reply
* [PATCH] rust: security: use `pin_init::zeroed()` for LSM context initialization
From: Atharv Dubey @ 2025-11-29 13:56 UTC (permalink / raw)
To: paul, jmorris, serge, ojeda, alex.gaynor
Cc: boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, aliceryhl,
tmgross, dakr, linux-security-module, rust-for-linux,
linux-kernel, Atharv Dubey
Replace the previous `unsafe { core::mem::zeroed() }` initialization of
`bindings::lsm_context` with `pin_init::zeroed()`.
Link: https://github.com/Rust-for-Linux/linux/issues/1189
Signed-off-by: Atharv Dubey <atharvd440@gmail.com>
---
rust/kernel/security.rs | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
index 9d271695265f..4dc3eba6ce84 100644
--- a/rust/kernel/security.rs
+++ b/rust/kernel/security.rs
@@ -62,8 +62,7 @@ impl SecurityCtx {
/// Get the security context given its id.
#[inline]
pub fn from_secid(secid: u32) -> Result<Self> {
- // SAFETY: `struct lsm_context` can be initialized to all zeros.
- let mut ctx: bindings::lsm_context = unsafe { core::mem::zeroed() };
+ let mut ctx: bindings::lsm_context = pin_init::zeroed();
// SAFETY: Just a C FFI call. The pointer is valid for writes.
to_result(unsafe { bindings::security_secid_to_secctx(secid, &mut ctx) })?;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] net: ipv6: fix spelling typos in comments
From: Jakub Kicinski @ 2025-11-29 3:55 UTC (permalink / raw)
To: Shi Hao
Cc: davem, pabeni, dsahern, edumazet, horms, netdev,
linux-security-module, linux-kernel, steffen.klassert, herbert,
pablo
In-Reply-To: <20251127103133.13877-1-i.shihao.999@gmail.com>
On Thu, 27 Nov 2025 16:01:33 +0530 Shi Hao wrote:
> Correct misspelled typos in comments
>
> - informations -> information
> - wont -> won't
> - upto -> up to
> - destionation -> destination
Your changes does not apply, please rebase on netdev/net-next/main
--
pw-bot: cr
^ permalink raw reply
* [PATCH] tomoyo: Use local kmap in tomoyo_dump_page()
From: Davidlohr Bueso @ 2025-11-28 22:27 UTC (permalink / raw)
To: penguin-kernel, takedakn
Cc: linux-security-module, linux-kernel, Davidlohr Bueso
Replace the now deprecated kmap_atomic() with kmap_local_page().
The memcpy does not need atomic semantics, and the removed comment
is now stale - this patch now makes it in sync again. Last but not
least, highmem is going to be removed[0].
[0] https://lore.kernel.org/all/4ff89b72-03ff-4447-9d21-dd6a5fe1550f@app.fastmail.com/
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
security/tomoyo/domain.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 5f9ccab26e9a..90cf0e2969df 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -934,17 +934,12 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
#endif
if (page != dump->page) {
const unsigned int offset = pos % PAGE_SIZE;
- /*
- * Maybe kmap()/kunmap() should be used here.
- * But remove_arg_zero() uses kmap_atomic()/kunmap_atomic().
- * So do I.
- */
- char *kaddr = kmap_atomic(page);
+ char *kaddr = kmap_local_page(page);
dump->page = page;
memcpy(dump->data + offset, kaddr + offset,
PAGE_SIZE - offset);
- kunmap_atomic(kaddr);
+ kunmap_local(kaddr);
}
/* Same with put_arg_page(page) in fs/exec.c */
#ifdef CONFIG_MMU
--
2.39.5
^ permalink raw reply related
* Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`
From: Daniel Almeida @ 2025-11-28 18:06 UTC (permalink / raw)
To: Oliver Mangold
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Benno Lossin, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <20251117-unique-ref-v13-4-b5b243df1250@pm.me>
Hi Oliver,
> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> Types implementing one of these traits can safely convert between an
> `ARef<T>` and an `Owned<T>`.
>
> This is useful for types which generally are accessed through an `ARef`
> but have methods which can only safely be called when the reference is
> unique, like e.g. `block::mq::Request::end_ok()`.
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/owned.rs | 138 ++++++++++++++++++++++++++++++++++++++++++++---
> rust/kernel/sync/aref.rs | 11 +++-
> rust/kernel/types.rs | 2 +-
> 3 files changed, 141 insertions(+), 10 deletions(-)
>
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index a26747cbc13b..26ab2b00ada0 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
> @@ -5,6 +5,7 @@
> //! These pointer types are useful for C-allocated objects which by API-contract
> //! are owned by Rust, but need to be freed through the C API.
>
> +use crate::sync::aref::{ARef, RefCounted};
> use core::{
> mem::ManuallyDrop,
> ops::{Deref, DerefMut},
> @@ -14,14 +15,16 @@
>
> /// Type allocated and destroyed on the C side, but owned by Rust.
> ///
> -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
> +/// - This is useful when it is desirable to tie the lifetime of an object reference to an owned
> +/// object, rather than pass around a bare reference.
> +/// - [`Ownable`] types can define custom drop logic that is executed when the owned reference
> +/// of type [`Owned<_>`] pointing to the object is dropped.
> ///
> /// Note: The underlying object is not required to provide internal reference counting, because it
> /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
> +/// between unique and shared (reference counted) ownership is needed.
> ///
> /// # Safety
> ///
> @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
> /// mutable reference requirements. That is, the kernel will not mutate or free the underlying
> /// object and is okay with it being modified by Rust code.
> pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> - Self {
> - ptr,
> - }
> + Self { ptr }
> }
Unrelated change?
>
> /// Consumes the [`Owned`], returning a raw pointer.
> @@ -193,3 +194,124 @@ fn drop(&mut self) {
> unsafe { T::release(self.ptr) };
> }
> }
> +
> +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> +/// [`ARef`].
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`OwnableRefCounted`], [`Ownable`] and its usage with
> +/// [`ARef`] and [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// # use core::cell::Cell;
> +/// # use core::ptr::NonNull;
> +/// # use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// # use kernel::sync::aref::{ARef, RefCounted};
> +/// # use kernel::types::{Owned, Ownable, OwnableRefCounted};
> +///
> +/// // Example internally refcounted struct.
nit: IMHO the wording could improve ^
> +/// //
> +/// // # Invariants
> +/// //
> +/// // - `refcount` is always non-zero for a valid object.
> +/// // - `refcount` is >1 if there are more than 1 Rust reference to it.
> +/// //
> +/// struct Foo {
> +/// refcount: Cell<usize>,
> +/// }
> +///
> +/// impl Foo {
> +/// fn new() -> Result<Owned<Self>, AllocError> {
> +/// // We are just using a `KBox` here to handle the actual allocation, as our `Foo` is
> +/// // not actually a C-allocated object.
> +/// let result = KBox::new(
> +/// Foo {
> +/// refcount: Cell::new(1),
> +/// },
> +/// flags::GFP_KERNEL,
> +/// )?;
> +/// let result = NonNull::new(KBox::into_raw(result))
> +/// .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +/// // SAFETY: We just allocated the `Self`, thus it is valid and there cannot be any other
> +/// // Rust references. Calling `into_raw()` makes us responsible for ownership and
> +/// // we won't use the raw pointer anymore, thus we can transfer ownership to the `Owned`.
> +/// Ok(unsafe { Owned::from_raw(result) })
> +/// }
> +/// }
> +///
> +/// // SAFETY: We increment and decrement each time the respective function is called and only free
> +/// // the `Foo` when the refcount reaches zero.
> +/// unsafe impl RefCounted for Foo {
> +/// fn inc_ref(&self) {
> +/// self.refcount.replace(self.refcount.get() + 1);
> +/// }
> +///
> +/// unsafe fn dec_ref(this: NonNull<Self>) {
> +/// // SAFETY: By requirement on calling this function, the refcount is non-zero,
> +/// // implying the underlying object is valid.
> +/// let refcount = unsafe { &this.as_ref().refcount };
> +/// let new_refcount = refcount.get() - 1;
> +/// if new_refcount == 0 {
> +/// // The `Foo` will be dropped when `KBox` goes out of scope.
> +/// // SAFETY: The [`KBox<Foo>`] is still alive as the old refcount is 1. We can pass
> +/// // ownership to the [`KBox`] as by requirement on calling this function,
> +/// // the `Self` will no longer be used by the caller.
> +/// unsafe { KBox::from_raw(this.as_ptr()) };
> +/// } else {
> +/// refcount.replace(new_refcount);
> +/// }
> +/// }
> +/// }
> +///
> +/// impl OwnableRefCounted for Foo {
> +/// fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> +/// if this.refcount.get() == 1 {
> +/// // SAFETY: The `Foo` is still alive and has no other Rust references as the refcount
> +/// // is 1.
> +/// Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +/// } else {
> +/// Err(this)
> +/// }
> +/// }
> +/// }
> +///
We wouldn’t need this implementation if we added a “refcount()”
member to this trait. This lets you abstract away this logic for all
implementors, which has the massive upside of making sure we hardcode (and thus
enforce) the refcount == 1 check.
> +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
> +/// unsafe impl Ownable for Foo {
> +/// unsafe fn release(this: NonNull<Self>) {
> +/// // SAFETY: Using `dec_ref()` from [`RefCounted`] to release is okay, as the refcount is
> +/// // always 1 for an [`Owned<Foo>`].
> +/// unsafe{ Foo::dec_ref(this) };
> +/// }
> +/// }
> +///
> +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
All these “expects()” and custom error strings would go away if you
place this behind a fictional function that returns Result.
> +/// let mut foo = ARef::from(foo);
> +/// {
> +/// let bar = foo.clone();
> +/// assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub trait OwnableRefCounted: RefCounted + Ownable + Sized {
> + /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> + /// Otherwise it returns again an [`ARef`] to the same underlying object.
> + fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
Again, this method can go way if we add a method to expose the refcount.
> +
> + /// Converts the [`Owned`] into an [`ARef`].
> + fn into_shared(this: Owned<Self>) -> ARef<Self> {
> + // SAFETY: Safe by the requirements on implementing the trait.
> + unsafe { ARef::from_raw(Owned::into_raw(this)) }
> + }
> +}
> +
> +impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
> + type Error = ARef<T>;
> + /// Tries to convert the [`ARef`] to an [`Owned`] by calling
> + /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
> + /// unique, it returns again an [`ARef`] to the same underlying object.
> + fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
> + T::try_from_shared(b)
> + }
> +}
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 937dcf6ed5de..2dbffe2ed1b8 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -30,7 +30,10 @@
> /// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
> /// internal reference count and provides only shared references. If unique references are required
> /// [`Ownable`](crate::types::Ownable) should be implemented which allows types to be wrapped in an
> -/// [`Owned<Self>`](crate::types::Owned).
> +/// [`Owned<Self>`](crate::types::Owned). Implementing the trait
> +/// [`OwnableRefCounted`](crate::types::OwnableRefCounted) allows to convert between unique and
> +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and
> +/// [`ARef<Self>`](crate::types::Owned)).
> ///
> /// # Safety
> ///
> @@ -180,6 +183,12 @@ fn from(b: &T) -> Self {
> }
> }
>
> +impl<T: crate::types::OwnableRefCounted> From<crate::types::Owned<T>> for ARef<T> {
> + fn from(b: crate::types::Owned<T>) -> Self {
> + T::into_shared(b)
> + }
> +}
> +
Not sure why we’re using fully-qualified names here if we can import them, but your call.
> impl<T: RefCounted> Drop for ARef<T> {
> fn drop(&mut self) {
> // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ef01393352b..a9b72709d0d3 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,7 +11,7 @@
> };
> use pin_init::{PinInit, Wrapper, Zeroable};
>
> -pub use crate::owned::{Ownable, Owned};
> +pub use crate::owned::{Ownable, OwnableRefCounted, Owned};
>
> pub use crate::sync::aref::{ARef, AlwaysRefCounted, RefCounted};
>
>
> --
> 2.51.2
>
>
>
— Daniel
^ permalink raw reply
* Re: [PATCH v13 3/4] rust: Add missing SAFETY documentation for `ARef` example
From: Daniel Almeida @ 2025-11-28 17:50 UTC (permalink / raw)
To: Oliver Mangold
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Benno Lossin, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <20251117-unique-ref-v13-3-b5b243df1250@pm.me>
> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> SAFETY comment in rustdoc example was just 'TODO'. Fixed.
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync/aref.rs | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 4226119d5ac9..937dcf6ed5de 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -129,12 +129,14 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> /// # Examples
> ///
> /// ```
> - /// use core::ptr::NonNull;
> - /// use kernel::sync::aref::{ARef, RefCounted};
> + /// # use core::ptr::NonNull;
> + /// # use kernel::sync::aref::{ARef, RefCounted};
> ///
> /// struct Empty {}
> ///
> - /// # // SAFETY: TODO.
> + /// // SAFETY: The `RefCounted` implementation for `Empty` does not count references and
> + /// // never frees the underlying object. Thus we can act as having a refcount on the object
nit: perhaps saying “an increment on the refcount” is clearer?
> + /// // that we pass to the newly created `ARef`.
> /// unsafe impl RefCounted for Empty {
> /// fn inc_ref(&self) {}
> /// unsafe fn dec_ref(_obj: NonNull<Self>) {}
> @@ -142,7 +144,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> ///
> /// let mut data = Empty {};
> /// let ptr = NonNull::<Empty>::new(&mut data).unwrap();
> - /// # // SAFETY: TODO.
> + /// // SAFETY: We keep `data` around longer than the `ARef`.
> /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
> /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref);
> ///
>
> --
> 2.51.2
>
>
>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply
* Re: [PATCH v13 2/4] rust: `AlwaysRefCounted` is renamed to `RefCounted`.
From: Daniel Almeida @ 2025-11-28 17:46 UTC (permalink / raw)
To: Oliver Mangold
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Benno Lossin, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Rafael J. Wysocki, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Alexander Viro, Christian Brauner, Jan Kara, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński, Paul Moore,
Serge Hallyn, Asahi Lina, rust-for-linux, linux-kernel,
linux-block, dri-devel, linux-fsdevel, linux-mm, linux-pm,
linux-pci, linux-security-module
In-Reply-To: <20251117-unique-ref-v13-2-b5b243df1250@pm.me>
> On 17 Nov 2025, at 07:07, Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> `AlwaysRefCounted` will become a marker trait to indicate that it is
> allowed to obtain an `ARef<T>` from a `&T`, which cannot be allowed for
> types which are also Ownable.
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/auxiliary.rs | 7 +++++-
> rust/kernel/block/mq/request.rs | 15 +++++++------
> rust/kernel/cred.rs | 13 ++++++++++--
> rust/kernel/device.rs | 13 ++++++++----
> rust/kernel/device/property.rs | 7 +++++-
> rust/kernel/drm/device.rs | 10 ++++++---
> rust/kernel/drm/gem/mod.rs | 10 ++++++---
> rust/kernel/fs/file.rs | 16 ++++++++++----
> rust/kernel/mm.rs | 15 +++++++++----
> rust/kernel/mm/mmput_async.rs | 9 ++++++--
> rust/kernel/opp.rs | 10 ++++++---
> rust/kernel/owned.rs | 2 +-
> rust/kernel/pci.rs | 10 ++++++---
> rust/kernel/pid_namespace.rs | 12 +++++++++--
> rust/kernel/platform.rs | 7 +++++-
> rust/kernel/sync/aref.rs | 47 ++++++++++++++++++++++++++---------------
> rust/kernel/task.rs | 10 ++++++---
> rust/kernel/types.rs | 2 +-
> 18 files changed, 154 insertions(+), 61 deletions(-)
>
> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 7a3b0b9c418e..7f5b16053c11 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -10,6 +10,7 @@
> driver,
> error::{from_result, to_result, Result},
> prelude::*,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::Opaque,
> ThisModule,
> };
> @@ -239,7 +240,7 @@ extern "C" fn release(dev: *mut bindings::device) {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_ref().as_raw()) };
> @@ -258,6 +259,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> index c5f1f6b1ccfb..b6165f96b4ce 100644
> --- a/rust/kernel/block/mq/request.rs
> +++ b/rust/kernel/block/mq/request.rs
> @@ -8,7 +8,7 @@
> bindings,
> block::mq::Operations,
> error::Result,
> - sync::{atomic::Relaxed, Refcount},
> + sync::{aref::RefCounted, atomic::Relaxed, Refcount},
> types::{ARef, AlwaysRefCounted, Opaque},
> };
> use core::{marker::PhantomData, ptr::NonNull};
> @@ -225,11 +225,10 @@ unsafe impl<T: Operations> Send for Request<T> {}
> // mutate `self` are internally synchronized`
> unsafe impl<T: Operations> Sync for Request<T> {}
>
> -// SAFETY: All instances of `Request<T>` are reference counted. This
> -// implementation of `AlwaysRefCounted` ensure that increments to the ref count
> -// keeps the object alive in memory at least until a matching reference count
> -// decrement is executed.
> -unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
> +// SAFETY: All instances of `Request<T>` are reference counted. This implementation of `RefCounted`
> +// ensure that increments to the ref count keeps the object alive in memory at least until a
> +// matching reference count decrement is executed.
> +unsafe impl<T: Operations> RefCounted for Request<T> {
> fn inc_ref(&self) {
> self.wrapper_ref().refcount().inc();
> }
> @@ -251,3 +250,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
> }
> }
> }
> +
> +// SAFETY: We currently do not implement `Ownable`, thus it is okay to obtain an `ARef<Request>`
> +// from a `&Request` (but this will change in the future).
> +unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {}
> diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs
> index ffa156b9df37..20ef0144094b 100644
> --- a/rust/kernel/cred.rs
> +++ b/rust/kernel/cred.rs
> @@ -8,7 +8,12 @@
> //!
> //! Reference: <https://www.kernel.org/doc/html/latest/security/credentials.html>
>
> -use crate::{bindings, sync::aref::AlwaysRefCounted, task::Kuid, types::Opaque};
> +use crate::{
> + bindings,
> + sync::aref::RefCounted,
> + task::Kuid,
> + types::{AlwaysRefCounted, Opaque},
> +};
>
> /// Wraps the kernel's `struct cred`.
> ///
> @@ -76,7 +81,7 @@ pub fn euid(&self) -> Kuid {
> }
>
> // SAFETY: The type invariants guarantee that `Credential` is always ref-counted.
> -unsafe impl AlwaysRefCounted for Credential {
> +unsafe impl RefCounted for Credential {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -90,3 +95,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Credential>) {
> unsafe { bindings::put_cred(obj.cast().as_ptr()) };
> }
> }
> +
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Credential>` from a
> +// `&Credential`.
> +unsafe impl AlwaysRefCounted for Credential {}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index a849b7dde2fd..a69ee32997c1 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,9 +5,10 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> - bindings, fmt,
> - sync::aref::ARef,
> - types::{ForeignOwnable, Opaque},
> + bindings,
> + fmt,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, ForeignOwnable, Opaque},
> };
> use core::{marker::PhantomData, ptr};
>
> @@ -407,7 +408,7 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
> kernel::impl_device_context_into_aref!(Device);
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_raw()) };
> @@ -419,6 +420,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> // SAFETY: As by the type invariant `Device` can be sent to any thread.
> unsafe impl Send for Device {}
>
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 3a332a8c53a9..a8bb824ad0ec 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -14,6 +14,7 @@
> fmt,
> prelude::*,
> str::{CStr, CString},
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::{ARef, Opaque},
> };
>
> @@ -359,7 +360,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> }
>
> // SAFETY: Instances of `FwNode` are always reference-counted.
> -unsafe impl crate::types::AlwaysRefCounted for FwNode {
> +unsafe impl RefCounted for FwNode {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the
> // refcount is non-zero.
> @@ -373,6 +374,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<FwNode>` from a
> +// `&FwNode`.
> +unsafe impl AlwaysRefCounted for FwNode {}
> +
> enum Node<'a> {
> Borrowed(&'a FwNode),
> Owned(ARef<FwNode>),
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3ce8f62a0056..38ce7f389ed0 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -11,8 +11,8 @@
> error::from_err_ptr,
> error::Result,
> prelude::*,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, Opaque},
> };
> use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>
> @@ -198,7 +198,7 @@ fn deref(&self) -> &Self::Target {
>
> // SAFETY: DRM device objects are always reference counted and the get/put functions
> // satisfy the requirements.
> -unsafe impl<T: drm::Driver> AlwaysRefCounted for Device<T> {
> +unsafe impl<T: drm::Driver> RefCounted for Device<T> {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::drm_dev_get(self.as_raw()) };
> @@ -213,6 +213,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl<T: drm::Driver> AlwaysRefCounted for Device<T> {}
> +
> impl<T: drm::Driver> AsRef<device::Device> for Device<T> {
> fn as_ref(&self) -> &device::Device {
> // SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid,
> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 30c853988b94..4afd36e49205 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -10,8 +10,8 @@
> drm::driver::{AllocImpl, AllocOps},
> error::{to_result, Result},
> prelude::*,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, Opaque},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -56,7 +56,7 @@ pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted {
> }
>
> // SAFETY: All gem objects are refcounted.
> -unsafe impl<T: IntoGEMObject> AlwaysRefCounted for T {
> +unsafe impl<T: IntoGEMObject> RefCounted for T {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::drm_gem_object_get(self.as_raw()) };
> @@ -75,6 +75,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<T>` from a
> +// `&T`.
> +unsafe impl<T: IntoGEMObject> crate::types::AlwaysRefCounted for T {}
> +
> extern "C" fn open_callback<T: DriverObject>(
> raw_obj: *mut bindings::drm_gem_object,
> raw_file: *mut bindings::drm_file,
> diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
> index cd6987850332..86309ca5dad0 100644
> --- a/rust/kernel/fs/file.rs
> +++ b/rust/kernel/fs/file.rs
> @@ -12,8 +12,8 @@
> cred::Credential,
> error::{code::*, to_result, Error, Result},
> fmt,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::{NotThreadSafe, Opaque},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> };
> use core::ptr;
>
> @@ -192,7 +192,7 @@ unsafe impl Sync for File {}
>
> // SAFETY: The type invariants guarantee that `File` is always ref-counted. This implementation
> // makes `ARef<File>` own a normal refcount.
> -unsafe impl AlwaysRefCounted for File {
> +unsafe impl RefCounted for File {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -207,6 +207,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<File>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<File>` from a
> +// `&File`.
> +unsafe impl AlwaysRefCounted for File {}
> +
> /// Wraps the kernel's `struct file`. Not thread safe.
> ///
> /// This type represents a file that is not known to be safe to transfer across thread boundaries.
> @@ -228,7 +232,7 @@ pub struct LocalFile {
>
> // SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
> // makes `ARef<LocalFile>` own a normal refcount.
> -unsafe impl AlwaysRefCounted for LocalFile {
> +unsafe impl RefCounted for LocalFile {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -244,6 +248,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<LocalFile>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<LocalFile>` from a
> +// `&LocalFile`.
> +unsafe impl AlwaysRefCounted for LocalFile {}
> +
> impl LocalFile {
> /// Constructs a new `struct file` wrapper from a file descriptor.
> ///
> diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> index 4764d7b68f2a..dd9e3969e720 100644
> --- a/rust/kernel/mm.rs
> +++ b/rust/kernel/mm.rs
> @@ -13,8 +13,8 @@
>
> use crate::{
> bindings,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::{NotThreadSafe, Opaque},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -55,7 +55,7 @@ unsafe impl Send for Mm {}
> unsafe impl Sync for Mm {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for Mm {
> +unsafe impl RefCounted for Mm {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -69,6 +69,9 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Mm>` from a `&Mm`.
> +unsafe impl AlwaysRefCounted for Mm {}
> +
> /// A wrapper for the kernel's `struct mm_struct`.
> ///
> /// This type is like [`Mm`], but with non-zero `mm_users`. It can only be used when `mm_users` can
> @@ -91,7 +94,7 @@ unsafe impl Send for MmWithUser {}
> unsafe impl Sync for MmWithUser {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for MmWithUser {
> +unsafe impl RefCounted for MmWithUser {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -105,6 +108,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<MmWithUser>` from a
> +// `&MmWithUser`.
> +unsafe impl AlwaysRefCounted for MmWithUser {}
> +
> // Make all `Mm` methods available on `MmWithUser`.
> impl Deref for MmWithUser {
> type Target = Mm;
> diff --git a/rust/kernel/mm/mmput_async.rs b/rust/kernel/mm/mmput_async.rs
> index b8d2f051225c..aba4ce675c86 100644
> --- a/rust/kernel/mm/mmput_async.rs
> +++ b/rust/kernel/mm/mmput_async.rs
> @@ -10,7 +10,8 @@
> use crate::{
> bindings,
> mm::MmWithUser,
> - sync::aref::{ARef, AlwaysRefCounted},
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted},
> };
> use core::{ops::Deref, ptr::NonNull};
>
> @@ -34,7 +35,7 @@ unsafe impl Send for MmWithUserAsync {}
> unsafe impl Sync for MmWithUserAsync {}
>
> // SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for MmWithUserAsync {
> +unsafe impl RefCounted for MmWithUserAsync {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The pointer is valid since self is a reference.
> @@ -48,6 +49,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<MmWithUserAsync>`
> +// from a `&MmWithUserAsync`.
> +unsafe impl AlwaysRefCounted for MmWithUserAsync {}
> +
> // Make all `MmWithUser` methods available on `MmWithUserAsync`.
> impl Deref for MmWithUserAsync {
> type Target = MmWithUser;
> diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
> index 2c763fa9276d..77d1cc89c412 100644
> --- a/rust/kernel/opp.rs
> +++ b/rust/kernel/opp.rs
> @@ -16,8 +16,8 @@
> ffi::c_ulong,
> prelude::*,
> str::CString,
> - sync::aref::{ARef, AlwaysRefCounted},
> - types::Opaque,
> + sync::aref::RefCounted,
> + types::{ARef, AlwaysRefCounted, Opaque},
> };
>
> #[cfg(CONFIG_CPU_FREQ)]
> @@ -1037,7 +1037,7 @@ unsafe impl Send for OPP {}
> unsafe impl Sync for OPP {}
>
> /// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
> -unsafe impl AlwaysRefCounted for OPP {
> +unsafe impl RefCounted for OPP {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> unsafe { bindings::dev_pm_opp_get(self.0.get()) };
> @@ -1049,6 +1049,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<OPP>` from an
> +// `&OPP`.
> +unsafe impl AlwaysRefCounted for OPP {}
> +
> impl OPP {
> /// Creates an owned reference to a [`OPP`] from a valid pointer.
> ///
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index a2cdd2cb8a10..a26747cbc13b 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
> @@ -21,7 +21,7 @@
> ///
> /// Note: The underlying object is not required to provide internal reference counting, because it
> /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> -/// [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented.
> +/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> ///
> /// # Safety
> ///
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 7fcc5f6022c1..9ac70823fb4d 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -13,8 +13,8 @@
> io::{Io, IoRaw},
> irq::{self, IrqRequest},
> str::CStr,
> - sync::aref::ARef,
> - types::Opaque,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, Opaque},
> ThisModule,
> };
> use core::{
> @@ -601,7 +601,7 @@ pub fn set_master(&self) {
> impl crate::dma::Device for Device<device::Core> {}
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::pci_dev_get(self.as_raw()) };
> @@ -613,6 +613,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/pid_namespace.rs b/rust/kernel/pid_namespace.rs
> index 979a9718f153..4f6a94540e33 100644
> --- a/rust/kernel/pid_namespace.rs
> +++ b/rust/kernel/pid_namespace.rs
> @@ -7,7 +7,11 @@
> //! C header: [`include/linux/pid_namespace.h`](srctree/include/linux/pid_namespace.h) and
> //! [`include/linux/pid.h`](srctree/include/linux/pid.h)
>
> -use crate::{bindings, sync::aref::AlwaysRefCounted, types::Opaque};
> +use crate::{
> + bindings,
> + sync::aref::RefCounted,
> + types::{AlwaysRefCounted, Opaque},
> +};
> use core::ptr;
>
> /// Wraps the kernel's `struct pid_namespace`. Thread safe.
> @@ -41,7 +45,7 @@ pub unsafe fn from_ptr<'a>(ptr: *const bindings::pid_namespace) -> &'a Self {
> }
>
> // SAFETY: Instances of `PidNamespace` are always reference-counted.
> -unsafe impl AlwaysRefCounted for PidNamespace {
> +unsafe impl RefCounted for PidNamespace {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -55,6 +59,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<PidNamespace>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<PidNamespace>` from
> +// a `&PidNamespace`.
> +unsafe impl AlwaysRefCounted for PidNamespace {}
> +
> // SAFETY:
> // - `PidNamespace::dec_ref` can be called from any thread.
> // - It is okay to send ownership of `PidNamespace` across thread boundaries.
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 7205fe3416d3..bf2aa496b377 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -13,6 +13,7 @@
> irq::{self, IrqRequest},
> of,
> prelude::*,
> + sync::aref::{AlwaysRefCounted, RefCounted},
> types::Opaque,
> ThisModule,
> };
> @@ -468,7 +469,7 @@ pub fn optional_irq_by_name(&self, name: &CStr) -> Result<IrqRequest<'_>> {
> impl crate::dma::Device for Device<device::Core> {}
>
> // SAFETY: Instances of `Device` are always reference-counted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +unsafe impl RefCounted for Device {
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> unsafe { bindings::get_device(self.as_ref().as_raw()) };
> @@ -480,6 +481,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Device>` from a
> +// `&Device`.
> +unsafe impl AlwaysRefCounted for Device {}
> +
> impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> fn as_ref(&self) -> &device::Device<Ctx> {
> // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index e175aefe8615..4226119d5ac9 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -19,11 +19,9 @@
>
> use core::{marker::PhantomData, mem::ManuallyDrop, ops::Deref, ptr::NonNull};
>
> -/// Types that are _always_ reference counted.
> +/// Types that are internally reference counted.
> ///
> /// It allows such types to define their own custom ref increment and decrement functions.
> -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> -/// [`ARef<T>`].
> ///
> /// This is usually implemented by wrappers to existing structures on the C side of the code. For
> /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> @@ -40,9 +38,8 @@
> /// at least until matching decrements are performed.
> ///
> /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> -/// alive.)
> -pub unsafe trait AlwaysRefCounted {
> +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
> +pub unsafe trait RefCounted {
> /// Increments the reference count on the object.
> fn inc_ref(&self);
>
> @@ -55,11 +52,27 @@ pub unsafe trait AlwaysRefCounted {
> /// Callers must ensure that there was a previous matching increment to the reference count,
> /// and that the object is no longer used after its reference count is decremented (as it may
> /// result in the object being freed), unless the caller owns another increment on the refcount
> - /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> - /// [`AlwaysRefCounted::dec_ref`] once).
> + /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
> unsafe fn dec_ref(obj: NonNull<Self>);
> }
>
> +/// Always reference-counted type.
> +///
> +/// It allows to derive a counted reference [`ARef<T>`] from a `&T`.
> +///
> +/// This provides some convenience, but it allows "escaping" borrow checks on `&T`. As it
> +/// complicates attempts to ensure that a reference to T is unique, it is optional to provide for
> +/// [`RefCounted`] types. See *Safety* below.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
> +/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`crate::types::Ownable`]
> +/// cannot be implemented for the same type, as this would allow to violate the uniqueness guarantee
> +/// of [`crate::types::Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from
> +/// that.
> +pub unsafe trait AlwaysRefCounted: RefCounted {}
> +
> /// An owned reference to an always-reference-counted object.
> ///
> /// The object's reference count is automatically decremented when an instance of [`ARef`] is
> @@ -70,7 +83,7 @@ pub unsafe trait AlwaysRefCounted {
> ///
> /// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> /// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> -pub struct ARef<T: AlwaysRefCounted> {
> +pub struct ARef<T: RefCounted> {
> ptr: NonNull<T>,
> _p: PhantomData<T>,
> }
> @@ -79,16 +92,16 @@ pub struct ARef<T: AlwaysRefCounted> {
> // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> // `T` to be `Send` because any thread that has an `ARef<T>` may ultimately access `T` using a
> // mutable reference, for example, when the reference count reaches zero and `T` is dropped.
> -unsafe impl<T: AlwaysRefCounted + Sync + Send> Send for ARef<T> {}
> +unsafe impl<T: RefCounted + Sync + Send> Send for ARef<T> {}
>
> // SAFETY: It is safe to send `&ARef<T>` to another thread when the underlying `T` is `Sync`
> // because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally,
> // it needs `T` to be `Send` because any thread that has a `&ARef<T>` may clone it and get an
> // `ARef<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
> // example, when the reference count reaches zero and `T` is dropped.
> -unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
> +unsafe impl<T: RefCounted + Sync + Send> Sync for ARef<T> {}
>
> -impl<T: AlwaysRefCounted> ARef<T> {
> +impl<T: RefCounted> ARef<T> {
> /// Creates a new instance of [`ARef`].
> ///
> /// It takes over an increment of the reference count on the underlying object.
> @@ -117,12 +130,12 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> ///
> /// ```
> /// use core::ptr::NonNull;
> - /// use kernel::sync::aref::{ARef, AlwaysRefCounted};
> + /// use kernel::sync::aref::{ARef, RefCounted};
> ///
> /// struct Empty {}
> ///
> /// # // SAFETY: TODO.
> - /// unsafe impl AlwaysRefCounted for Empty {
> + /// unsafe impl RefCounted for Empty {
> /// fn inc_ref(&self) {}
> /// unsafe fn dec_ref(_obj: NonNull<Self>) {}
> /// }
> @@ -140,7 +153,7 @@ pub fn into_raw(me: Self) -> NonNull<T> {
> }
> }
>
> -impl<T: AlwaysRefCounted> Clone for ARef<T> {
> +impl<T: RefCounted> Clone for ARef<T> {
> fn clone(&self) -> Self {
> self.inc_ref();
> // SAFETY: We just incremented the refcount above.
> @@ -148,7 +161,7 @@ fn clone(&self) -> Self {
> }
> }
>
> -impl<T: AlwaysRefCounted> Deref for ARef<T> {
> +impl<T: RefCounted> Deref for ARef<T> {
> type Target = T;
>
> fn deref(&self) -> &Self::Target {
> @@ -165,7 +178,7 @@ fn from(b: &T) -> Self {
> }
> }
>
> -impl<T: AlwaysRefCounted> Drop for ARef<T> {
> +impl<T: RefCounted> Drop for ARef<T> {
> fn drop(&mut self) {
> // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> // decrement.
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 49fad6de0674..0a6e38d98456 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -9,8 +9,8 @@
> ffi::{c_int, c_long, c_uint},
> mm::MmWithUser,
> pid_namespace::PidNamespace,
> - sync::aref::ARef,
> - types::{NotThreadSafe, Opaque},
> + sync::aref::{AlwaysRefCounted, RefCounted},
> + types::{ARef, NotThreadSafe, Opaque},
> };
> use core::{
> cmp::{Eq, PartialEq},
> @@ -348,7 +348,7 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
> }
>
> // SAFETY: The type invariants guarantee that `Task` is always refcounted.
> -unsafe impl crate::sync::aref::AlwaysRefCounted for Task {
> +unsafe impl RefCounted for Task {
> #[inline]
> fn inc_ref(&self) {
> // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> @@ -362,6 +362,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +// SAFETY: We do not implement `Ownable`, thus it is okay to obtain an `ARef<Task>` from a
> +// `&Task`.
> +unsafe impl AlwaysRefCounted for Task {}
> +
> impl Kuid {
> /// Get the current euid.
> #[inline]
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 7bc07c38cd6c..8ef01393352b 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -13,7 +13,7 @@
>
> pub use crate::owned::{Ownable, Owned};
>
> -pub use crate::sync::aref::{ARef, AlwaysRefCounted};
> +pub use crate::sync::aref::{ARef, AlwaysRefCounted, RefCounted};
>
> /// Used to transfer ownership to and from foreign (non-Rust) languages.
> ///
>
> --
> 2.51.2
>
>
>
Can you use imperative voice in the title? i.e.:
rust: rename `AlwaysRefCounted` to `RefCounted
…or something similar?
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply
* Re: [PATCH v4 1/4] landlock: Fix handling of disconnected directories
From: Tingmao Wang @ 2025-11-28 17:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Al Viro, Ben Scarlato, Christian Brauner,
Jann Horn, Jeff Xu, Justin Suess, Mikhail Ivanov, Paul Moore,
Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <20251128.oht7Aic8nu9d@digikod.net>
On 11/28/25 16:56, Mickaël Salaün wrote:
> On Fri, Nov 28, 2025 at 01:45:29AM +0000, Tingmao Wang wrote:
>> [...]
>>
>> Stepping back a bit, I also think it is reasonable to leave this issue as
>> is and not mitigate it (maybe warn about it in some way in the docs),
>> given that this can only happen if the policy is already weird (if the
>> intention is to protect some file, setting an allow access rule on its
>> parent, even if that parent is "hidden", is questionable).
>
> I agree.
Some additional bit of reasoning, just to make sure this is sound, and
access gaining can really only happen if the policy deliberately adds rule
above protected hierarchies (i.e. this can't be exploited if the policy is
not "problematic", even if it has other hidden rules):
As far as I can tell, there is no way to exploit a "hidden" rule like this
to e.g. read a file if the file is not already under the "hidden" rule,
since in this case the file must be outside of the bind mount. You can't
move files across mounts, and so the sandboxed application won't be able
to move it into the bind mount and cause the situation described above,
whether the destination is connected or disconnected. (It also can't move
the file into such a mount from the source fs of the bind mount, even if
it has visibility to the source fs, since the refer check would fail
there.)
^ permalink raw reply
* [PATCH v5 3/4] selftests/landlock: Add tests for access through disconnected paths
From: Mickaël Salaün @ 2025-11-28 17:21 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Al Viro, Ben Scarlato, Christian Brauner, Jann Horn, Jeff Xu,
Justin Suess, Matthieu Buffet, Mikhail Ivanov, NeilBrown,
Nicolas Bouchinet, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module, Mickaël Salaün
In-Reply-To: <20251128172200.760753-1-mic@digikod.net>
From: Tingmao Wang <m@maowtm.org>
This adds tests for the edge case discussed in [1], with specific ones
for rename and link operations when the operands are through
disconnected paths, as that go through a separate code path in Landlock.
This has resulted in a warning, due to collect_domain_accesses() not
expecting to reach a different root from path->mnt:
# RUN layout1_bind.path_disconnected ...
# OK layout1_bind.path_disconnected
ok 96 layout1_bind.path_disconnected
# RUN layout1_bind.path_disconnected_rename ...
[..] ------------[ cut here ]------------
[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
[..] ...
[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
[..] current_check_refer_path (security/landlock/fs.c:1205)
[..] ...
[..] hook_path_rename (security/landlock/fs.c:1526)
[..] security_path_rename (security/security.c:2026 (discriminator 1))
[..] do_renameat2 (fs/namei.c:5264)
# OK layout1_bind.path_disconnected_rename
ok 97 layout1_bind.path_disconnected_rename
Move the const char definitions a bit above so that we can use the path
for s4d1 in cleanup code.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Link: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org [1]
Signed-off-by: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- Update layout1_bind.path_disconnected tests according to the semantic
change of the fix.
- Update documentation for the layout1_bind hierarchy with the new s4d1
directory.
Changes since v1:
- Integrate this patch into my patch series, and change the result for
two tests with updated comments. Diff here:
https://lore.kernel.org/r/20250701183812.3201231-2-mic@digikod.net
- Replace most ASSERT with EXPECT, add extra checks, massage commit
message and comments.
- Squash Tingmao's patches:
https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org
https://lore.kernel.org/r/8ed0bfcd-aefa-44bd-86b6-e12583779187@maowtm.org
https://lore.kernel.org/r/3080e512-64b0-42cf-b379-8f52cfeff78a@maowtm.org
---
tools/testing/selftests/landlock/fs_test.c | 423 ++++++++++++++++++++-
1 file changed, 415 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index fa0f18ec62c4..032dd5dcf5eb 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4561,6 +4561,18 @@ TEST_F_FORK(ioctl, handle_file_access_file)
FIXTURE(layout1_bind) {};
/* clang-format on */
+static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
+static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+
+/* Move targets for disconnected path tests. */
+static const char dir_s4d1[] = TMP_DIR "/s4d1";
+static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
+static const char file2_s4d1[] = TMP_DIR "/s4d1/f2";
+static const char dir_s4d2[] = TMP_DIR "/s4d1/s4d2";
+static const char file1_s4d2[] = TMP_DIR "/s4d1/s4d2/f1";
+static const char file1_name[] = "f1";
+static const char file2_name[] = "f2";
+
FIXTURE_SETUP(layout1_bind)
{
prepare_layout(_metadata);
@@ -4576,14 +4588,14 @@ FIXTURE_TEARDOWN_PARENT(layout1_bind)
{
/* umount(dir_s2d2)) is handled by namespace lifetime. */
+ remove_path(file1_s4d1);
+ remove_path(file2_s4d1);
+
remove_layout1(_metadata);
cleanup_layout(_metadata);
}
-static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
-static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
-
/*
* layout1_bind hierarchy:
*
@@ -4594,20 +4606,25 @@ static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
* │ └── s1d2
* │ ├── f1
* │ ├── f2
- * │ └── s1d3
+ * │ └── s1d3 [disconnected by path_disconnected]
* │ ├── f1
* │ └── f2
* ├── s2d1
* │ ├── f1
- * │ └── s2d2
+ * │ └── s2d2 [bind mount from s1d2]
* │ ├── f1
* │ ├── f2
* │ └── s1d3
* │ ├── f1
* │ └── f2
- * └── s3d1
- * └── s3d2
- * └── s3d3
+ * ├── s3d1
+ * │ └── s3d2
+ * │ └── s3d3
+ * └── s4d1 [renamed from s1d3 by path_disconnected]
+ * ├── f1
+ * ├── f2
+ * └── s4d2
+ * └── f1
*/
TEST_F_FORK(layout1_bind, no_restriction)
@@ -4806,6 +4823,396 @@ TEST_F_FORK(layout1_bind, reparent_cross_mount)
ASSERT_EQ(0, rename(bind_file1_s1d3, file1_s2d2));
}
+/*
+ * Make sure access to file through a disconnected path works as expected.
+ * This test moves s1d3 to s4d1.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected)
+{
+ const struct rule layer1_allow_all[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_ALL,
+ },
+ {},
+ };
+ const struct rule layer2_allow_just_f1[] = {
+ {
+ .path = file1_s1d3,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+ const struct rule layer3_only_s1d2[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+
+ /* Landlock should not deny access just because it is disconnected. */
+ int ruleset_fd_l1 =
+ create_ruleset(_metadata, ACCESS_ALL, layer1_allow_all);
+
+ /* Creates the new ruleset now before we move the dir containing the file. */
+ int ruleset_fd_l2 =
+ create_ruleset(_metadata, ACCESS_RW, layer2_allow_just_f1);
+ int ruleset_fd_l3 =
+ create_ruleset(_metadata, ACCESS_RW, layer3_only_s1d2);
+ int bind_s1d3_fd;
+
+ ASSERT_LE(0, ruleset_fd_l1);
+ ASSERT_LE(0, ruleset_fd_l2);
+ ASSERT_LE(0, ruleset_fd_l3);
+
+ enforce_ruleset(_metadata, ruleset_fd_l1);
+ EXPECT_EQ(0, close(ruleset_fd_l1));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+
+ /* Tests access is possible before we move. */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
+
+ /* Makes it disconnected. */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+ strerror(errno));
+ }
+
+ /* Tests that access is still possible. */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+
+ /*
+ * Tests that ".." is not possible (not because of Landlock, but just
+ * because it's disconnected).
+ */
+ EXPECT_EQ(ENOENT,
+ test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
+
+ /* This should still work with a narrower rule. */
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY));
+ /*
+ * Accessing a file through a disconnected file descriptor can still be
+ * allowed by a rule tied to this file, even if it is no longer visible in
+ * its mount point.
+ */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+
+ enforce_ruleset(_metadata, ruleset_fd_l3);
+ EXPECT_EQ(0, close(ruleset_fd_l3));
+
+ EXPECT_EQ(EACCES, test_open(file1_s4d1, O_RDONLY));
+ /*
+ * Accessing a file through a disconnected file descriptor can still be
+ * allowed by a rule tied to the original mount point, even if it is no
+ * longer visible in its mount point.
+ */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+}
+
+/*
+ * Test that renameat with disconnected paths works under Landlock. This test
+ * moves s1d3 to s4d2, so that we can have a rule allowing refers on the move
+ * target's immediate parent.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_rename)
+{
+ const struct rule layer1[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_DIR |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {
+ .path = dir_s4d1,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_DIR |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {}
+ };
+
+ /* This layer only handles LANDLOCK_ACCESS_FS_READ_FILE. */
+ const struct rule layer2_only_s1d2[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+ int ruleset_fd_l1, ruleset_fd_l2;
+ pid_t child_pid;
+ int bind_s1d3_fd, status;
+
+ ASSERT_EQ(0, mkdir(dir_s4d1, 0755))
+ {
+ TH_LOG("Failed to create %s: %s", dir_s4d1, strerror(errno));
+ }
+ ruleset_fd_l1 = create_ruleset(_metadata, ACCESS_ALL, layer1);
+ ruleset_fd_l2 = create_ruleset(_metadata, LANDLOCK_ACCESS_FS_READ_FILE,
+ layer2_only_s1d2);
+ ASSERT_LE(0, ruleset_fd_l1);
+ ASSERT_LE(0, ruleset_fd_l2);
+
+ enforce_ruleset(_metadata, ruleset_fd_l1);
+ EXPECT_EQ(0, close(ruleset_fd_l1));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+ /* Tests ENOENT priority over EACCES for disconnected directory. */
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+ strerror(errno));
+ }
+ EXPECT_EQ(ENOENT, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));
+
+ /*
+ * The file is no longer under s1d2 but we should still be able to access it
+ * with layer 2 because its mount point is evaluated as the first valid
+ * directory because it was initially a parent. Do a fork to test this so
+ * we don't prevent ourselves from renaming it back later.
+ */
+ child_pid = fork();
+ ASSERT_LE(0, child_pid);
+ if (child_pid == 0) {
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s4d2, O_RDONLY));
+
+ /*
+ * Tests that access widening checks indeed prevents us from renaming it
+ * back.
+ */
+ EXPECT_EQ(-1, rename(dir_s4d2, dir_s1d3));
+ EXPECT_EQ(EXDEV, errno);
+
+ /*
+ * Including through the now disconnected fd (but it should return
+ * EXDEV).
+ */
+ EXPECT_EQ(-1, renameat(bind_s1d3_fd, file1_name, AT_FDCWD,
+ file1_s2d2));
+ EXPECT_EQ(EXDEV, errno);
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ EXPECT_EQ(child_pid, waitpid(child_pid, &status, 0));
+ EXPECT_EQ(1, WIFEXITED(status));
+ EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+ ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+ {
+ TH_LOG("Failed to rename %s back to %s: %s", dir_s4d1, dir_s1d3,
+ strerror(errno));
+ }
+
+ /* Now checks that we can access it under l2. */
+ child_pid = fork();
+ ASSERT_LE(0, child_pid);
+ if (child_pid == 0) {
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ EXPECT_EQ(child_pid, waitpid(child_pid, &status, 0));
+ EXPECT_EQ(1, WIFEXITED(status));
+ EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+ /*
+ * Also test that we can rename via a disconnected path. We move the
+ * dir back to the disconnected place first, then we rename file1 to
+ * file2 through our dir fd.
+ */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+ strerror(errno));
+ }
+ ASSERT_EQ(0,
+ renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name))
+ {
+ TH_LOG("Failed to rename %s to %s within disconnected %s: %s",
+ file1_name, file2_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+ ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
+ {
+ TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+ file2_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s2d2, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d2, O_RDONLY));
+
+ /* Move it back using the disconnected path as the target. */
+ ASSERT_EQ(0, renameat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file1_name))
+ {
+ TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+ file1_s1d2, file1_name, bind_dir_s1d3, strerror(errno));
+ }
+
+ /* Now make it connected again. */
+ ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+ {
+ TH_LOG("Failed to rename %s back to %s: %s", dir_s4d2, dir_s1d3,
+ strerror(errno));
+ }
+
+ /* Checks again that we can access it under l2. */
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+}
+
+/*
+ * Test that linkat(2) with disconnected paths works under Landlock. This
+ * test moves s1d3 to s4d1.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_link)
+{
+ /* Ruleset to be applied after renaming s1d3 to s4d1. */
+ const struct rule layer1[] = {
+ {
+ .path = dir_s4d1,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {
+ .path = dir_s2d2,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {}
+ };
+ int ruleset_fd, bind_s1d3_fd;
+
+ /* Removes unneeded files created by layout1, otherwise it will EEXIST. */
+ ASSERT_EQ(0, unlink(file1_s1d2));
+ ASSERT_EQ(0, unlink(file2_s1d3));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+ /* Disconnects bind_s1d3_fd. */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+ strerror(errno));
+ }
+
+ /* Need this later to test different parent link. */
+ ASSERT_EQ(0, mkdir(dir_s4d2, 0755))
+ {
+ TH_LOG("Failed to create %s: %s", dir_s4d2, strerror(errno));
+ }
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* From disconnected to connected. */
+ ASSERT_EQ(0, linkat(bind_s1d3_fd, file1_name, AT_FDCWD, file1_s2d2, 0))
+ {
+ TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+ file1_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+ }
+
+ /* Tests that we can access via the new link... */
+ EXPECT_EQ(0, test_open(file1_s2d2, O_RDONLY))
+ {
+ TH_LOG("Failed to open newly linked %s: %s", file1_s2d2,
+ strerror(errno));
+ }
+
+ /* ...as well as the old one. */
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+ {
+ TH_LOG("Failed to open original %s: %s", file1_s4d1,
+ strerror(errno));
+ }
+
+ /* From connected to disconnected. */
+ ASSERT_EQ(0, unlink(file1_s4d1));
+ ASSERT_EQ(0, linkat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file2_name, 0))
+ {
+ TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+ file1_s2d2, file2_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file2_s4d1, O_RDONLY));
+ ASSERT_EQ(0, unlink(file1_s2d2));
+
+ /* From disconnected to disconnected (same parent). */
+ ASSERT_EQ(0,
+ linkat(bind_s1d3_fd, file2_name, bind_s1d3_fd, file1_name, 0))
+ {
+ TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+ file2_name, file1_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+ {
+ TH_LOG("Failed to open newly linked %s: %s", file1_s4d1,
+ strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY))
+ {
+ TH_LOG("Failed to open %s through newly created link under disconnected path: %s",
+ file1_name, strerror(errno));
+ }
+ ASSERT_EQ(0, unlink(file2_s4d1));
+
+ /* From disconnected to disconnected (different parent). */
+ ASSERT_EQ(0,
+ linkat(bind_s1d3_fd, file1_name, bind_s1d3_fd, "s4d2/f1", 0))
+ {
+ TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+ file1_name, "s4d2/f1", bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s4d2, O_RDONLY))
+ {
+ TH_LOG("Failed to open %s after link: %s", file1_s4d2,
+ strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, "s4d2/f1", O_RDONLY))
+ {
+ TH_LOG("Failed to open %s through disconnected path after link: %s",
+ "s4d2/f1", strerror(errno));
+ }
+}
+
#define LOWER_BASE TMP_DIR "/lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
--
2.51.0
^ permalink raw reply related
* [PATCH v5 1/4] landlock: Fix handling of disconnected directories
From: Mickaël Salaün @ 2025-11-28 17:21 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Matthieu Buffet, Mikhail Ivanov, NeilBrown, Nicolas Bouchinet,
Paul Moore, Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <20251128172200.760753-1-mic@digikod.net>
Disconnected files or directories can appear when they are visible and
opened from a bind mount, but have been renamed or moved from the source
of the bind mount in a way that makes them inaccessible from the mount
point (i.e. out of scope).
Previously, access rights tied to files or directories opened through a
disconnected directory were collected by walking the related hierarchy
down to the root of the filesystem, without taking into account the
mount point because it couldn't be found. This could lead to
inconsistent access results, potential access right widening, and
hard-to-debug renames, especially since such paths cannot be printed.
For a sandboxed task to create a disconnected directory, it needs to
have write access (i.e. FS_MAKE_REG, FS_REMOVE_FILE, and FS_REFER) to
the underlying source of the bind mount, and read access to the related
mount point. Because a sandboxed task cannot acquire more access
rights than those defined by its Landlock domain, this could lead to
inconsistent access rights due to missing permissions that should be
inherited from the mount point hierarchy, while inheriting permissions
from the filesystem hierarchy hidden by this mount point instead.
Landlock now handles files and directories opened from disconnected
directories by taking into account the filesystem hierarchy when the
mount point is not found in the hierarchy walk, and also always taking
into account the mount point from which these disconnected directories
were opened. This ensures that a rename is not allowed if it would
widen access rights [1].
The rationale is that, even if disconnected hierarchies might not be
visible or accessible to a sandboxed task, relying on the collected
access rights from them improves the guarantee that access rights will
not be widened during a rename because of the access right comparison
between the source and the destination (see LANDLOCK_ACCESS_FS_REFER).
It may look like this would grant more access on disconnected files and
directories, but the security policies are always enforced for all the
evaluated hierarchies. This new behavior should be less surprising to
users and safer from an access control perspective.
Remove a wrong WARN_ON_ONCE() canary in collect_domain_accesses() and
fix the related comment.
Because opened files have their access rights stored in the related file
security properties, there is no impact for disconnected or unlinked
files.
Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Reported-by: Tingmao Wang <m@maowtm.org>
Closes: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org
Closes: https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org
Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
Link: https://lore.kernel.org/r/b0f46246-f2c5-42ca-93ce-0d629702a987@maowtm.org [1]
Reviewed-by: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v4:
- Improve comment.
- Add Reviewed-by.
Changes since v3:
- Simplify the approach by checking both the filesystem hierarchy and
the mount point hierarchy for disconnected directories. Remove the
related snapshot mechanism. See [1].
- Update erratum.
- Do not expose path_connected() anymore, and remove related Acked-by.
Changes since v2:
- Fix domain check mode reset, spotted by Tingmao. Replace a
conditional branch (when all accesses are granted) by only looking for
a rule when needed. This simplifies code and remove a call to
path_connected(). Add a new is_dom_check_bkp to safely handle race
conditions and move initial backup for layer_masks_parent1.
- Fix layer masks parent backup initialization, spotted by Tingmao.
- Add more comments, suggested by Tingmao.
- Reformat erratum.
Changes since v1:
- Remove useless else branch in is_access_to_paths_allowed().
- Update commit message and squash "landlock: Remove warning in
collect_domain_accesses()":
https://lore.kernel.org/r/20250618134734.1673254-1-mic@digikod.net
- Remove "extern" for path_connected() in fs.h, requested by Christian.
- Add Acked-by Christian.
- Fix docstring and improve doc for collect_domain_accesses().
- Remove path_connected() check for the real root.
- Fix allowed_parent* resets to be consistent with their initial values
infered from the evaluated domain.
- Add Landlock erratum.
---
security/landlock/errata/abi-1.h | 16 +++++++++++++
security/landlock/fs.c | 40 ++++++++++++++++++++++----------
2 files changed, 44 insertions(+), 12 deletions(-)
create mode 100644 security/landlock/errata/abi-1.h
diff --git a/security/landlock/errata/abi-1.h b/security/landlock/errata/abi-1.h
new file mode 100644
index 000000000000..e8a2bff2e5b6
--- /dev/null
+++ b/security/landlock/errata/abi-1.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/**
+ * DOC: erratum_3
+ *
+ * Erratum 3: Disconnected directory handling
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This fix addresses an issue with disconnected directories that occur when a
+ * directory is moved outside the scope of a bind mount. The change ensures
+ * that evaluated access rights include both those from the disconnected file
+ * hierarchy down to its filesystem root and those from the related mount point
+ * hierarchy. This prevents access right widening through rename or link
+ * actions.
+ */
+LANDLOCK_ERRATUM(3)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index d9c12b993fa7..a2ed0e76938a 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -909,21 +909,31 @@ static bool is_access_to_paths_allowed(
break;
}
}
+
if (unlikely(IS_ROOT(walker_path.dentry))) {
- /*
- * Stops at disconnected root directories. Only allows
- * access to internal filesystems (e.g. nsfs, which is
- * reachable through /proc/<pid>/ns/<namespace>).
- */
- if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
+ if (likely(walker_path.mnt->mnt_flags & MNT_INTERNAL)) {
+ /*
+ * Stops and allows access when reaching disconnected root
+ * directories that are part of internal filesystems (e.g. nsfs,
+ * which is reachable through /proc/<pid>/ns/<namespace>).
+ */
allowed_parent1 = true;
allowed_parent2 = true;
+ break;
}
- break;
+
+ /*
+ * We reached a disconnected root directory from a bind mount.
+ * Let's continue the walk with the mount point we missed.
+ */
+ dput(walker_path.dentry);
+ walker_path.dentry = walker_path.mnt->mnt_root;
+ dget(walker_path.dentry);
+ } else {
+ parent_dentry = dget_parent(walker_path.dentry);
+ dput(walker_path.dentry);
+ walker_path.dentry = parent_dentry;
}
- parent_dentry = dget_parent(walker_path.dentry);
- dput(walker_path.dentry);
- walker_path.dentry = parent_dentry;
}
path_put(&walker_path);
@@ -1021,6 +1031,9 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)
* file. While walking from @dir to @mnt_root, we record all the domain's
* allowed accesses in @layer_masks_dom.
*
+ * Because of disconnected directories, this walk may not reach @mnt_dir. In
+ * this case, the walk will continue to @mnt_dir after this call.
+ *
* This is similar to is_access_to_paths_allowed() but much simpler because it
* only handles walking on the same mount point and only checks one set of
* accesses.
@@ -1062,8 +1075,11 @@ static bool collect_domain_accesses(
break;
}
- /* We should not reach a root other than @mnt_root. */
- if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir)))
+ /*
+ * Stops at the mount point or the filesystem root for a disconnected
+ * directory.
+ */
+ if (dir == mnt_root || unlikely(IS_ROOT(dir)))
break;
parent_dentry = dget_parent(dir);
--
2.51.0
^ permalink raw reply related
* [PATCH v5 4/4] selftests/landlock: Add disconnected leafs and branch test suites
From: Mickaël Salaün @ 2025-11-28 17:21 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Matthieu Buffet, Mikhail Ivanov, NeilBrown, Nicolas Bouchinet,
Paul Moore, Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <20251128172200.760753-1-mic@digikod.net>
Test disconnected directories with two test suites
(layout4_disconnected_leafs and layout5_disconnected_branch) and 43
variants to cover the main corner cases.
These tests are complementary to the previous commit.
Add test_renameat() and test_exchangeat() helpers.
Test coverage for security/landlock is 92.1% of 1927 lines according to
LLVM 20.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Cc: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v4:
- Update the decision path description of layout5.
- Extend layout4.s1d41_s1d42_disconnected, layout4.s3d1_s4d1_new_parent,
layout5.s2d3_mount1_dst_parent and layout5.s4d1_rename_parent with
their _refer, _create, and _rename variants to better check the new
semantic.
- Add new tests to check different source and destination access rights
with layout4_disconnected_leafs.s1d41_s1d42_disconnected_rename and
s3d1_s4d1_disconnected_rename by extending them with _even, _more, and
_less variants, see
https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org
- Update ../.. checks in
layout5_disconnected_branch.read_rename_exchange tests to highlight
the specific ENOENT error.
Changes since v3:
- Update tests to reflect the new approach:
* layout4_disconnected_leafs.s1d41_s1d42_disconnected: allow all
* layout4_disconnected_leafs.s3d1_s4d1_new_parent: allow all
* layout4_disconnected_leafs.f1_f2_f3: allow read
* layout5_disconnected_branch.s2d3_mount1_dst_parent: allow all
* layout5_disconnected_branch.s4d1_rename_parent: allow all
- Update test coverage.
Changes since v2:
- Update test coverage.
Changes since v1:
- Rename layout4_disconnected to layout4_disconnected_leafs.
- Fix variable names.
- Add layout5_disconnected_branch test suite with 19 variants to cover
potential implementation issues.
---
tools/testing/selftests/landlock/fs_test.c | 1051 ++++++++++++++++++++
1 file changed, 1051 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 032dd5dcf5eb..eee814e09dd7 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -2267,6 +2267,22 @@ static int test_exchange(const char *const oldpath, const char *const newpath)
return 0;
}
+static int test_renameat(int olddirfd, const char *oldpath, int newdirfd,
+ const char *newpath)
+{
+ if (renameat2(olddirfd, oldpath, newdirfd, newpath, 0))
+ return errno;
+ return 0;
+}
+
+static int test_exchangeat(int olddirfd, const char *oldpath, int newdirfd,
+ const char *newpath)
+{
+ if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE))
+ return errno;
+ return 0;
+}
+
TEST_F_FORK(layout1, rename_file)
{
const struct rule rules[] = {
@@ -5213,6 +5229,1041 @@ TEST_F_FORK(layout1_bind, path_disconnected_link)
}
}
+/*
+ * layout4_disconnected_leafs with bind mount and renames:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the bind mount]
+ * │ ├── s1d31
+ * │ │ └── s1d41 [now renamed beneath s3d1]
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d32
+ * │ └── s1d42 [now renamed beneath s4d1]
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [bind mount of s1d2]
+ * │ ├── s1d31
+ * │ │ └── s1d41 [opened FD, now renamed beneath s3d1]
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d32
+ * │ └── s1d42 [opened FD, now renamed beneath s4d1]
+ * │ ├── f3
+ * │ └── f4
+ * ├── s3d1
+ * │ └── s1d41 [renamed here]
+ * │ ├── f1
+ * │ └── f2
+ * └── s4d1
+ * └── s1d42 [renamed here]
+ * ├── f3
+ * └── f4
+ */
+/* clang-format off */
+FIXTURE(layout4_disconnected_leafs) {
+ int s2d2_fd;
+};
+/* clang-format on */
+
+FIXTURE_SETUP(layout4_disconnected_leafs)
+{
+ prepare_layout(_metadata);
+
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f1");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f2");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f3");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f4");
+ create_directory(_metadata, TMP_DIR "/s2d1/s2d2");
+ create_directory(_metadata, TMP_DIR "/s3d1");
+ create_directory(_metadata, TMP_DIR "/s4d1");
+
+ self->s2d2_fd =
+ open(TMP_DIR "/s2d1/s2d2", O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s2d2_fd);
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0, mount(TMP_DIR "/s1d1/s1d2", TMP_DIR "/s2d1/s2d2", NULL,
+ MS_BIND, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout4_disconnected_leafs)
+{
+ /* umount(TMP_DIR "/s2d1") is handled by namespace lifetime. */
+
+ /* Removes files after renames. */
+ remove_path(TMP_DIR "/s3d1/s1d41/f1");
+ remove_path(TMP_DIR "/s3d1/s1d41/f2");
+ remove_path(TMP_DIR "/s4d1/s1d42/f1");
+ remove_path(TMP_DIR "/s4d1/s1d42/f3");
+ remove_path(TMP_DIR "/s4d1/s1d42/f4");
+ remove_path(TMP_DIR "/s4d1/s1d42/f5");
+
+ cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(layout4_disconnected_leafs)
+{
+ /*
+ * Parent of the bind mount source. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d1;
+ /*
+ * Source of bind mount (to s2d2). It should always be enforced when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d2;
+ /*
+ * Original parent of s1d41. It should always be ignored when testing
+ * against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s1d31;
+ /*
+ * Original parent of s1d42. It should always be ignored when testing
+ * against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s1d32;
+ /*
+ * Opened and disconnected source directory. It should always be enforced
+ * when testing against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s1d41;
+ /*
+ * Opened and disconnected source directory. It should always be enforced
+ * when testing against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s1d42;
+ /*
+ * File in the s1d41 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_f1;
+ /*
+ * File in the s1d41 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_f2;
+ /*
+ * File in the s1d42 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_f3;
+ /*
+ * Parent of the bind mount destination. It should always be enforced when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s2d1;
+ /*
+ * Directory covered by the bind mount. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s2d2;
+ /*
+ * New parent of the renamed s1d41. It should always be ignored when
+ * testing against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s3d1;
+ /*
+ * New parent of the renamed s1d42. It should always be ignored when
+ * testing against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s4d1;
+
+ /* Expected result of the call to open([fd:s1d41]/f1, O_RDONLY). */
+ const int expected_read_result;
+ /* Expected result of the call to renameat([fd:s1d41]/f1, [fd:s1d42]/f1). */
+ const int expected_rename_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d41]/f2, [fd:s1d42]/f3,
+ * RENAME_EXCHANGE).
+ */
+ const int expected_exchange_result;
+ /* Expected result of the call to renameat([fd:s1d42]/f4, [fd:s1d42]/f5). */
+ const int expected_same_dir_rename_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d1_mount_src_parent) {
+ /* clang-format on */
+ .allowed_s1d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_refer) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_create) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_rename) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d31_s1d32_old_parent) {
+ /* clang-format on */
+ .allowed_s1d31 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d32 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected_refer) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected_create) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected_rename_even) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* The destination directory has more access right. */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected_rename_more) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_EXECUTE,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ /* Access denied. */
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* The destination directory has less access right. */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected_rename_less) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_EXECUTE,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ /* Access allowed. */
+ .expected_rename_result = 0,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_create) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_refer) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_mini) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d2_covered_by_mount) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* Tests collect_domain_accesses(). */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s3d1_s4d1_new_parent_refer) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s3d1_s4d1_new_parent_create) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs,
+ s3d1_s4d1_disconnected_rename_even){
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* The destination directory has more access right. */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s3d1_s4d1_disconnected_rename_more) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_EXECUTE,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ /* Access denied. */
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* The destination directory has less access right. */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s3d1_s4d1_disconnected_rename_less) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_EXECUTE,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ /* Access allowed. */
+ .expected_rename_result = 0,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, f1_f2_f3) {
+ /* clang-format on */
+ .allowed_f1 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_f2 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_f3 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+TEST_F_FORK(layout4_disconnected_leafs, read_rename_exchange)
+{
+ const __u64 handled_access =
+ LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_MAKE_REG;
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR "/s1d1",
+ .access = variant->allowed_s1d1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2",
+ .access = variant->allowed_s1d2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31",
+ .access = variant->allowed_s1d31,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32",
+ .access = variant->allowed_s1d32,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41",
+ .access = variant->allowed_s1d41,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32/s1d42",
+ .access = variant->allowed_s1d42,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f1",
+ .access = variant->allowed_f1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f2",
+ .access = variant->allowed_f2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f3",
+ .access = variant->allowed_f3,
+ },
+ {
+ .path = TMP_DIR "/s2d1",
+ .access = variant->allowed_s2d1,
+ },
+ /* s2d2_fd */
+ {
+ .path = TMP_DIR "/s3d1",
+ .access = variant->allowed_s3d1,
+ },
+ {
+ .path = TMP_DIR "/s4d1",
+ .access = variant->allowed_s4d1,
+ },
+ {},
+ };
+ int ruleset_fd, s1d41_bind_fd, s1d42_bind_fd;
+
+ ruleset_fd = create_ruleset(_metadata, handled_access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds rule for the covered directory. */
+ if (variant->allowed_s2d2) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s2d2_fd,
+ .allowed_access =
+ variant->allowed_s2d2,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s2d2_fd));
+
+ s1d41_bind_fd = open(TMP_DIR "/s2d1/s2d2/s1d31/s1d41",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d41_bind_fd);
+ s1d42_bind_fd = open(TMP_DIR "/s2d1/s2d2/s1d32/s1d42",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d42_bind_fd);
+
+ /* Disconnects and checks source and destination directories. */
+ EXPECT_EQ(0, test_open_rel(s1d41_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(0, test_open_rel(s1d42_bind_fd, "..", O_DIRECTORY));
+ /* Renames to make it accessible through s3d1/s1d41 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d31/s1d41",
+ AT_FDCWD, TMP_DIR "/s3d1/s1d41"));
+ /* Renames to make it accessible through s4d1/s1d42 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d32/s1d42",
+ AT_FDCWD, TMP_DIR "/s4d1/s1d42"));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d41_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d42_bind_fd, "..", O_DIRECTORY));
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ EXPECT_EQ(variant->expected_read_result,
+ test_open_rel(s1d41_bind_fd, "f1", O_RDONLY));
+
+ EXPECT_EQ(variant->expected_rename_result,
+ test_renameat(s1d41_bind_fd, "f1", s1d42_bind_fd, "f1"));
+ EXPECT_EQ(variant->expected_exchange_result,
+ test_exchangeat(s1d41_bind_fd, "f2", s1d42_bind_fd, "f3"));
+
+ EXPECT_EQ(variant->expected_same_dir_rename_result,
+ test_renameat(s1d42_bind_fd, "f4", s1d42_bind_fd, "f5"));
+}
+
+/*
+ * layout5_disconnected_branch before rename:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the first bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [source of the second bind mount]
+ * │ └── s2d3
+ * │ └── s2d4 [first s1d2 bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s3d1
+ * │ └── s3d2 [second s2d2 bind mount]
+ * │ └── s2d3
+ * │ └── s2d4 [first s1d2 bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * └── s4d1
+ *
+ * After rename:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the first bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [source of the second bind mount]
+ * ├── s3d1
+ * │ └── s3d2 [second s2d2 bind mount]
+ * └── s4d1
+ * └── s2d3 [renamed here]
+ * └── s2d4 [first s1d2 bind mount]
+ * └── s1d3
+ * ├── s1d41
+ * │ ├── f1
+ * │ └── f2
+ * └── s1d42
+ * ├── f3
+ * └── f4
+ *
+ * Decision path for access from the s3d1/s3d2/s2d3/s2d4/s1d3 file descriptor:
+ * 1. first bind mount: s1d3 -> s1d2
+ * 2. second bind mount: s2d3
+ * 3. tmp mount: s4d1 -> tmp [disconnected branch]
+ * 4. second bind mount: s2d2
+ * 5. tmp mount: s3d1 -> tmp
+ * 6. parent mounts: [...] -> /
+ *
+ * The s4d1 directory is evaluated even if it is not in the s2d2 mount.
+ */
+
+/* clang-format off */
+FIXTURE(layout5_disconnected_branch) {
+ int s2d4_fd, s3d2_fd;
+};
+/* clang-format on */
+
+FIXTURE_SETUP(layout5_disconnected_branch)
+{
+ prepare_layout(_metadata);
+
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f1");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f2");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f3");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f4");
+ create_directory(_metadata, TMP_DIR "/s2d1/s2d2/s2d3/s2d4");
+ create_directory(_metadata, TMP_DIR "/s3d1/s3d2");
+ create_directory(_metadata, TMP_DIR "/s4d1");
+
+ self->s2d4_fd = open(TMP_DIR "/s2d1/s2d2/s2d3/s2d4",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s2d4_fd);
+
+ self->s3d2_fd =
+ open(TMP_DIR "/s3d1/s3d2", O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s3d2_fd);
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0, mount(TMP_DIR "/s1d1/s1d2", TMP_DIR "/s2d1/s2d2/s2d3/s2d4",
+ NULL, MS_BIND, NULL));
+ ASSERT_EQ(0, mount(TMP_DIR "/s2d1/s2d2", TMP_DIR "/s3d1/s3d2", NULL,
+ MS_BIND | MS_REC, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout5_disconnected_branch)
+{
+ /* Bind mounts are handled by namespace lifetime. */
+
+ /* Removes files after renames. */
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f1");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f2");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f1");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f3");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f4");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f5");
+
+ cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(layout5_disconnected_branch)
+{
+ /*
+ * Parent of all files. It should always be enforced when testing against
+ * files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_base;
+ /*
+ * Parent of the first bind mount source. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d1;
+ const __u64 allowed_s1d2;
+ const __u64 allowed_s1d3;
+ const __u64 allowed_s2d1;
+ const __u64 allowed_s2d2;
+ const __u64 allowed_s2d3;
+ const __u64 allowed_s2d4;
+ const __u64 allowed_s3d1;
+ const __u64 allowed_s3d2;
+ const __u64 allowed_s4d1;
+
+ /* Expected result of the call to open([fd:s1d3]/s1d41/f1, O_RDONLY). */
+ const int expected_read_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d41/f1,
+ * [fd:s1d3]/s1d42/f1).
+ */
+ const int expected_rename_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d41/f2,
+ * [fd:s1d3]/s1d42/f3, RENAME_EXCHANGE).
+ */
+ const int expected_exchange_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d42/f4,
+ * [fd:s1d3]/s1d42/f5).
+ */
+ const int expected_same_dir_rename_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d1_mount1_src_parent) {
+ /* clang-format on */
+ .allowed_s1d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_refer) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_create) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_rename) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_refer) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_create) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_rename) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_full) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d1_mount2_src_parent) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_refer) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_create) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_rename) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d3_mount1_dst_parent_refer) {
+ /* clang-format on */
+ .allowed_s2d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d3_mount1_dst_parent_create) {
+ /* clang-format on */
+ .allowed_s2d3 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d3_mount1_dst_parent_rename) {
+ /* clang-format on */
+ .allowed_s2d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d4_mount1_dst) {
+ /* clang-format on */
+ .allowed_s2d4 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_refer) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_create) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_rename) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d2_mount1_dst) {
+ /* clang-format on */
+ .allowed_s3d2 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s4d1_rename_parent_refer) {
+ /* clang-format on */
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s4d1_rename_parent_create) {
+ /* clang-format on */
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s4d1_rename_parent_rename) {
+ /* clang-format on */
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+TEST_F_FORK(layout5_disconnected_branch, read_rename_exchange)
+{
+ const __u64 handled_access =
+ LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_MAKE_REG;
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR "/s1d1",
+ .access = variant->allowed_s1d1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2",
+ .access = variant->allowed_s1d2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d3",
+ .access = variant->allowed_s1d3,
+ },
+ {
+ .path = TMP_DIR "/s2d1",
+ .access = variant->allowed_s2d1,
+ },
+ {
+ .path = TMP_DIR "/s2d1/s2d2",
+ .access = variant->allowed_s2d2,
+ },
+ {
+ .path = TMP_DIR "/s2d1/s2d2/s2d3",
+ .access = variant->allowed_s2d3,
+ },
+ /* s2d4_fd */
+ {
+ .path = TMP_DIR "/s3d1",
+ .access = variant->allowed_s3d1,
+ },
+ /* s3d2_fd */
+ {
+ .path = TMP_DIR "/s4d1",
+ .access = variant->allowed_s4d1,
+ },
+ {},
+ };
+ int ruleset_fd, s1d3_bind_fd;
+
+ ruleset_fd = create_ruleset(_metadata, handled_access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds rules for the covered directories. */
+ if (variant->allowed_s2d4) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s2d4_fd,
+ .allowed_access =
+ variant->allowed_s2d4,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s2d4_fd));
+
+ if (variant->allowed_s3d2) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s3d2_fd,
+ .allowed_access =
+ variant->allowed_s3d2,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s3d2_fd));
+
+ s1d3_bind_fd = open(TMP_DIR "/s3d1/s3d2/s2d3/s2d4/s1d3",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d3_bind_fd);
+
+ /* Disconnects and checks source and destination directories. */
+ EXPECT_EQ(0, test_open_rel(s1d3_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(0, test_open_rel(s1d3_bind_fd, "../..", O_DIRECTORY));
+ /* Renames to make it accessible through s3d1/s1d41 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s2d1/s2d2/s2d3",
+ AT_FDCWD, TMP_DIR "/s4d1/s2d3"));
+ EXPECT_EQ(0, test_open_rel(s1d3_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d3_bind_fd, "../..", O_DIRECTORY));
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ EXPECT_EQ(variant->expected_read_result,
+ test_open_rel(s1d3_bind_fd, "s1d41/f1", O_RDONLY));
+
+ EXPECT_EQ(variant->expected_rename_result,
+ test_renameat(s1d3_bind_fd, "s1d41/f1", s1d3_bind_fd,
+ "s1d42/f1"));
+ EXPECT_EQ(variant->expected_exchange_result,
+ test_exchangeat(s1d3_bind_fd, "s1d41/f2", s1d3_bind_fd,
+ "s1d42/f3"));
+
+ EXPECT_EQ(variant->expected_same_dir_rename_result,
+ test_renameat(s1d3_bind_fd, "s1d42/f4", s1d3_bind_fd,
+ "s1d42/f5"));
+}
+
#define LOWER_BASE TMP_DIR "/lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
--
2.51.0
^ permalink raw reply related
* [PATCH v5 0/4] Landlock: Disconnected directory handling
From: Mickaël Salaün @ 2025-11-28 17:21 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Matthieu Buffet, Mikhail Ivanov, NeilBrown, Nicolas Bouchinet,
Paul Moore, Song Liu, linux-fsdevel, linux-security-module
Hi,
This patch series fixes and test Landlock's handling of disconnected
directories.
This fifth version improve a comment and add more tests.
This approach is not perfect but it is the best we found:
https://lore.kernel.org/r/20251128.oht7Aic8nu9d@digikod.net
Previous versions:
v4: https://lore.kernel.org/r/20251126191159.3530363-1-mic@digikod.net
v3: https://lore.kernel.org/r/20250719104204.545188-1-mic@digikod.net
v2: https://lore.kernel.org/r/20250711191938.2007175-1-mic@digikod.net
v1: https://lore.kernel.org/r/20250701183812.3201231-1-mic@digikod.net
Regards,
Mickaël Salaün (3):
landlock: Fix handling of disconnected directories
landlock: Improve variable scope
selftests/landlock: Add disconnected leafs and branch test suites
Tingmao Wang (1):
selftests/landlock: Add tests for access through disconnected paths
security/landlock/errata/abi-1.h | 16 +
security/landlock/fs.c | 43 +-
tools/testing/selftests/landlock/fs_test.c | 1474 +++++++++++++++++++-
3 files changed, 1512 insertions(+), 21 deletions(-)
create mode 100644 security/landlock/errata/abi-1.h
--
2.51.0
^ permalink raw reply
* [PATCH v5 2/4] landlock: Improve variable scope
From: Mickaël Salaün @ 2025-11-28 17:21 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Matthieu Buffet, Mikhail Ivanov, NeilBrown, Nicolas Bouchinet,
Paul Moore, Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <20251128172200.760753-1-mic@digikod.net>
This is now possible thanks to the disconnected directory fix.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Cc: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- New patch extracted from the previous one.
---
security/landlock/fs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index a2ed0e76938a..8998fa4ca5f0 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -837,7 +837,6 @@ static bool is_access_to_paths_allowed(
* restriction.
*/
while (true) {
- struct dentry *parent_dentry;
const struct landlock_rule *rule;
/*
@@ -930,7 +929,9 @@ static bool is_access_to_paths_allowed(
walker_path.dentry = walker_path.mnt->mnt_root;
dget(walker_path.dentry);
} else {
- parent_dentry = dget_parent(walker_path.dentry);
+ struct dentry *const parent_dentry =
+ dget_parent(walker_path.dentry);
+
dput(walker_path.dentry);
walker_path.dentry = parent_dentry;
}
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v4 1/4] landlock: Fix handling of disconnected directories
From: Mickaël Salaün @ 2025-11-28 16:56 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Al Viro, Ben Scarlato, Christian Brauner,
Jann Horn, Jeff Xu, Justin Suess, Mikhail Ivanov, Paul Moore,
Song Liu, linux-fsdevel, linux-security-module
In-Reply-To: <adf1f57c-8f8e-45a9-922c-4e08899bf14a@maowtm.org>
On Fri, Nov 28, 2025 at 01:45:29AM +0000, Tingmao Wang wrote:
> Hi Mickaël,
>
> I think this implementation makes sense - to me this feels better than
> ignoring rules between the leaf and the mount when disconnected, given the
> interaction with domain checks. This approach is also simpler in code.
>
> However, there is one caveat which, while requiring a slightly problematic
> policy to happen in the first place, might still be a bit surprising: if,
> for some reason, there are rules "hidden" in the "real" parent of a (bind)
> mounted dir, a sandboxed program that is able to cause directories to be
> disconnected (for example, because there are more bind mounts within the
> bind mount, and the program has enough rename access (but not read/write))
> may be able to "surface" those rules and "gain access" (requires the
> existance of the already questionable "hidden" rule):
The crux of the issue is indeed the policy.
>
> root@g3ef6e4434e3a-dirty /# mkdir -p /hidden/bind1_src /bind1_dst
> /# cd hidden
> /hidden# mount --bind bind1_src /bind1_dst
> /hidden# mkdir -p bind1_src/bind2_src/dir bind1_src/bind2_dst
> /hidden# mount --bind /bind1_dst/bind2_src /bind1_dst/bind2_dst
> /hidden# echo secret > bind1_src/bind2_src/dir/secret
> /hidden# ls -la /bind1_dst/bind2_dst/dir/secret
> -rw-r--r-- 1 root root 7 Nov 28 00:49 /bind1_dst/bind2_dst/dir/secret
> /hidden# mount -t tmpfs none /hidden
> /hidden# ls .
> bind1_src/
> /hidden# ls /hidden
> /hidden# LL_FS_RO=/usr:/bin:/lib:/etc:. LL_FS_RW= LL_FS_CREATE_DELETE_REFER=./bind1_src /sandboxer bash
> ^ this attaches a read rule to a "invisible" dir
> Executing the sandboxed command...
> /hidden# cd /
> /# ls /hidden
> ls: cannot open directory '/hidden': Permission denied
> /# cd /bind1_dst/bind2_dst/dir
> /bind1_dst/bind2_dst/dir# cat secret
> cat: secret: Permission denied
> /bind1_dst/bind2_dst/dir# mv -v /bind1_dst/bind2_src/dir /bind1_dst/outside
> renamed '/bind1_dst/bind2_src/dir' -> '/bind1_dst/outside'
> /bind1_dst/bind2_dst/dir# ls ..
> ls: cannot access '..': No such file or directory
> /bind1_dst/bind2_dst/dir# cat secret
> secret
This is valid, but in this case access to secret is explicitly allowed
by the policy, even if the related path is no longer reachable.
>
> Earlier I was thinking we could make domain check for rename/links
> stricter, in that it would make sure there are no rules granting more
> access on the destination than what's granted by the "visible" rules on
> the source even if those rules are "hidden" within the fs above the
> mountpoint. This way, the application would not be able to move the
> source's parent to cause a disconnection in the first place. However, I'm
> not sure if this is worth the complication (e.g. in the case of exchange
> rename, source is also the destination, and so this check needs to also
> check that there are no "hidden" rules on the source that grants more access
> than the "visible" rules on the destination).
>
> I see another approach to mitigate this - we can disallow (return with
> -EXDEV probably) rename/links altogether when the destination (and also
> source if exchange) contains "hidden" rules that grants more access than
> the "visible" rules. However this approach would break backward
> compatibility if a sandboxer or Landlock-enlightened application creates
> such problematic policies (most likely unknowingly).
>
> Stepping back a bit, I also think it is reasonable to leave this issue as
> is and not mitigate it (maybe warn about it in some way in the docs),
> given that this can only happen if the policy is already weird (if the
> intention is to protect some file, setting an allow access rule on its
> parent, even if that parent is "hidden", is questionable).
I agree.
>
> Not sure which is best, but even with this issue this patch is probably
> still an improvement over the existing behavior (i.e. the one currently in
> mainline, where if the path is disconnected, the "hidden" rules are used
> and any "normal" rules from mnt_parent and above are ignored).
>
> Reviewed-by: Tingmao Wang <m@maowtm.org>
Thanks for the deep analysis!
>
> Kind regards,
> Tingmao
>
^ permalink raw reply
* Re: [PATCH v3 1/5] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-28 15:53 UTC (permalink / raw)
To: utilityemal77; +Cc: gnoack, jack, linux-security-module, m, mic, xandfury
In-Reply-To: <20251126122039.3832162-2-utilityemal77@gmail.com>
This is the core part of the patch series I was unsure about.
Here when a rule tagged with LANDLOCK_ADD_RULE_NO_INHERIT is
inserted, "blank" dentry rules (meaning rules with no access grants),
are inserted along with the original rule on each parent of the
original rule up to the root (but only for inodes without existing
rules). These are then (by the mark_no_inherit_ancestors call) tagged
with the has no inherit descendent marker (has_no_inherit_desc). This
is used to provide the parent directory protections.
The purpose of these blank rules is to ensure when we do a
find_rule() on any of the LANDLOCK_ADD_RULE_NO_INHERIT tagged rule's
parents, we will immediately know we have to disallow topology
changes on that inode to enforce the parent directory protections
described in the cover letter.
This lets us perform the check for these protections in O(log(n))
consistent with the red black tree's insertion time. The insertion
penalty for this is O(depth * log(n)), but this is done only at
ruleset creation time. That is somewhat more acceptable in my
opinion.
Additionally I suspect keeping all the rule tracking logic in one
data structure helps with cache locality and decreases bugs from not
keeping the rules in sync with a seperate structure. It also reduces
the LOC and complexity somewhat.
The previous v2 of this patch used a complex (and buggy I found after
running the v3 test suite on it) xarray to track this. The check for
parent directory protections was O(n) and insertion was O(n * depth).
My questions for reviewers:
* Is it acceptable in this case for landlock to automatically and
silently insert rules that the user didn't explicity declare?
* Should these protections instead be implemented in a seperate
data structure?
* Is the performance cost for the current implementation
acceptable?
Normal landlock insertion and checking is O(log(n)). For rules with
this tag, checking is still O(log(n)) but insertion is O(depth *
log(n)).
Kind Regards,
Justin Suess
^ 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