* Re: [PATCH v2 0/7] fs/9p: Reuse inode based on path (in addition to qid)
From: Christian Schoenebeck @ 2025-09-29 13:06 UTC (permalink / raw)
To: Mickaël Salaün, Dominique Martinet, qemu-devel,
Greg Kurz
Cc: Eric Van Hensbergen, Latchesar Ionkov, v9fs, Günther Noack,
linux-security-module, Jan Kara, Amir Goldstein,
Matthew Bobrowski, Al Viro, Christian Brauner, linux-fsdevel,
qemu-devel, Tingmao Wang
In-Reply-To: <f1228978-dac0-4d1a-a820-5ac9562675d0@maowtm.org>
On Sunday, September 21, 2025 6:24:49 PM CEST Tingmao Wang wrote:
> On 9/17/25 16:00, Mickaël Salaün wrote:
[...]
Hi Greg,
I'd appreciate comments from your side as well, as you are much on longer on
the QEMU 9p front than me.
I know you won't have the time to read up on the entire thread so I try to
summarize: basically this is yet another user-after-unlink issue, this time on
directories instead of files.
> So I did some quick debugging and realized that I had a wrong
> understanding of how fids relates to opened files on the host, under QEMU.
> It turns out that in QEMU's 9p server implementation, a fid does not
> actually correspond to any opened file descriptors - it merely represents
> a (string-based) path that QEMU stores internally. It only opens the
> actual file if the client actually does an T(l)open, which is in fact
> separate from acquiring the fid with T(l)walk. The reason why renaming
> file/dirs from the client doesn't break those fids is because QEMU will
> actually fix those paths when a rename request is processed - c.f.
> v9fs_fix_fid_paths [1].
Correct, that's based on what the 9p protocols define: a FID does not exactly
translate to what a file handle is on a local system. Even after acquiring a
new FID by sending a Twalk request, subsequently client would still need to
send a Topen for server to actually open that file/directory.
And yes, QEMU's 9p server "fixes" the path string of a FID if it was moved
upon client request. If the move happened on host side, outside of server's
knowledge, then this won't happen ATM and hence it would break your use
case.
> It turns out that even if a guest process opens the file with O_PATH, that
> file descriptor does not cause an actual Topen, and therefore QEMU does
> not open the file on the host, and later on reopening that fd with another
> mode (via e.g. open("/proc/self/fd/...", O_RDONLY)) will fail if the file
> has moved on the host without QEMU's knowledge. Also, openat will fail if
> provided with a dir fd that "points" to a moved directory, regardless of
> whether the fd is opened with O_PATH or not, since path walk in QEMU is
> completely string-based and does not actually issue openat on the host fs
> [2].
I don't think the problem here is the string based walk per se, but rather
that the string based walk always starts from the export root:
https://github.com/qemu/qemu/blob/4975b64efb5aa4248cbc3760312bbe08d6e71638/hw/9pfs/9p-local.c#L64
I guess that's something that could be changed in QEMU such that the walk
starts from FID's fs point, as the code already uses openat() to walk relative
to a file descriptor (for security reasons actually), Greg?
That alone would still not fix your use case though: things being moved on
host side. For this to work, it would require to already have a fd open on
host for the FID. This could be done by server for each FID as you suggested,
or it could be done by client by opening the FID.
Also keep in mind: once the open file descriptor limit on host is exhausted,
QEMU is forced to close older open file desciptors to keep the QEMU process
alive. So this might still break what you are trying to achieve there.
Having said that, I wonder whether it'd be simpler for server to track for
file tree changes (inotify API) and fix the pathes accordingly for host
side changes as well?
/Christian
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: Add hash chain signature support for arbitrary maps
From: Quentin Monnet @ 2025-09-29 9:25 UTC (permalink / raw)
To: Blaise Boscaccy, bpf, linux-security-module, kpsingh, paul, kys,
ast, daniel, andrii, James.Bottomley, wufan
In-Reply-To: <20250926203111.1305999-2-bboscaccy@linux.microsoft.com>
2025-09-26 13:30 UTC-0700 ~ Blaise Boscaccy <bboscaccy@linux.microsoft.com>
> This patch introduces hash chain support for signature verification of
> arbitrary bpf map objects which was described here:
> https://lore.kernel.org/linux-security-module/20250721211958.1881379-1-kpsingh@kernel.org/
>
> The UAPI is extended to allow for in-kernel checking of maps passed in
> via the fd_array. A hash chain is constructed from the maps, in order
> specified by the signature_maps field. The hash chain is terminated
> with the hash of the program itself.
>
> Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com>
> ---
> include/uapi/linux/bpf.h | 6 ++
> kernel/bpf/syscall.c | 73 ++++++++++++++++++-
> .../bpf/bpftool/Documentation/bpftool-gen.rst | 7 +-
> tools/bpf/bpftool/gen.c | 27 ++++++-
> tools/bpf/bpftool/main.c | 9 ++-
> tools/bpf/bpftool/main.h | 1 +
> tools/bpf/bpftool/sign.c | 17 ++++-
> tools/include/uapi/linux/bpf.h | 6 ++
> tools/lib/bpf/libbpf.h | 3 +-
> tools/lib/bpf/skel_internal.h | 6 +-
> 10 files changed, 143 insertions(+), 12 deletions(-)
>
[...]
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> index d0a36f442db72..b632ab87adf20 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> @@ -16,7 +16,7 @@ SYNOPSIS
>
> **bpftool** [*OPTIONS*] **gen** *COMMAND*
>
> -*OPTIONS* := { |COMMON_OPTIONS| | { **-L** | **--use-loader** } | [ { **-S** | **--sign** } {**-k** <private_key.pem>} **-i** <certificate.x509> ] }
> +*OPTIONS* := { |COMMON_OPTIONS| | { **-L** | **--use-loader** } | [ { **-S** | **--sign** } { **-M** | **--sign-maps** } {**-k** <private_key.pem>} **-i** <certificate.x509> ] }
>
> *COMMAND* := { **object** | **skeleton** | **help** }
>
> @@ -190,6 +190,11 @@ OPTIONS
> For skeletons, generate a signed skeleton. This option must be used with
> **-k** and **-i**. Using this flag implicitly enables **--use-loader**.
>
> +-M --sign-maps
> + For skeletons, generate a signed skeleton that includes a hash chain for the
> + skeletons maps. This option must be used with **-k** and **-i**. Using this
> + flag implicitly enables **--use-loader** and **--sign**.
> +
Hi! Pardon my ignorance, I haven't followed all the details of the
discussions around signing. Is there a use case for signing the programs
only (using -S) without signing the maps (using -M)? Or should we
consider collapsing maps signing under the existing -S option?
If you do keep the new option, would you mind updating the bash
completion file, please? Simply adding the long form like this:
------
diff --git i/tools/bpf/bpftool/bash-completion/bpftool w/tools/bpf/bpftool/bash-completion/bpftool
index 53bcfeb1a76e..f8c217f09989 100644
--- i/tools/bpf/bpftool/bash-completion/bpftool
+++ w/tools/bpf/bpftool/bash-completion/bpftool
@@ -262,7 +262,7 @@ _bpftool()
# Deal with options
if [[ ${words[cword]} == -* ]]; then
local c='--version --json --pretty --bpffs --mapcompat --debug \
- --use-loader --base-btf --sign -i -k'
+ --use-loader --base-btf --sign --sign-maps -i -k'
COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
return 0
fi
------
Other than that, the changes for bpftool look OK from my side. I'd
probably split the patch further into kernel/libbpf/bpftool changes, but
that's your call.
Thanks,
Quentin
^ permalink raw reply related
* Re: [syzbot] [fs?] BUG: sleeping function called from invalid context in hook_sb_delete
From: Christian Brauner @ 2025-09-29 8:51 UTC (permalink / raw)
To: Jan Kara
Cc: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro,
Mickaël Salaün, linux-security-module,
Günther Noack
In-Reply-To: <fnxbqe3nlcptxqcs7tkt6qnacupkxu2xn4duwc6g6n2bk4tstb@hi2gl5cwishr>
On Wed, Sep 24, 2025 at 01:05:03PM +0200, Jan Kara wrote:
> Hello!
>
> Added Landlock guys to CC since this is a bug in Landlock.
>
> On Tue 23-09-25 15:59:37, syzbot wrote:
> > syzbot found the following issue on:
> >
> > HEAD commit: ce7f1a983b07 Add linux-next specific files for 20250923
> > git tree: linux-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=118724e2580000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=1be6fa3d47bce66e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=12479ae15958fc3f54ec
> > compiler: Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1376e27c580000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=136e78e2580000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/c30be6f36c31/disk-ce7f1a98.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/ae9ea347d4d8/vmlinux-ce7f1a98.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/d59682a4f33c/bzImage-ce7f1a98.xz
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+12479ae15958fc3f54ec@syzkaller.appspotmail.com
> >
> > BUG: sleeping function called from invalid context at fs/inode.c:1928
>
> The first catch from the new might_sleep() annotations in iput().
>
> > in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 6028, name: syz.0.17
> > preempt_count: 1, expected: 0
> > RCU nest depth: 0, expected: 0
> > 2 locks held by syz.0.17/6028:
> > #0: ffff8880326bc0e0 (&type->s_umount_key#48){+.+.}-{4:4}, at: __super_lock fs/super.c:57 [inline]
> > #0: ffff8880326bc0e0 (&type->s_umount_key#48){+.+.}-{4:4}, at: __super_lock_excl fs/super.c:72 [inline]
> > #0: ffff8880326bc0e0 (&type->s_umount_key#48){+.+.}-{4:4}, at: deactivate_super+0xa9/0xe0 fs/super.c:505
> > #1: ffff8880326bc998 (&s->s_inode_list_lock){+.+.}-{3:3}, at: spin_lock include/linux/spinlock.h:351 [inline]
> > #1: ffff8880326bc998 (&s->s_inode_list_lock){+.+.}-{3:3}, at: hook_sb_delete+0xae/0xbd0 security/landlock/fs.c:1405
> > Preemption disabled at:
> > [<0000000000000000>] 0x0
> > CPU: 0 UID: 0 PID: 6028 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full)
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> > Call Trace:
> > <TASK>
> > dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
> > __might_resched+0x495/0x610 kernel/sched/core.c:8960
> > iput+0x2b/0xc50 fs/inode.c:1928
> > hook_sb_delete+0x6b5/0xbd0 security/landlock/fs.c:1468
>
> Indeed looks like a bug because we can call iput() while holding
> sb->s_inode_list_lock in one case in hook_sb_delete().
Very nice that the annotations help finding this!
^ permalink raw reply
* [PATCH v2 9/9] tpm-buf: Build PCR extend commands
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Build and append TPM_ORD_EXTEND and TPM2_CC_PCR_EXTEND command bodies
with the two new functions:
1. tpm1_buf_append_extend()
2. tpm2_buf_append_pcr_extend()
These changes make the fallback more informative of the situation, as the
underlying programming error is catched at the call site, instead of
masking it as a tpm_transmit() failure.
Further, decoupling the build of the command bodies for extending PCRs
will be mandatory for the Trenchboot early boot code.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 67 +++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm1-cmd.c | 15 +++++----
drivers/char/tpm/tpm2-cmd.c | 13 ++++---
include/linux/tpm.h | 4 +++
include/linux/tpm_command.h | 5 +--
5 files changed, 88 insertions(+), 16 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index c2bf7556cb23..d54cc4273e8c 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -243,4 +243,71 @@ u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
}
EXPORT_SYMBOL_GPL(tpm_buf_read_u32);
+static bool tpm1_buf_is_command(struct tpm_buf *buf, u32 ordinal)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return !(buf->flags & TPM_BUF_TPM2B) &&
+ be16_to_cpu(head->tag) == TPM_TAG_RQU_COMMAND &&
+ be32_to_cpu(head->ordinal) == ordinal;
+}
+
+/**
+ * tpm1_buf_append_extend() - Append command body for TPM_Extend
+ * @buf: &tpm_buf instance
+ * @pcr_idx: index of the PCR
+ * @hash: SHA1 hash
+ */
+void tpm1_buf_append_extend(struct tpm_buf *buf, u32 pcr_idx, const u8 *hash)
+{
+ if (buf->flags & TPM_BUF_INVALID)
+ return;
+
+ if (!tpm1_buf_is_command(buf, TPM_ORD_EXTEND)) {
+ WARN(1, "tpm_buf: invalid TPM_Extend command\n");
+ buf->flags |= TPM_BUF_INVALID;
+ return;
+ }
+
+ tpm_buf_append_u32(buf, pcr_idx);
+ tpm_buf_append(buf, hash, TPM_DIGEST_SIZE);
+}
+
+static bool tpm2_buf_is_command(struct tpm_buf *buf, u32 ordinal)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+ u16 tag = be16_to_cpu(head->tag);
+
+ return !(buf->flags & TPM_BUF_TPM2B) &&
+ (tag == TPM2_ST_SESSIONS || tag == TPM2_ST_NO_SESSIONS) &&
+ be32_to_cpu(head->ordinal) == ordinal;
+}
+
+/**
+ * tpm2_buf_append_pcr_extend() - Append command body for TPM2_PCR_Extend
+ * @buf: &tpm_buf instance
+ * @digests: list of PCR digests
+ * @banks: PCR bank descriptors
+ * @nr_banks: number of PCR banks
+ */
+void tpm2_buf_append_pcr_extend(struct tpm_buf *buf, struct tpm_digest *digests,
+ struct tpm_bank_info *banks,
+ unsigned int nr_banks)
+{
+ int i;
+ if (buf->flags & TPM_BUF_INVALID)
+ return;
+
+ if (!tpm2_buf_is_command(buf, TPM2_CC_PCR_EXTEND)) {
+ WARN(1, "tpm_buf: invalid TPM2_PCR_Extend command\n");
+ buf->flags |= TPM_BUF_INVALID;
+ return;
+ }
+
+ tpm_buf_append_u32(buf, nr_banks);
+ for (i = 0; i < nr_banks; i++) {
+ tpm_buf_append_u16(buf, digests[i].alg_id);
+ tpm_buf_append(buf, digests[i].digest, banks[i].digest_size);
+ }
+}
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 5c49bdff33de..4f1af8beeed4 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -18,8 +18,8 @@
#include <linux/mutex.h>
#include <linux/spinlock.h>
#include <linux/freezer.h>
+#include <linux/tpm_command.h>
#include <linux/tpm_eventlog.h>
-
#include "tpm.h"
#define TPM_MAX_ORDINAL 243
@@ -459,21 +459,23 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
return 0;
}
-#define TPM_ORD_PCR_EXTEND 20
int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
const char *log_msg)
{
struct tpm_buf buf;
int rc;
- rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
+ rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_EXTEND);
if (rc)
return rc;
- tpm_buf_append_u32(&buf, pcr_idx);
- tpm_buf_append(&buf, hash, TPM_DIGEST_SIZE);
+ tpm1_buf_append_extend(&buf, pcr_idx, hash);
+
+ if (buf.flags & TPM_BUF_INVALID)
+ rc = -EINVAL;
+ else
+ rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, log_msg);
- rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, log_msg);
tpm_buf_destroy(&buf);
return rc;
}
@@ -511,7 +513,6 @@ ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
}
EXPORT_SYMBOL_GPL(tpm1_getcap);
-#define TPM_ORD_GET_RANDOM 70
struct tpm1_get_random_out {
__be32 rng_data_len;
u8 rng_data[TPM_MAX_RNG_DATA];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 4248e59265aa..09ea4a090475 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -171,7 +171,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
{
struct tpm_buf buf;
int rc;
- int i;
if (!disable_pcr_integrity) {
rc = tpm2_start_auth_session(chip);
@@ -194,12 +193,12 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_auth(chip, &buf, NULL, 0);
}
- tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+ tpm2_buf_append_pcr_extend(&buf, digests, chip->allocated_banks,
+ chip->nr_allocated_banks);
- for (i = 0; i < chip->nr_allocated_banks; i++) {
- tpm_buf_append_u16(&buf, digests[i].alg_id);
- tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
- chip->allocated_banks[i].digest_size);
+ if (buf.flags & TPM_BUF_INVALID) {
+ rc = -EINVAL;
+ goto out;
}
if (!disable_pcr_integrity)
@@ -208,8 +207,8 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
if (!disable_pcr_integrity)
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
+out:
tpm_buf_destroy(&buf);
-
return rc;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b2d89df70c18..6c7349dce871 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -424,6 +424,10 @@ u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle);
+void tpm1_buf_append_extend(struct tpm_buf *buf, u32 pcr_idx, const u8 *hash);
+void tpm2_buf_append_pcr_extend(struct tpm_buf *buf, struct tpm_digest *digests,
+ struct tpm_bank_info *banks,
+ unsigned int nr_banks);
/*
* Check if TPM device is in the firmware upgrade mode.
diff --git a/include/linux/tpm_command.h b/include/linux/tpm_command.h
index f5c03e9c3913..02038972a05f 100644
--- a/include/linux/tpm_command.h
+++ b/include/linux/tpm_command.h
@@ -16,11 +16,12 @@
#define TPM_TAG_RSP_AUTH2_COMMAND 198
/* Command Ordinals */
-#define TPM_ORD_GETRANDOM 70
-#define TPM_ORD_OSAP 11
#define TPM_ORD_OIAP 10
+#define TPM_ORD_OSAP 11
+#define TPM_ORD_EXTEND 20
#define TPM_ORD_SEAL 23
#define TPM_ORD_UNSEAL 24
+#define TPM_ORD_GET_RANDOM 70
/* Other constants */
#define SRKHANDLE 0x40000000
--
2.39.5
^ permalink raw reply related
* [PATCH v2 8/9] tpm-buf: Remove chip parameeter from tpm_buf_append_handle
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Remove chip parameter from tpm_buf_append_handle() in order to maintain
decoupled state with tpm-buf. This is mandatory change in order to re-use
the module in early boot code of Trenchboot, and the binding itself brings
no benefit. Use WARN like in other functions, as the error condition can
happen only as a net effect of a trivial programming mistake.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 6 ++----
drivers/char/tpm/tpm2-cmd.c | 2 +-
drivers/char/tpm/tpm2-sessions.c | 2 +-
include/linux/tpm.h | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 5526f548b4de..c2bf7556cb23 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -147,21 +147,19 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
/**
* tpm_buf_append_handle() - Add a handle
- * @chip: &tpm_chip instance
* @buf: &tpm_buf instance
* @handle: a TPM object handle
*
* Add a handle to the buffer, and increase the count tracking the number of
* handles in the command buffer. Works only for command buffers.
*/
-void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
+void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle)
{
if (buf->flags & TPM_BUF_INVALID)
return;
if (buf->flags & TPM_BUF_TPM2B) {
- buf->flags |= TPM_BUF_INVALID;
- dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+ WARN(1, "tpm-buf: invalid type: TPM2B\n");
return;
}
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index eef324e61308..4248e59265aa 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -190,7 +190,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
- tpm_buf_append_handle(chip, &buf, pcr_idx);
+ tpm_buf_append_handle(&buf, pcr_idx);
tpm_buf_append_auth(chip, &buf, NULL, 0);
}
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 13f019d1312a..bbc05f0997a8 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -232,7 +232,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
#endif
if (!tpm2_chip_auth(chip)) {
- tpm_buf_append_handle(chip, buf, handle);
+ tpm_buf_append_handle(buf, handle);
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5283f32781c4..b2d89df70c18 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -423,7 +423,7 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
-void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle);
+void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle);
/*
* Check if TPM device is in the firmware upgrade mode.
--
2.39.5
^ permalink raw reply related
* [PATCH v2 7/9] tpm-buf: check for corruption in tpm_buf_append_handle()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW into TPM_BUF_INVALID
flag because semantically they are identical.
Test and set TPM_BUF_INVALID in tpm_buf_append_handle() following the
pattern from other functions in tpm-buf.c.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 14 ++++++++------
include/linux/tpm.h | 8 +++-----
security/keys/trusted-keys/trusted_tpm2.c | 6 +++---
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index dc882fc9fa9e..5526f548b4de 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -104,13 +104,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
*/
void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
{
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
+ if (buf->flags & TPM_BUF_INVALID)
return;
if ((buf->length + new_length) > PAGE_SIZE) {
WARN(1, "tpm_buf: write overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
@@ -157,7 +156,11 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
*/
void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
{
+ if (buf->flags & TPM_BUF_INVALID)
+ return;
+
if (buf->flags & TPM_BUF_TPM2B) {
+ buf->flags |= TPM_BUF_INVALID;
dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
return;
}
@@ -177,14 +180,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
{
off_t next_offset;
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
+ if (buf->flags & TPM_BUF_INVALID)
return;
next_offset = *offset + count;
if (next_offset > buf->length) {
WARN(1, "tpm_buf: read out of boundary\n");
- buf->flags |= TPM_BUF_BOUNDARY_ERROR;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index e72e7657faa2..5283f32781c4 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -366,12 +366,10 @@ struct tpm_header {
} __packed;
enum tpm_buf_flags {
- /* the capacity exceeded: */
- TPM_BUF_OVERFLOW = BIT(0),
/* TPM2B format: */
- TPM_BUF_TPM2B = BIT(1),
- /* read out of boundary: */
- TPM_BUF_BOUNDARY_ERROR = BIT(2),
+ TPM_BUF_TPM2B = BIT(0),
+ /* The buffer is in invalid and unusable state: */
+ TPM_BUF_INVALID = BIT(1),
};
/*
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 8e3b283a59b2..119d5152c0db 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -295,7 +295,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
/* creation PCR */
tpm_buf_append_u32(&buf, 0);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
@@ -308,7 +308,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
blob_len = tpm_buf_read_u32(&buf, &offset);
- if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
+ if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
goto out;
}
@@ -414,7 +414,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
tpm_buf_append(&buf, blob, blob_len);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
--
2.39.5
^ permalink raw reply related
* [PATCH v2 6/9] KEYS: trusted: Open code tpm2_buf_append()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Jonathan McDowell,
David Howells, Jarkko Sakkinen, Paul Moore, James Morris,
Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
tpm2_buf_append_auth() has only single call site and most of its parameters
are redundant. Open code it to the call site. Remove illegit FIXME comment
as there is no categorized bug and replace it with more sane comment about
implementation (i.e. "non-opionated inline comment").
Reviewed-by: Jonathan McDowell <noodles@earth.li>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- No changes.
---
security/keys/trusted-keys/trusted_tpm2.c | 51 ++++-------------------
1 file changed, 9 insertions(+), 42 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c414a7006d78..8e3b283a59b2 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -198,36 +198,6 @@ int tpm2_key_priv(void *context, size_t hdrlen,
return 0;
}
-/**
- * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
- *
- * @buf: an allocated tpm_buf instance
- * @session_handle: session handle
- * @nonce: the session nonce, may be NULL if not used
- * @nonce_len: the session nonce length, may be 0 if not used
- * @attributes: the session attributes
- * @hmac: the session HMAC or password, may be NULL if not used
- * @hmac_len: the session HMAC or password length, maybe 0 if not used
- */
-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
- const u8 *nonce, u16 nonce_len,
- u8 attributes,
- const u8 *hmac, u16 hmac_len)
-{
- tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
- tpm_buf_append_u32(buf, session_handle);
- tpm_buf_append_u16(buf, nonce_len);
-
- if (nonce && nonce_len)
- tpm_buf_append(buf, nonce, nonce_len);
-
- tpm_buf_append_u8(buf, attributes);
- tpm_buf_append_u16(buf, hmac_len);
-
- if (hmac && hmac_len)
- tpm_buf_append(buf, hmac, hmac_len);
-}
-
/**
* tpm2_seal_trusted() - seal the payload of a trusted key
*
@@ -507,19 +477,16 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
options->blobauth_len);
} else {
/*
- * FIXME: The policy session was generated outside the
- * kernel so we don't known the nonce and thus can't
- * calculate a HMAC on it. Therefore, the user can
- * only really use TPM2_PolicyPassword and we must
- * send down the plain text password, which could be
- * intercepted. We can still encrypt the returned
- * key, but that's small comfort since the interposer
- * could repeat our actions with the exfiltrated
- * password.
+ * The policy session is generated outside the kernel, and thus
+ * the password will end up being unencrypted on the bus, as
+ * HMAC nonce cannot be calculated for it.
*/
- tpm2_buf_append_auth(&buf, options->policyhandle,
- NULL /* nonce */, 0, 0,
- options->blobauth, options->blobauth_len);
+ tpm_buf_append_u32(&buf, 9 + options->blobauth_len);
+ tpm_buf_append_u32(&buf, options->policyhandle);
+ tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u8(&buf, 0);
+ tpm_buf_append_u16(&buf, options->blobauth_len);
+ tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
if (tpm2_chip_auth(chip)) {
tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
} else {
--
2.39.5
^ permalink raw reply related
* [PATCH v2 5/9] tpm2-sessions: Umask tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Open code tpm_buf_append_hmac_session_opt() in order to unmask the code
paths in the call sites of tpm_buf_append_hmac_session().
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Uncorrupt the patch.
---
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
include/linux/tpm.h | 23 -----------------------
security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
3 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c182a07b70de..eef324e61308 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -257,9 +257,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
do {
tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
- | TPM2_SA_CONTINUE_SESSION,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf,
+ TPM2_SA_ENCRYPT |
+ TPM2_SA_CONTINUE_SESSION,
+ NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
tpm_buf_append_u16(&buf, num_bytes);
tpm_buf_fill_hmac_session(chip, &buf);
err = tpm_transmit_cmd(chip, &buf,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 1fa02e18e688..e72e7657faa2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -532,29 +532,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
u8 *passphrase, int passphraselen);
-static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
- struct tpm_buf *buf,
- u8 attributes,
- u8 *passphrase,
- int passphraselen)
-{
- struct tpm_header *head;
- int offset;
-
- if (tpm2_chip_auth(chip)) {
- tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
- } else {
- offset = buf->handles * 4 + TPM_HEADER_SIZE;
- head = (struct tpm_header *)buf->data;
-
- /*
- * If the only sessions are optional, the command tag must change to
- * TPM2_ST_NO_SESSIONS.
- */
- if (tpm_buf_length(buf) == offset)
- head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
- }
-}
#ifdef CONFIG_TCG_TPM2_HMAC
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index e165b117bbca..c414a7006d78 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -482,8 +482,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
struct trusted_key_options *options,
u32 blob_handle)
{
+ struct tpm_header *head;
struct tpm_buf buf;
u16 data_len;
+ int offset;
u8 *data;
int rc;
@@ -518,8 +520,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
tpm2_buf_append_auth(&buf, options->policyhandle,
NULL /* nonce */, 0, 0,
options->blobauth, options->blobauth_len);
- tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
- NULL, 0);
+ if (tpm2_chip_auth(chip)) {
+ tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
+ } else {
+ offset = buf.handles * 4 + TPM_HEADER_SIZE;
+ head = (struct tpm_header *)buf.data;
+ if (tpm_buf_length(&buf) == offset)
+ head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+ }
}
tpm_buf_fill_hmac_session(chip, &buf);
--
2.39.5
^ permalink raw reply related
* [PATCH v2 4/9] tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Roberto Sassu, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
In a previous bug fix, 'attributes' was added by mistake to
tpm_buf_append_auth(). Remove the parameter.
Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Uncorrupt the patch.
---
drivers/char/tpm/tpm2-cmd.c | 2 +-
drivers/char/tpm/tpm2-sessions.c | 5 ++---
include/linux/tpm.h | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e416cc8705e3..c182a07b70de 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -191,7 +191,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
} else {
tpm_buf_append_handle(chip, &buf, pcr_idx);
- tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+ tpm_buf_append_auth(chip, &buf, NULL, 0);
}
tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6d03c224e6b2..13f019d1312a 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -266,7 +266,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
EXPORT_SYMBOL_GPL(tpm_buf_append_name);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
- u8 attributes, u8 *passphrase, int passphrase_len)
+ u8 *passphrase, int passphrase_len)
{
/* offset tells us where the sessions area begins */
int offset = buf->handles * 4 + TPM_HEADER_SIZE;
@@ -327,8 +327,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
#endif
if (!tpm2_chip_auth(chip)) {
- tpm_buf_append_auth(chip, buf, attributes, passphrase,
- passphrase_len);
+ tpm_buf_append_auth(chip, buf, passphrase, passphrase_len);
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 51846317d662..1fa02e18e688 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -531,7 +531,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
u8 attributes, u8 *passphrase,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
- u8 attributes, u8 *passphrase, int passphraselen);
+ u8 *passphrase, int passphraselen);
static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
struct tpm_buf *buf,
u8 attributes,
--
2.39.5
^ permalink raw reply related
* [PATCH v2 3/9] KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, David Howells,
Jarkko Sakkinen, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Use tpm_ret_to_err() to transmute TPM return codes in trusted_tpm2.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- New patch split out from the fix.
---
security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 024be262702f..e165b117bbca 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -348,25 +348,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
}
blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
+ if (blob_len < 0)
+ rc = blob_len;
out:
tpm_buf_destroy(&sized);
tpm_buf_destroy(&buf);
- if (rc > 0) {
- if (tpm2_rc_value(rc) == TPM2_RC_HASH)
- rc = -EINVAL;
- else
- rc = -EPERM;
- }
- if (blob_len < 0)
- rc = blob_len;
- else
+ if (!rc)
payload->blob_len = blob_len;
out_put:
tpm_put_ops(chip);
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -468,10 +462,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
kfree(blob);
tpm_buf_destroy(&buf);
- if (rc > 0)
- rc = -EPERM;
-
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -534,8 +525,6 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
tpm_buf_fill_hmac_session(chip, &buf);
rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
rc = tpm_buf_check_hmac_response(chip, &buf, rc);
- if (rc > 0)
- rc = -EPERM;
if (!rc) {
data_len = be16_to_cpup(
@@ -568,7 +557,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
out:
tpm_buf_destroy(&buf);
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -600,6 +589,5 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
out:
tpm_put_ops(chip);
-
- return rc;
+ return tpm_ret_to_err(rc);
}
--
2.39.5
^ permalink raw reply related
* [PATCH v2 2/9] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Peter Huewe,
Jarkko Sakkinen, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Stefano Garzarella, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Using -EFAULT as the tpm_ret_to_err() fallback error code causes makes it
incompatible on how trusted keys transmute TPM return codes.
Change the fallback as -EPERM in order to gain compatibility with trusted
keys. In addition, map TPM_RC_HASH to -EINVAL in order to be compatible
with tpm2_seal_trusted() return values.
Fixes: 539fbab37881 ("tpm: Mask TPM RC in tpm2_start_auth_session()")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- Split trusted_tpm2 change to a separate patch.
---
include/linux/tpm.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index fc7df87dfb9a..51846317d662 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -453,8 +453,10 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
return 0;
case TPM2_RC_SESSION_MEMORY:
return -ENOMEM;
+ case TPM2_RC_HASH:
+ return -EINVAL;
default:
- return -EFAULT;
+ return -EPERM;
}
}
--
2.39.5
^ permalink raw reply related
* [PATCH v2 1/9] tpm: cap PCR bank in tpm2_get_pcr_allocations()
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, Roberto Sassu,
Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929035938.1773341-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
tpm2_get_pcr_allocation() does not cap any upper limit for the number of
banks received from external hardware device. This could lead into resource
over-consumption with a fauly TPM device.
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
v2:
- A new patch.
---
drivers/char/tpm/tpm-chip.c | 13 +++++++++----
drivers/char/tpm/tpm.h | 1 -
drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
drivers/char/tpm/tpm2-cmd.c | 8 +++-----
include/linux/tpm.h | 18 ++++++++----------
5 files changed, 20 insertions(+), 45 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 687f6d8cd601..9a6538f76f50 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
static int tpm_get_pcr_allocation(struct tpm_chip *chip)
{
- int rc;
+ int rc = 0;
if (tpm_is_firmware_upgrade(chip))
return 0;
- rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
- tpm2_get_pcr_allocation(chip) :
- tpm1_get_pcr_allocation(chip);
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+ chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+ chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
+ chip->nr_allocated_banks = 1;
+ } else {
+ rc = tpm2_get_pcr_allocation(chip);
+ }
if (rc > 0)
return -ENODEV;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 57ef8589f5f5..769fa6b00c54 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
const char *desc, size_t min_cap_length);
int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
-int tpm1_get_pcr_allocation(struct tpm_chip *chip);
unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
int tpm_pm_suspend(struct device *dev);
int tpm_pm_resume(struct device *dev);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index cf64c7385105..5c49bdff33de 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
return rc;
}
-
-/**
- * tpm1_get_pcr_allocation() - initialize the allocated bank
- * @chip: TPM chip to use.
- *
- * The function initializes the SHA1 allocated bank to extend PCR
- *
- * Return:
- * * 0 on success,
- * * < 0 on error.
- */
-int tpm1_get_pcr_allocation(struct tpm_chip *chip)
-{
- chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks)
- return -ENOMEM;
-
- chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
- chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
- chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
- chip->nr_allocated_banks = 1;
-
- return 0;
-}
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 7d77f6fbc152..e416cc8705e3 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
nr_possible_banks = be32_to_cpup(
(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
-
- chip->allocated_banks = kcalloc(nr_possible_banks,
- sizeof(*chip->allocated_banks),
- GFP_KERNEL);
- if (!chip->allocated_banks) {
+ if (nr_possible_banks > TPM2_MAX_BANKS) {
+ pr_err("tpm:: unexpected large number of banks: %u > %u",
+ nr_possible_banks, TPM2_MAX_BANKS);
rc = -ENOMEM;
goto out;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 900c81a2bc41..fc7df87dfb9a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -27,7 +27,12 @@
#include <crypto/aes.h>
#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
-#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
+#define TPM_HEADER_SIZE 10
+
+#define TPM2_PLATFORM_PCR 24
+#define TPM2_PCR_SELECT_MIN 3
+#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
+#define TPM2_MAX_BANKS 4
struct tpm_chip;
struct trusted_key_payload;
@@ -69,7 +74,7 @@ enum tpm2_curves {
struct tpm_digest {
u16 alg_id;
- u8 digest[TPM_MAX_DIGEST_SIZE];
+ u8 digest[TPM2_MAX_DIGEST_SIZE];
} __packed;
struct tpm_bank_info {
@@ -190,7 +195,7 @@ struct tpm_chip {
unsigned int groups_cnt;
u32 nr_allocated_banks;
- struct tpm_bank_info *allocated_banks;
+ struct tpm_bank_info allocated_banks[TPM2_MAX_BANKS];
#ifdef CONFIG_ACPI
acpi_handle acpi_dev_handle;
char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -217,13 +222,6 @@ struct tpm_chip {
#endif
};
-#define TPM_HEADER_SIZE 10
-
-enum tpm2_const {
- TPM2_PLATFORM_PCR = 24,
- TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
-};
-
enum tpm2_timeouts {
TPM2_TIMEOUT_A = 750,
TPM2_TIMEOUT_B = 4000,
--
2.39.5
^ permalink raw reply related
* [PATCH v2 0/9] tpm: Decouple PCR extend from driver
From: Jarkko Sakkinen @ 2025-09-29 3:59 UTC (permalink / raw)
To: linux-integrity
Cc: dpsmith, ross.philipson, Jarkko Sakkinen, David Howells,
Paul Moore, James Morris, Serge E. Hallyn,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
Decouple tpm2-sessions enough from implementation so that building for PCR
extend commands can be decoupled from rest of the implementation. This is
a mandatory for Trenchboot series, and including all these changes for
that series would over-complicate it.
This is first part of refactorizations for make grounds for Trenchboot,
and still aimed for 6.18. The second part includes robustness updates
for tpm-buf.
v2:
- While including fixes from v1, this patch set has a refocus in order to
do minimal changes to make code base more compatible Trenchboot.
Jarkko Sakkinen (9):
tpm: cap PCR bank in tpm2_get_pcr_allocations()
tpm: Use -EPERM as fallback error code in tpm_ret_to_err
KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
tpm2-sessions: Umask tpm_buf_append_hmac_session()
KEYS: trusted: Open code tpm2_buf_append()
tpm-buf: check for corruption in tpm_buf_append_handle()
tpm-buf: Remove chip parameeter from tpm_buf_append_handle
tpm-buf: Build PCR extend commands
drivers/char/tpm/tpm-buf.c | 85 +++++++++++++++++---
drivers/char/tpm/tpm-chip.c | 13 +++-
drivers/char/tpm/tpm.h | 1 -
drivers/char/tpm/tpm1-cmd.c | 40 ++--------
drivers/char/tpm/tpm2-cmd.c | 39 ++++++----
drivers/char/tpm/tpm2-sessions.c | 7 +-
include/linux/tpm.h | 61 +++++----------
include/linux/tpm_command.h | 5 +-
security/keys/trusted-keys/trusted_tpm2.c | 95 +++++++----------------
9 files changed, 170 insertions(+), 176 deletions(-)
--
2.39.5
^ permalink raw reply
* Re: [RFC PATCH v2] lsm,selinux: introduce LSM_ATTR_UNSHARE and wire it up for SELinux
From: Serge E. Hallyn @ 2025-09-29 3:35 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-security-module, selinux, paul, omosnace, john.johansen,
serge, casey
In-Reply-To: <20250918135904.9997-2-stephen.smalley.work@gmail.com>
On Thu, Sep 18, 2025 at 09:59:05AM -0400, Stephen Smalley wrote:
> RFC-only, will ultimately split the LSM-only changes to their own
> patch for submission. I have now tested this with the corresponding
> selinux userspace change that you can find at
> https://lore.kernel.org/selinux/20250918135118.9896-2-stephen.smalley.work@gmail.com/
> and also verified that my modified systemd-nspawn still works when
> starting containers with their own SELinux namespace.
>
> This defines a new LSM_ATTR_UNSHARE attribute for the
> lsm_set_self_attr(2) system call and wires it up for SELinux to invoke
> the underlying function for unsharing the SELinux namespace. As with
> the selinuxfs interface, this immediately unshares the SELinux
> namespace of the current process just like an unshare(2) system call
> would do for other namespaces. I have not yet explored the
> alternatives of deferring the unshare to the next unshare(2),
> clone(2), or execve(2) call and would want to first confirm that doing
> so does not introduce any issues in the kernel or make it harder to
> integrate with existing container runtimes.
Doing it immediately seems like the right thing to do. So that
the container runtime can keep the umount/remount of selinuxfs
with the unshare, instead of having to defer that until after
a later syscall.
> Differences between this syscall interface and the selinuxfs interface
> that need discussion before moving forward:
>
> 1. The syscall interface does not currently check any Linux capability
> or DAC permissions, whereas the selinuxfs interface can only be set by
> uid-0 or CAP_DAC_OVERRIDE processes. We need to decide what if any
> capability or DAC check should apply to this syscall interface and if
> any, add the checks to either the LSM framework code or to the SELinux
> hook function.
I think this should be done by the SELinux hook. And I suspect you
do want to require those privs, but I could be wrong.
> Pros: Checking a capability or DAC permissions prevents misuse of this
> interface by unprivileged processes, particularly on systems with
> policies that do not yet define any of the new SELinux permissions
> introduced for controlling this operation. This is a potential concern
> on Linux distributions that do not tightly coordinate kernel updates
> with policy updates (or where users may choose to deploy upstream
> kernels on their own), but not on Android.
Hm, that's an interesting problem.
> Cons: Checking a capability or DAC permissions requires any process
> that uses this facility to have the corresponding capability or
> permissions, which might otherwise be unnecessary and create
> additional risks. This is less likely if we use a capability already
> required by container runtimes and similar components that might
> leverage this facility for unsharing SELinux namespaces.
>
> 2. The syscall interface checks a new SELinux unshare_selinuxns
> permission in the process2 class between the task SID and itself,
> similar to other checks for setting process attributes. This means
> that:
> allow domain self:process2 *; -or-
> allow domain self:process2 ~anything-other-than-unshare_selinuxns; -or-
> allow domain self:process2 unshare_selinuxns;
> would allow a process to unshare its SELinux namespace.
>
> The selinuxfs interface checks a new unshare permission in the
> security class between the task SID and the security initial SID,
> likewise similar to other checks for setting selinuxfs attributes.
> This means that:
> allow domain security_t:security *; -or-
> allow domain security_t:security ~anything-other-than-unshare; -or-
> allow domain security_t:security unshare;
> would allow a process to unshare its SELinux namespace.
>
> Technically, the selinuxfs interface also currently requires open and
> write access to the selinuxfs node; hence:
> allow domain security_t:file { open write };
> is also required for the selinuxfs interface.
>
> We need to decide what we want the SELinux check(s) to be for the
> syscall and whether it should be more like the former (process
> attributes) or more like the latter (security policy settings). Note
> that the permission name itself is unimportant here and only differs
> because it seemed less evident in the process2 class that we are
> talking about a SELinux namespace otherwise.
>
> Regardless, either form of allow rule can be prohibited in policies
> via neverallow rules on systems that enforce their usage
> (e.g. Android, not necessarily on Linux distributions).
>
> 3. The selinuxfs interface currently offers more functionality than I
> have implemented here for the sycall interface, including:
>
> a) the ability to read the selinuxfs node to see if your namespace has
> been unshared, which should be easily implementable via
> lsm_get_self_attr(2). However, questions remain as to when that
> should return 1 versus 0 (currently returns 1 whenever your namespace
> is NOT the initial SELinux namespace, useful for the testsuite to
> detect it is in a child, but could instead be reset to 0 by a
> subsequent policy load to indicate completion of the setup of the
> namespace, thus hiding from child processes that they are in a child
> namespace once its policy has been loaded).
maybe 'unshare' means that an unshare is in progress, and add an
'unshared' which is incremented on every unshare (and never
decremented) for use by the testsuite?
> b) the abilities to get and set the maximum number of SELinux
> namespaces (via a /sys/fs/selinux/maxns node) and to get and set the
> maximum depth for SELinux namespaces (via a /sys/fs/selinux/maxnsdepth
> node). These could be left in selinuxfs or migrated to some other LSM
> management APIs since they are global in scope, not per-process
> attributes.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 fixes a typo (PROCESS->PROCESS2) and is now tested.
>
> include/uapi/linux/lsm.h | 1 +
> security/selinux/hooks.c | 8 ++++++++
> security/selinux/include/classmap.h | 4 +++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index 938593dfd5da..fb1b4a8aa639 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -83,6 +83,7 @@ struct lsm_ctx {
> #define LSM_ATTR_KEYCREATE 103
> #define LSM_ATTR_PREV 104
> #define LSM_ATTR_SOCKCREATE 105
> +#define LSM_ATTR_UNSHARE 106
>
> /*
> * LSM_FLAG_XXX definitions identify special handling instructions
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f48483383d6e..1e34a16b7954 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6816,6 +6816,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETCURRENT, NULL);
> break;
> + case LSM_ATTR_UNSHARE:
> + error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS2,
> + PROCESS2__UNSHARE_SELINUXNS, NULL);
> + break;
> default:
> error = -EOPNOTSUPP;
> break;
> @@ -6927,6 +6931,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> }
>
> tsec->sid = sid;
> + } else if (attr == LSM_ATTR_UNSHARE) {
> + error = selinux_state_create(new);
> + if (error)
> + goto abort_change;
> } else {
> error = -EINVAL;
> goto abort_change;
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index be52ebb6b94a..07fe316308cd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -60,7 +60,9 @@ const struct security_class_mapping secclass_map[] = {
> "siginh", "setrlimit", "rlimitinh", "dyntransition",
> "setcurrent", "execmem", "execstack", "execheap",
> "setkeycreate", "setsockcreate", "getrlimit", NULL } },
> - { "process2", { "nnp_transition", "nosuid_transition", NULL } },
> + { "process2",
> + { "nnp_transition", "nosuid_transition", "unshare_selinuxns",
> + NULL } },
> { "system",
> { "ipc_info", "syslog_read", "syslog_mod", "syslog_console",
> "module_request", "module_load", "firmware_load",
> --
> 2.50.1
^ permalink raw reply
* [PATCH] Documentation/landlock: Make docs in cred.h and domain.h visible.
From: Tingmao Wang @ 2025-09-28 23:49 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tingmao Wang, Günther Noack, linux-security-module
Currently even though the structures in these files have documentation,
they are not shown in the "Landlock LSM: kernel documentation" page.
Signed-off-by: Tingmao Wang <m@maowtm.org>
---
Documentation/security/landlock.rst | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst
index e0fc54aff09e..5066f8c3091e 100644
--- a/Documentation/security/landlock.rst
+++ b/Documentation/security/landlock.rst
@@ -110,6 +110,12 @@ Filesystem
.. kernel-doc:: security/landlock/fs.h
:identifiers:
+Process credential
+------------------
+
+.. kernel-doc:: security/landlock/cred.h
+ :identifiers:
+
Ruleset and domain
------------------
@@ -128,6 +134,9 @@ makes the reasoning much easier and helps avoid pitfalls.
.. kernel-doc:: security/landlock/ruleset.h
:identifiers:
+.. kernel-doc:: security/landlock/domain.h
+ :identifiers:
+
Additional documentation
========================
base-commit: f83ec76bf285bea5727f478a68b894f5543ca76e
--
2.51.0
^ permalink raw reply related
* Re: [PATCH RFC 0/4] landlock: add LANDLOCK_SCOPE_MEMFD_EXEC execution
From: Abhinav Saxena @ 2025-09-28 23:37 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Paul Moore, James Morris, Serge E. Hallyn,
Shuah Khan, Nathan Chancellor, Nick Desaulniers, Bill Wendling,
Justin Stitt, linux-security-module, linux-kernel,
linux-kselftest, llvm, Daniel Verkamp, Jeff Xu,
Thiébaud Weksteen, Stephen Smalley
In-Reply-To: <20250918.io7too8ain7A@digikod.net>
[-- Attachment #1: Type: text/plain, Size: 8093 bytes --]
Thanks for the detailed reply Mickaël!
Mickaël Salaün <mic@digikod.net> writes:
> Thanks for this patch series Abhinav! The code looks good overall, but
> we should clarify the design. Sorry for the delayed response, it is on
> my radar now.
>
> CCing Jeff and Daniel
>
> On Sat, Jul 19, 2025 at 05:13:10AM -0600, Abhinav Saxena wrote:
>> This patch series introduces LANDLOCK_SCOPE_MEMFD_EXEC, a new Landlock
>> scoping mechanism that restricts execution of anonymous memory file
>> descriptors (memfd) created via memfd_create(2). This addresses security
>> gaps where processes can bypass W^X policies and execute arbitrary code
>> through anonymous memory objects.
>>
>> Fixes: <https://github.com/landlock-lsm/linux/issues/37>
>>
>> SECURITY PROBLEM
>> `=============='
>>
>> Current Landlock filesystem restrictions do not cover memfd objects,
>> allowing processes to:
>>
>> 1. Read-to-execute bypass: Create writable memfd, inject code,
>> then execute via mmap(PROT_EXEC) or direct execve()
>> 2. Anonymous execution: Execute code without touching the filesystem via
>> execve(“/proc/self/fd/N”) where N is a memfd descriptor
>
>> 3. Cross-domain access violations: Pass memfd between processes to
>> bypass domain restrictions
>
> Landlock only restricts access at open time, which is a useful property.
> This enables to create more restricted sandboxes but still get access to
> outside resources via trusted processes. If the process passing the FDs
> is not trusted, the sandboxed process could just ask to execute
> arbitrary code outside the sandbox anyway.
>
> However, the Landlock scopes are designed to block IPC from within a
> sandbox to outside the sandbox. We could have a new scope to forbid a
> sandbox process to receive or inherit file descriptors, but that would
> be a different and generic feature. For compatibility reasons, this
> might not be easy to implement and I think there are more important
> features to implement before that.
>
> Thinking more about it, restricting memfd should not be a “scoped” flag
> because the semantic is not the same, but we should have a new ruleset
> property instead, something like “ruleset.denied” with a related
> LANDLOCK_DENY_EXECUTE_MEMFD flag. This flag will only have an impact on
> newly created memfd from a sandboxed process with this restriction at
> creation time. This could be implemented with hook_file_alloc_security()
> by checking if the file is indeed a memfd and checking inode->i_mode for
> executability bits (which would imply MFD_NOEXEC_SEAL).
>
Thanks for the clarification! So if I understood correctly we are
proposing adding a `denied` field to the `landlock_ruleset_attr` struct
struct landlock_ruleset_attr {
__u64 handled_access_fs;
__u64 handled_access_net;
__u64 scoped;
__u64 denied; /* New field */
};
which allows memfd_create() to be allowed by default unless
LANDLOCK_DENY_EXECUTE_MEMFD bit is set. Also it seems Thiébaud
Weksteen’s patch[1] will land, and maybe we can use
security_inode_init_security_anon instead? What do you think?
Apologies for my ignorance, do we have to wait till his patch has
landed into Linus’s tree?
>>
>> These scenarios can occur in sandboxed environments where filesystem
>> access is restricted but memfd creation remains possible.
>>
>> IMPLEMENTATION
>> `============'
>>
>> The implementation adds hierarchical execution control through domain
>> scoping:
>>
>> Core Components:
>> - is_memfd_file(): Reliable memfd detection via “memfd:” dentry prefix
>> - domain_is_scoped(): Cross-domain hierarchy checking (moved to domain.c)
>> - LSM hooks: mmap_file, file_mprotect, bprm_creds_for_exec
>> - Creation-time restrictions: hook_file_alloc_security
>>
>> Security Matrix:
>> Execution decisions follow domain hierarchy rules preventing both
>> same-domain bypass attempts and cross-domain access violations while
>> preserving legitimate hierarchical access patterns.
>>
>> Domain Hierarchy with LANDLOCK_SCOPE_MEMFD_EXEC:
>> `============================================='
>>
>> Root (no domain) - No restrictions
>> |
>> +– Domain A [SCOPE_MEMFD_EXEC] Layer 1
>> | +– memfd_A (tagged with Domain A as creator)
>> | |
>> | +– Domain A1 (child) [NO SCOPE] Layer 2
>> | | +– Inherits Layer 1 restrictions from parent
>> | | +– memfd_A1 (can create, inherits restrictions)
>> | | +– Domain A1a [SCOPE_MEMFD_EXEC] Layer 3
>> | | +– memfd_A1a (tagged with Domain A1a)
>> | |
>> | +– Domain A2 (child) [SCOPE_MEMFD_EXEC] Layer 2
>> | +– memfd_A2 (tagged with Domain A2 as creator)
>> | +– CANNOT access memfd_A1 (different subtree)
>> |
>> +– Domain B [SCOPE_MEMFD_EXEC] Layer 1
>> +– memfd_B (tagged with Domain B as creator)
>> +– CANNOT access ANY memfd from Domain A subtree
>>
>> Execution Decision Matrix:
>> `======================'
>> Executor-> | A | A1 | A1a | A2 | B | Root
>> Creator | | | | | |
>> ————|—–|—-|—–|—-|—-|—–
>> Domain A | X | X | X | X | X | Y
>> Domain A1 | Y | X | X | X | X | Y
>> Domain A1a | Y | Y | X | X | X | Y
>> Domain A2 | Y | X | X | X | X | Y
>> Domain B | X | X | X | X | X | Y
>> Root | Y | Y | Y | Y | Y | Y
>>
>> Legend: Y = Execution allowed, X = Execution denied
>
> Because checks should not be related to scopes, this will be much
> simpler.
>
>>
>> Scenarios Covered:
>> - Direct mmap(PROT_EXEC) on memfd files
>> - Two-stage mmap(PROT_READ) + mprotect(PROT_EXEC) bypass attempts
>> - execve("/proc/self/fd/N") anonymous execution
>> - execveat() and fexecve() file descriptor execution
>> - Cross-process memfd inheritance and IPC passing
>>
>> TESTING
>> `====='
>>
>> All patches have been validated with:
>> - scripts/checkpatch.pl –strict (clean)
>> - Selftests covering same-domain restrictions, cross-domain
>> hierarchy enforcement, and regular file isolation
>> - KUnit tests for memfd detection edge cases
>
> Thanks for all these tests!
>
>>
>> DISCLAIMER
>> `========'
>>
>> My understanding of Landlock scoping semantics may be limited, but this
>> implementation reflects my current understanding based on available
>> documentation and code analysis. I welcome feedback and corrections
>> regarding the scoping logic and domain hierarchy enforcement.
>>
>> Signed-off-by: Abhinav Saxena <xandfury@gmail.com>
>> —
>> Abhinav Saxena (4):
>> landlock: add LANDLOCK_SCOPE_MEMFD_EXEC scope
>> landlock: implement memfd detection
>> landlock: add memfd exec LSM hooks and scoping
>> selftests/landlock: add memfd execution tests
>>
>> include/uapi/linux/landlock.h | 5 +
>> security/landlock/.kunitconfig | 1 +
>> security/landlock/audit.c | 4 +
>> security/landlock/audit.h | 1 +
>> security/landlock/cred.c | 14 -
>> security/landlock/domain.c | 67 ++++
>> security/landlock/domain.h | 4 +
>> security/landlock/fs.c | 405 ++++++++++++++++++++-
>> security/landlock/limits.h | 2 +-
>> security/landlock/task.c | 67 —-
>> …/selftests/landlock/scoped_memfd_exec_test.c | 325 +++++++++++++++++
>> 11 files changed, 812 insertions(+), 83 deletions(-)
>> —
>> base-commit: 5b74b2eff1eeefe43584e5b7b348c8cd3b723d38
>> change-id: 20250716-memfd-exec-ac0d582018c3
>>
>> Best regards,
>> –
>> Abhinav Saxena <xandfury@gmail.com>
>>
>>
Best,
Abhinav
[1] - <https://lore.kernel.org/all/20250918020434.1612137-1-tweek@google.com/>
^ permalink raw reply
* Re: [PATCH v3 01/14] ACPI: APEI: Remove redundant rcu_read_lock/unlock() in spin_lock
From: Rafael J. Wysocki @ 2025-09-28 10:33 UTC (permalink / raw)
To: Hanjun Guo, pengdonglin
Cc: tj, tony.luck, jani.nikula, ap420073, jv, freude, bcrl, trondmy,
longman, kees, bigeasy, hdanton, paulmck, linux-kernel,
linux-rt-devel, linux-nfs, linux-aio, linux-fsdevel,
linux-security-module, netdev, intel-gfx, linux-wireless,
linux-acpi, linux-s390, cgroups, Rafael J. Wysocki, pengdonglin
In-Reply-To: <03ad08d9-4510-19fb-bbad-652159308119@huawei.com>
On Sat, Sep 27, 2025 at 5:22 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> On 2025/9/16 12:47, pengdonglin wrote:
> > From: pengdonglin <pengdonglin@xiaomi.com>
> >
> > Since commit a8bb74acd8efe ("rcu: Consolidate RCU-sched update-side function definitions")
> > there is no difference between rcu_read_lock(), rcu_read_lock_bh() and
> > rcu_read_lock_sched() in terms of RCU read section and the relevant grace
> > period. That means that spin_lock(), which implies rcu_read_lock_sched(),
> > also implies rcu_read_lock().
> >
> > There is no need no explicitly start a RCU read section if one has already
> > been started implicitly by spin_lock().
> >
> > Simplify the code and remove the inner rcu_read_lock() invocation.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Hanjun Guo <guohanjun@huawei.com>
> > Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
> > Signed-off-by: pengdonglin <dolinux.peng@gmail.com>
> > ---
> > drivers/acpi/apei/ghes.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index a0d54993edb3..97ee19f2cae0 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -1207,12 +1207,10 @@ static int ghes_notify_hed(struct notifier_block *this, unsigned long event,
> > int ret = NOTIFY_DONE;
> >
> > spin_lock_irqsave(&ghes_notify_lock_irq, flags);
> > - rcu_read_lock();
> > list_for_each_entry_rcu(ghes, &ghes_hed, list) {
> > if (!ghes_proc(ghes))
> > ret = NOTIFY_OK;
> > }
> > - rcu_read_unlock();
> > spin_unlock_irqrestore(&ghes_notify_lock_irq, flags);
> >
> > return ret;
>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
Applied as 6.18 material, thanks!
^ permalink raw reply
* [PATCH] ima: Fall back to default kernel module signature verification
From: Coiby Xu @ 2025-09-28 3:03 UTC (permalink / raw)
To: linux-integrity
Cc: Dmitry Torokhov, Karel Srot, Mimi Zohar, Roberto Sassu,
Dmitry Kasatkin, Eric Snowberg, Paul Moore, James Morris,
Serge E. Hallyn, open list:SECURITY SUBSYSTEM, open list
Currently, for any IMA policy that requires appraisal for kernel modules
e.g. ima_policy=secure_boot, PowerPC architecture specific policy,
booting will fail because IMA will reject a kernel module which will
be decompressed in the kernel space and then have its signature
verified.
This happens because when in-kernel module decompression
(CONFIG_MODULE_DECOMPRESS) is enabled, kmod will use finit_module
syscall instead of init_module to load a module. And IMA mandates IMA
xattr verification for finit_module unless appraise_type=imasig|modsig
is specified in the rule. However currently initramfs doesn't support
xattr. And IMA rule "func=MODULE_CHECK appraise_type=imasig|modsig"
doesn't work either because IMA will treat to-be-decompressed kernel
module as not having module signature as it can't decompress kernel
module to check if signature exists.
So fall back to default kernel module signature verification when we have
no way to verify IMA xattr.
Reported-by: Karel Srot <ksrot@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
Another approach will be to make IMA decompress the kernel module to
check the signature. This requires refactoring kernel module code to
make the in-kernel module decompressing feature modular and seemingly
more efforts are needed. A second disadvantage is it feels
counter-intuitive to verify the same kernel module signature twice. And
we still need to make ima_policy=secure_boot allow verifying appended
module signature.
Anyways, I'm open to suggestions and can try the latter approach if
there are some benefits I'm not aware of or a better approach.
security/integrity/ima/ima_appraise.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f435eff4667f..fcc75dd1486f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -502,9 +502,10 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
enum integrity_status status = INTEGRITY_UNKNOWN;
int rc = xattr_len;
bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig;
+ bool enforce_module_sig = iint->flags & IMA_DIGSIG_REQUIRED && func == MODULE_CHECK;
- /* If not appraising a modsig, we need an xattr. */
- if (!(inode->i_opflags & IOP_XATTR) && !try_modsig)
+ /* If not appraising a modsig or using default module verification, we need an xattr. */
+ if (!(inode->i_opflags & IOP_XATTR) && !try_modsig && !enforce_module_sig)
return INTEGRITY_UNKNOWN;
/*
@@ -517,8 +518,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
if (is_bprm_creds_for_exec(func, file))
audit_msgno = AUDIT_INTEGRITY_USERSPACE;
- /* If reading the xattr failed and there's no modsig, error out. */
- if (rc <= 0 && !try_modsig) {
+ /* If reading the xattr failed and there's no modsig or module verification, error out. */
+ if (rc <= 0 && !try_modsig && !enforce_module_sig) {
if (rc && rc != -ENODATA)
goto out;
@@ -549,8 +550,8 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
case INTEGRITY_UNKNOWN:
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
- /* It's fine not to have xattrs when using a modsig. */
- if (try_modsig)
+ /* Fine to not have xattrs when using a modsig or default module verification. */
+ if (try_modsig || enforce_module_sig)
break;
fallthrough;
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
@@ -580,6 +581,18 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
rc == -ENOKEY))
rc = modsig_verify(func, modsig, &status, &cause);
+ /* Fall back to default kernel module signature verification */
+ if (rc && enforce_module_sig) {
+ rc = 0;
+ set_module_sig_enforced();
+ /* CONFIG_MODULE_SIG may be disabled */
+ if (is_module_sig_enforced()) {
+ rc = 0;
+ status = INTEGRITY_PASS;
+ pr_debug("Fall back to default kernel module verification for %s\n", filename);
+ }
+ }
+
out:
/*
* File signatures on some filesystems can not be properly verified.
base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
--
2.51.0
^ permalink raw reply related
* Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules
From: Tingmao Wang @ 2025-09-27 23:12 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Jan Kara, linux-security-module
In-Reply-To: <20250927.ivat2tho0Uom@digikod.net>
On 9/27/25 20:00, Mickaël Salaün wrote:
> On Sat, Sep 27, 2025 at 04:43:50PM +0100, Tingmao Wang wrote:
>> [..]
>>
>> Also, do we want to consider calling this something else instead, like
>> "suppress_audit"?
>
> Quiet is simpler (similar to the LANDLOCK_RESTRICT_SELF_LOG_* flags) and
> if we get other ways to log actions this will also be used. For the
> supervisor case, that would be useful to not forward a request to the
> supervisor. The *_LOG_* flags could be used the same way too (even if
> "LOG" may be a subset of the supervisor capabilities). Do you think
> that would be OK? Dedicated flags would be more flexible but also a bit
> more complex. Is it worth it? In any case, the semantic and need
> should be quite similar.
I don't think we need a dedicated flag, I was just wondering if "QUIET" is
the right name, but I guess I don't have a better suggestion either. On
second thought SUPPRESS_AUDIT would no longer be accurate if we later use
it to control supervisor forwarding (it would be doing more than just
suppressing audit logs).
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Eric Biggers @ 2025-09-27 23:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, bpf, LSM List, Blaise Boscaccy, Paul Moore,
K. Y. Srinivasan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
In-Reply-To: <CAADnVQ+t6JJ9YgH_xgicbzvvP2WvEJWxi+hioQtFKrR6BLTsCg@mail.gmail.com>
On Sat, Sep 27, 2025 at 11:33:12PM +0100, Alexei Starovoitov wrote:
> On Sat, Sep 27, 2025 at 10:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> > > Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> > > used for the loader generation code to embed the metadata hash in the
> > > loader program and also by the bpf_map__make_exclusive API to calculate
> > > the hash of the program the map is exclusive to.
> > >
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> >
> > Nacked-by: Eric Biggers <ebiggers@kernel.org>
> >
> > No more users of AF_ALG, please. It's a huge mistake and has been
> > incredibly problematic over the years.
>
> Lol. True, but good luck with that. AF_ALG is uapi and it will be removed
> only when the last user retires many years from now.
Many Linux systems never enabled AF_ALG in the first place, and those
that have it enabled often only have a few users of it or even none at
all. Sure, AF_ALG support will remain in-tree for a very long time or
even forever. But many systems can keep it disabled, or can disable it,
if new users are not introduced and existing users continue to be fixed.
Let's do the right thing here, instead of making the situation even
worse and also adding undocumented kconfig dependencies to libbpf.
> > If you don't want to depend on a library, then just include some basic
> > SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
> > https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
> > I'd even be glad to write the patch for you, if you want.
>
> Yes. Please. If you can craft sha256 without external dependencies
> we can certainly use it.
> Certainly agree that it would be better than AF_ALG.
Sure, I'll do that.
- Eric
^ permalink raw reply
* Re: [PATCH v2 0/7] fs/9p: Reuse inode based on path (in addition to qid)
From: Tingmao Wang @ 2025-09-27 22:53 UTC (permalink / raw)
To: Mickaël Salaün, Greg Kurz
Cc: Christian Schoenebeck, Dominique Martinet, Eric Van Hensbergen,
Latchesar Ionkov, v9fs, Günther Noack, linux-security-module,
Jan Kara, Amir Goldstein, Matthew Bobrowski, Al Viro,
Christian Brauner, linux-fsdevel, qemu-devel
In-Reply-To: <20250927.ahGhiiy0koo0@digikod.net>
On 9/27/25 19:27, Mickaël Salaün wrote:
> Adding Greg Kurz too.
>
> On Sun, Sep 21, 2025 at 05:24:49PM +0100, Tingmao Wang wrote:
>> On 9/17/25 16:00, Mickaël Salaün wrote:
>>> [...]
>>
>> Alternatively if we believe this to be a QEMU issue, maybe
>> Landlock don't need to work around it and should just hold fids (and use
>> QIDs to key the rules) anyway despite server quirks like these. This can
>> perhaps then be fixed in QEMU?
>
> Yes, I think it would make sense for Landlock to open and keep open a
> fid (and hopefully the related remote file). However, the v9fs umount
> should be handled gracefully the same way Landlock tag inodes are
> handled. This should come with a QEMU patch to fix the consistency
> issue.
>
>>
>> (I guess the fact that QEMU is doing path tracking in the first place does
>> gives more precedent for justifying doing path tracking in v9fs as well,
>> but maybe that's the wrong way to think about it)
>
> Anyway, if QEMU does it, wouldn't it be the same for Landlock to just
> rely on fid?
The fid can't be relied on because it's just a handle. The client can
open multiple fids pointing to the same file (and in fact this is what
v9fs does - new fid for each open())
> If QEMU uses FD+O_PATH, then Landlock would work even for
> server-moved files.
(With this new approach, Landlock would have to key the rules based on
qid, but it also needs to hold an open fid to prevent that qid from being
reused (due to ext4 inode number reuse, etc))
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Alexei Starovoitov @ 2025-09-27 22:33 UTC (permalink / raw)
To: Eric Biggers
Cc: KP Singh, bpf, LSM List, Blaise Boscaccy, Paul Moore,
K. Y. Srinivasan, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
In-Reply-To: <20250927210345.GE9798@quark>
On Sat, Sep 27, 2025 at 10:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> > Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> > used for the loader generation code to embed the metadata hash in the
> > loader program and also by the bpf_map__make_exclusive API to calculate
> > the hash of the program the map is exclusive to.
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
>
> Nacked-by: Eric Biggers <ebiggers@kernel.org>
>
> No more users of AF_ALG, please. It's a huge mistake and has been
> incredibly problematic over the years.
Lol. True, but good luck with that. AF_ALG is uapi and it will be removed
only when the last user retires many years from now.
> If you don't want to depend on a library, then just include some basic
> SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
> https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
> I'd even be glad to write the patch for you, if you want.
Yes. Please. If you can craft sha256 without external dependencies
we can certainly use it.
Certainly agree that it would be better than AF_ALG.
^ permalink raw reply
* Re: [PATCH v5 03/12] libbpf: Implement SHA256 internal helper
From: Eric Biggers @ 2025-09-27 21:03 UTC (permalink / raw)
To: KP Singh
Cc: bpf, linux-security-module, bboscaccy, paul, kys, ast, daniel,
andrii
In-Reply-To: <20250921133133.82062-4-kpsingh@kernel.org>
On Sun, Sep 21, 2025 at 03:31:24PM +0200, KP Singh wrote:
> Use AF_ALG sockets to not have libbpf depend on OpenSSL. The helper is
> used for the loader generation code to embed the metadata hash in the
> loader program and also by the bpf_map__make_exclusive API to calculate
> the hash of the program the map is exclusive to.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
Nacked-by: Eric Biggers <ebiggers@kernel.org>
No more users of AF_ALG, please. It's a huge mistake and has been
incredibly problematic over the years.
If you don't want to depend on a library, then just include some basic
SHA-256 code, similar to what I'm doing for iproute2 and SHA-1 at
https://lore.kernel.org/netdev/20250925225322.13013-1-ebiggers@kernel.org/.
I'd even be glad to write the patch for you, if you want.
- Eric
^ permalink raw reply
* [syzbot] Monthly lsm report (Sep 2025)
From: syzbot @ 2025-09-27 20:43 UTC (permalink / raw)
To: linux-kernel, linux-security-module, syzkaller-bugs
Hello lsm maintainers/developers,
This is a 31-day syzbot report for the lsm subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/lsm
During the period, 0 new issues were detected and 0 were fixed.
In total, 6 issues are still open and 41 have already been fixed.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 45 No possible deadlock in process_measurement (5)
https://syzkaller.appspot.com/bug?extid=6529afa25091aee8536c
<2> 43 Yes INFO: task hung in process_measurement (3)
https://syzkaller.appspot.com/bug?extid=cb9e66807bcb882cd0c5
<3> 13 Yes INFO: task hung in ima_file_free (4)
https://syzkaller.appspot.com/bug?extid=8036326eebe7d0140944
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH v2 0/7] fs/9p: Reuse inode based on path (in addition to qid)
From: Mickaël Salaün @ 2025-09-27 18:27 UTC (permalink / raw)
To: Tingmao Wang, Greg Kurz
Cc: Christian Schoenebeck, Dominique Martinet, Eric Van Hensbergen,
Latchesar Ionkov, v9fs, Günther Noack, linux-security-module,
Jan Kara, Amir Goldstein, Matthew Bobrowski, Al Viro,
Christian Brauner, linux-fsdevel, qemu-devel
In-Reply-To: <f1228978-dac0-4d1a-a820-5ac9562675d0@maowtm.org>
Adding Greg Kurz too.
On Sun, Sep 21, 2025 at 05:24:49PM +0100, Tingmao Wang wrote:
> On 9/17/25 16:00, Mickaël Salaün wrote:
> > On Wed, Sep 17, 2025 at 11:52:35AM +0200, Christian Schoenebeck wrote:
> >> On Wednesday, September 17, 2025 1:59:21 AM CEST Tingmao Wang wrote:
> >>> On 9/16/25 20:22, Christian Schoenebeck wrote:
> >>>> On Tuesday, September 16, 2025 4:01:40 PM CEST Tingmao Wang wrote:
> >> [...]
> >>>> I see that you are proposing an option for your proposed qid based
> >>>> re-using of dentries. I don't think it should be on by default though,
> >>>> considering what we already discussed (e.g. inodes recycled by ext4, but
> >>>> also not all 9p servers handling inode collisions).
> >>>
> >>> Just to be clear, this approach (Landlock holding a fid reference, then
> >>> using the qid as a key to search for rules when a Landlocked process
> >>> accesses the previously remembered file, possibly after the file has been
> >>> moved on the server) would only be in Landlock, and would only affect
> >>> Landlock, not 9pfs (so not sure what you meant by "re-using of dentries").
> >>>
> >>> The idea behind holding a fid reference within Landlock is that, because
> >>> we have the file open, the inode would not get recycled in ext4, and thus
> >>> no other file will reuse the qid, until we close that reference (when the
> >>> Landlock domain terminates, or when the 9p filesystem is unmounted)
> >>
> >> So far I only had a glimpse on your kernel patches and had the impression that
> >> they are changing behaviour for all users, since you are touching dentry
> >> lookup.
> >
> > I think we should not hold dentries because:
> > - they reference other dentries (i.e. a file hierarchy),
> > - they block umount and I'm convinced the VFS (and users) are not going
> > to like long-lived dentries,
> > - Landlock and inotify don't need dentries, just inodes.
> >
> > I'm wondering why fid are referenced by dentries instead of inodes.
> >
> > The need for Landlock is to be able to match an inode with a previously
> > seen one. Not all LSM hooks (nor VFS internals) always have access to
> > dentries, but they do have access to inodes.
> >
> >>
> >>>> For all open FIDs QEMU retains a descriptor to the file/directory.
> >>>>
> >>>> Which 9p message do you see sent to server, Trename or Trenameat?
> >>>>
> >>>> Does this always happen to you or just sometimes, i.e. under heavy load?
> >>>
> >>> Always happen, see log: (no Trename since the rename is done on the host)
> >> [...]
> >>> Somehow if I rename in the guest, it all works, even though it's using the
> >>> same fid 2 (and it didn't ask QEMU to walk the new path)
> >>
> >> Got it. Even though QEMU *should* hold a file descriptor (or a DIR* stream,
> >
> > It's reasonable to assume that QEMU and other should hold opened fid In
> > practice, this might not always be the case, but let's move on and
> > consider that a 9p server bug.
> >
> > Landlock and fanotify need some guarantees on opened files, and we
> > cannot consider every server bug. For Landlock, inode may get an
> > "ephemeral tag" (with the Landlock object mechanism) to match previously
> > seen inodes. In a perfect world, Landlock could keep a reference on 9p
> > inodes (as for other filesystems) and these inodes would always match
> > the same file. In practice this is not the case, but the 9p client
> > requirements and the Landlock requirements are not exactly the same.
> >
> > A 9p client (the kernel) wants to safely deal with duplicated qid, which
> > should not happen but still happen in practice as explained before.
> > On the other side, Landlock wants to not deny access to allowed files
> > (currently identified by their inodes), but I think it would be
> > reasonable to allow access theoretically denied (i.e. not allowed to be
> > precise, because of the denied by default mechanism) files because of a
> > 9p server bug mishandling qid (e.g. mapping them to recycled ext4
> > inodes).
> >
> > All that to say that it looks reasonable for Landlock to trust the
> > filesystem, and by that I mean all its dependencies, including the 9p
> > server, to not have bugs.
> >
> > Another advantage to rely on qid and server-side opened files is that we
> > get (in theory) the same semantic as when Landlock is used with local
> > filesystems (e.g. files moved on the server should still be correctly
> > identified by Landlock on the client).
> >
> >> which should imply a file descriptor), there is still a path string stored at
> >> V9fsFidState and that path being processed at some places, probably because
> >> there are path based and FID based variants (e.g Trename vs. Trenameat). Maybe
> >> that clashes somewhere, not sure. So I fear you would need to debug this.
> >
> > Good to know that it is not a legitimate behavior for a 9p client.
>
> So I did some quick debugging and realized that I had a wrong
> understanding of how fids relates to opened files on the host, under QEMU.
> It turns out that in QEMU's 9p server implementation, a fid does not
> actually correspond to any opened file descriptors - it merely represents
> a (string-based) path that QEMU stores internally. It only opens the
> actual file if the client actually does an T(l)open, which is in fact
> separate from acquiring the fid with T(l)walk. The reason why renaming
> file/dirs from the client doesn't break those fids is because QEMU will
> actually fix those paths when a rename request is processed - c.f.
> v9fs_fix_fid_paths [1].
>
> It turns out that even if a guest process opens the file with O_PATH, that
> file descriptor does not cause an actual Topen, and therefore QEMU does
> not open the file on the host, and later on reopening that fd with another
> mode (via e.g. open("/proc/self/fd/...", O_RDONLY)) will fail if the file
> has moved on the host without QEMU's knowledge. Also, openat will fail if
> provided with a dir fd that "points" to a moved directory, regardless of
> whether the fd is opened with O_PATH or not, since path walk in QEMU is
> completely string-based and does not actually issue openat on the host fs
> [2].
>
> I'm not sure if this was is intentional in QEMU - it would seem to me that
> a fid should translate to a fd (maybe opened with just O_PATH) on the
> host, and path walks based on that fid should be done via openat with this
> fd, which will also "automatically" handle renames without QEMU needing to
> fixup the string paths?
I agree, it would make sense for QEMU to map fid to FD+O_PATH. That
would avoid the kind of issues you mentioned.
Christian, Greg, what do you think?
>
> In any case, this probably means that even if Landlock were to hold a fid
> reference, and QEMU does qid remapping, that's still not enough to
> guarantees that we won't have a different, unrelated file ending up with
> the same qid, at least under ext4.
>
> I'm not sure what's the way forward - would Landlock need to actually
> "open" the files (or do something that will cause a Topen to be issued by
> v9fs)?
> Alternatively if we believe this to be a QEMU issue, maybe
> Landlock don't need to work around it and should just hold fids (and use
> QIDs to key the rules) anyway despite server quirks like these. This can
> perhaps then be fixed in QEMU?
Yes, I think it would make sense for Landlock to open and keep open a
fid (and hopefully the related remote file). However, the v9fs umount
should be handled gracefully the same way Landlock tag inodes are
handled. This should come with a QEMU patch to fix the consistency
issue.
>
> (I guess the fact that QEMU is doing path tracking in the first place does
> gives more precedent for justifying doing path tracking in v9fs as well,
> but maybe that's the wrong way to think about it)
Anyway, if QEMU does it, wouldn't it be the same for Landlock to just
rely on fid? If QEMU uses FD+O_PATH, then Landlock would work even for
server-moved files.
>
> Test programs: openat.c [3], open_procselffd.c [4]
>
>
> [1]: https://gitlab.com/qemu-project/qemu/-/blob/44f51c1a3cf435daa82eb757740b59b1fd4fe71c/hw/9pfs/9p.c#L3403
> [2]: https://gitlab.com/qemu-project/qemu/-/blob/371a269ff8ce561c28e4fa03bb49e4940f990637/hw/9pfs/9p-local.c#L1243
> [3]: https://fileshare.maowtm.org/9pfs-landlock-fix/20250921/openat.c
> [4]: https://fileshare.maowtm.org/9pfs-landlock-fix/20250921/open_procselffd.c
>
^ permalink raw reply
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