* Re: [PATCH] ima: setting security.ima to fix security.evm for a file with IMA signature
From: Coiby Xu @ 2025-09-24 8:03 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <40e9c7bd15d4ab8b71ac335b5d896ed39c61980c.camel@linux.ibm.com>
On Mon, Sep 15, 2025 at 05:05:42PM -0400, Mimi Zohar wrote:
>On Wed, 2025-09-10 at 09:20 +0800, Coiby Xu wrote:
>> On Tue, Sep 09, 2025 at 11:31:20AM -0400, Mimi Zohar wrote:
>> > On Tue, 2025-09-09 at 12:19 +0800, Coiby Xu wrote:
>> > > When both IMA and EVM fix modes are enabled, accessing a file with IMA
>> > > signature won't cause security.evm to be fixed. But this doesn't happen
>> > > to a file with correct IMA hash already set because accessing it will
>> > > cause setting security.ima again which triggers fixing security.evm
>> > > thanks to security_inode_post_setxattr->evm_update_evmxattr.
>> > >
>> > > Let's use the same mechanism to fix security.evm for a file with IMA
>> > > signature.
>> > >
[...]
>> > > ---
>> > > security/integrity/ima/ima_appraise.c | 27 +++++++++++++++++++++------
>> > > 1 file changed, 21 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> > > index f435eff4667f..18c3907c5e44 100644
>> > > --- a/security/integrity/ima/ima_appraise.c
>> > > +++ b/security/integrity/ima/ima_appraise.c
>> > > @@ -595,12 +595,27 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
>> > > integrity_audit_msg(audit_msgno, inode, filename,
>> > > op, cause, rc, 0);
>> > > } else if (status != INTEGRITY_PASS) {
>> > > - /* Fix mode, but don't replace file signatures. */
>> > > - if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
>> > > - (!xattr_value ||
>> > > - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>> > > - if (!ima_fix_xattr(dentry, iint))
>> > > - status = INTEGRITY_PASS;
>> > > + /*
>> > > + * Fix mode, but don't replace file signatures.
>> > > + *
>> > > + * When EVM fix mode is also enabled, security.evm will be
>> > > + * fixed automatically when security.ima is set because of
>> > > + * security_inode_post_setxattr->evm_update_evmxattr.
>> > > + */
>> > > + if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
>> > > + if (!xattr_value ||
>> > > + xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
>> > > + if (ima_fix_xattr(dentry, iint))
>> > > + status = INTEGRITY_PASS;
>> > > + } else if (xattr_value->type == EVM_IMA_XATTR_DIGSIG &&
>> > > + evm_revalidate_status(XATTR_NAME_IMA)) {
>> > > + if (!__vfs_setxattr_noperm(&nop_mnt_idmap,
>> > > + dentry,
>> > > + XATTR_NAME_IMA,
>> > > + xattr_value,
>> > > + xattr_len, 0))
>> > > + status = INTEGRITY_PASS;
>> > > + }
>> > > }
>
>Instead of re-writing the IMA signature without a clear explanation, define a
>new EVM function named evm_fix_hmac() and add a call here in IMA. Only in EVM
>fix mode would evm_fix_hmac() update the EVM hmac.
>
> } else if (status != INTEGRITY_PASS) {
> /*
> * IMA fix mode updates the IMA file hash, which triggers EVM
> * to update security.evm. ....
> *
> * Similarly, trigger fixing EVM HMAC for IMA file signatures.
> */
> if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
> if (!xattr_value ||
> xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
> if (ima_fix_xattr(dentry, iint))
> status = INTEGRITY_PASS;
> } else if (status == INTEGRITY_NOLABEL) {
> evm_fix_hmac(dentry, XATTR_NAME_IMA, ....);
> }
> }
Thanks for the advice! I wonder if we should use existing
evm_update_evmxattr instead of defining a new EVM function.
/*
* Calculate the hmac and update security.evm xattr
*
* Expects to be called with i_mutex locked.
*/
int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
const char *xattr_value, size_t xattr_value_len)
{
}
I already tried evm_update_evmxattr and can confirm it works. But later
I switched to __vfs_setxattr_noperm because I thought it's consistent
with current logic of adding security.evm when there is already correct
security.ima and it's a slightly smaller change.
--
Best regards,
Coiby
^ permalink raw reply
* Re: [RFC PATCH v2] lsm,selinux: introduce LSM_ATTR_UNSHARE and wire it up for SELinux
From: Dr. Greg @ 2025-09-24 8:08 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:
Good morning, I hope the week is going well for everyone.
> 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.
>
> 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.
>
> 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.
>
> 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).
>
> 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.
We had a number of exchanges regarding LSM namespacing in the thread
that Paul Moore started on this issue:
https://lore.kernel.org/linux-security-module/CAHC9VhRGMmhxbajwQNfGFy+ZFF1uN=UEBjqQZQ4UBy7yds3eVQ@mail.gmail.com/
The one issue that seemed to achieve universal consensus was that
every LSM was going to have different requirements for namespacing.
At the risk of playing devil's advocate, this seems to raise the
question as to whether or not there is a need to have a common API for
requesting security namespace separation or leave the issue to LSM
specific implementations.
The primary rationale for some modicum of centralized infrastructure
would seem to be to have a system call rather than an LSM specific
pseudo-filesystem interface to control security namespaces. Since
creating a system call interface is going to lock the API in stone it
would seem that we would want to get this right, or at least as
generic as possibe.
So some comments to that end.
If we use the lsm_set_self_attr(2) system call as our approach, the
namespace separation process needs to be split into two separate
calls. One to request the creation of a namespace and a second call
to request that the process join the new namespace.
This is required in order to support the ability for an orchestration
process to load a policy or model and have it in place before the new
namespace is allowed to enforce the policy or model.
So we would need something like an LSM_ATTR_UNSHARE_INIT as well as
the LSM_ATTR_UNSHARE attribute.
So the model would be for a process to issue an LSM_ATTR_UNSHARE_INIT
call to create the new security context namespace. That namespace
context can then be configured through either an LSM specific
pseudo-filesystem interface or alternatively through additional calls
to lsm_set_self_attr(2).
Once the configuration process is complete, the process would be set
free in its new namespace with the LSM_ATTR_UNSHARE attribute.
Separating from a security policy namespace to a new namespace will be
one of the most security sensitive operations that a system can
execute. As such it has to be gated by some type of security control.
At a minimum this needs to be uid-0 or posession of CAP_MAC_ADMIN.
Given the current LSM concept of stacking, there needs to be an LSM
security hooks assigned, so as to give all of the LSM's an opportunity
to accept or deny the attribute operations.
For example, it would seem entirely reasonable that the lockdown LSM
may want to deny the ability to create any departures from the current
security configuration.
Making this generic for any security namespace will require some
additional plumbing, most notably the ability for any LSM to register
for the ability to receive namespace event notifications. If we
create new security_task_secns_init() and
security_task_secns_unshare() hooks we could use those as both
notification and security control mechanisms.
So lots of details to discuss but the above should be about the most
generic implementation that can be leveraged by all of the LSM's.
Comments/suggestion welcome.
Have a good remainder of the week.
As always,
Dr. Greg
The Quixote Project - Flailing at the Travails of Cybersecurity
https://github.com/Quixote-Project
^ permalink raw reply
* Re: [PATCH 4/4] keys, trusted: Remove redundant helper
From: Jonathan McDowell @ 2025-09-24 8:29 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Jarkko Sakkinen, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250922164318.3540792-5-jarkko@kernel.org>
On Mon, Sep 22, 2025 at 07:43:17PM +0300, Jarkko Sakkinen wrote:
>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").
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Seems like a reasonable cleanup.
Reviewed-by: Jonathan McDowell <noodles@earth.li>
>---
> 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
>
>
J.
--
If a program is useful, it must be changed.
^ permalink raw reply
* Re: [PATCH 1/4] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Jonathan McDowell @ 2025-09-24 8:32 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, Stefano Garzarella, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250922164318.3540792-2-jarkko@kernel.org>
On Mon, Sep 22, 2025 at 07:43:14PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>Using -EFAULT here was not the best idea for tpm_ret_to_err as the fallback
>error code as it is no concise with trusted keys.
>
>Change the fallback as -EPERM, process TPM_RC_HASH also in tpm_ret_to_err,
>and by these changes make the helper applicable for trusted keys.
>
>Fixes: 539fbab37881 ("tpm: Mask TPM RC in tpm2_start_auth_session()")
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
> include/linux/tpm.h | 9 +++++---
> security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
> 2 files changed, 13 insertions(+), 22 deletions(-)
>
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index dc0338a783f3..667d290789ca 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -449,13 +449,16 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
> if (ret < 0)
> return ret;
>
>- switch (tpm2_rc_value(ret)) {
>- case TPM2_RC_SUCCESS:
>+ if (!ret)
> return 0;
Fold this into the check above to get:
if (ret <= 0)
?
>+
>+ switch (tpm2_rc_value(ret)) {
> case TPM2_RC_SESSION_MEMORY:
> return -ENOMEM;
>+ case TPM2_RC_HASH:
>+ return -EINVAL;
> default:
>- return -EFAULT;
>+ return -EPERM;
> }
> }
>
>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
>
>
J.
--
Web [ Sleep? Isn't that some inferior replacement for caffeine? ]
site: https:// [ ] Made by
www.earth.li/~noodles/ [ ] HuggieTag 0.0.24
^ permalink raw reply
* Re: [PATCH] tpm2-sessions: Remove unnecessary wrapper
From: Jonathan McDowell @ 2025-09-24 8:35 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, 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: <20250922115009.3053664-1-jarkko@kernel.org>
On Mon, Sep 22, 2025 at 02:50:09PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>Open code tpm_buf_append_hmac_session_opt() because it adds unnecessary
>disperancy to the call sites (and reduces the amount of code).
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
I don't have a strong opinion on whether this is significantly better,
but for 2 call sites it's not unreasonable, so:
Reviewed-By: Jonathan McDowell <noodles@earth.li>
>---
> 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 7d77f6fbc152..89e6169add88 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 667d290789ca..aaa407ddef21 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -534,29 +534,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 attributes, 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
>
>
J.
--
"Where else in computing can a random government that isn't even in
your country force you to make a change to your servers on three day's
notice?" -- Russ Allbery on DST
^ permalink raw reply
* Re: [PATCH 2/4] tpm2-sessions: Remove unused parameter from tpm_buf_append_auth
From: Jonathan McDowell @ 2025-09-24 8:47 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
Mimi Zohar, Roberto Sassu, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250922164318.3540792-3-jarkko@kernel.org>
On Mon, Sep 22, 2025 at 07:43:15PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>In earlier bug fix, an extra parameter was by mistake to the function.
>
>Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
> drivers/char/tpm/tpm2-cmd.c | 2 +-
> drivers/char/tpm/tpm2-sessions.c | 5 ++---
> include/linux/tpm.h | 25 +------------------------
> 3 files changed, 4 insertions(+), 28 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>index 7d77f6fbc152..61a4daaef292 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;
> }
This chunk below should be in patch 3/4 where you open code
tpm_buf_append_hmac_session_opt, rather than here:
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index 667d290789ca..a8984d273c28 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -533,30 +533,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);
>-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);
>- }
>-}
>+ u8 *passphrase, int passphraselen);
>
> #ifdef CONFIG_TCG_TPM2_HMAC
>
>--
>2.39.5
>
>
J.
--
101 things you can't have too much of : 19 - A Good Thing.
This .sig brought to you by the letter V and the number 13
Product of the Republic of HuggieTag
^ permalink raw reply
* Re: [RFC PATCH 1/6] landlock: Add a place for flags to layer rules
From: Mickaël Salaün @ 2025-09-24 9:20 UTC (permalink / raw)
To: Tingmao Wang; +Cc: Günther Noack, Jan Kara, linux-security-module
In-Reply-To: <a43a9985-cf62-482b-9a2d-fce463ca69b0@maowtm.org>
On Mon, Sep 22, 2025 at 12:52:19AM +0100, Tingmao Wang wrote:
> On 9/19/25 17:02, Mickaël Salaün wrote:
> > On Tue, Sep 09, 2025 at 01:06:35AM +0100, Tingmao Wang wrote:
> >> To avoid unnecessarily increasing the size of struct landlock_layer, we
> >> make the layer level a u8 and use the space to store the flags struct.
> >>
> >> Signed-off-by: Tingmao Wang <m@maowtm.org>
> >> ---
> >> security/landlock/fs.c | 75 ++++++++++++++++++++++++-------------
> >> security/landlock/net.c | 3 +-
> >> security/landlock/ruleset.c | 9 ++++-
> >> security/landlock/ruleset.h | 27 ++++++++++++-
> >> 4 files changed, 83 insertions(+), 31 deletions(-)
> >>
> >> [...]
> >> @@ -643,6 +644,12 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
> >> unsigned long access_bit;
> >> bool is_empty;
> >>
> >> + if (rule_flags) {
> >> + /* Collect rule flags for each layer */
> >> + if (layer->flags.quiet)
> >> + rule_flags->quiet_masks |= layer_bit;
> >
> > This patch makes quiet flags related to on object, but not tied to the
> > rule access rights (as explained in the next patch's doc). To tie it to
> > rule access rights would require to duplicate the access bits for each
> > rule (because multiple rules can be tied to the same object for the same
> > layer/ruleset).
>
> (imo the use of "rule" here as a terminology is a bit confusing, I would
> have thought that a "rule" is a collection of access rights associated
> with an object, and therefore you can of course only have one rule per
> object in a ruleset. Otherwise landlock_add_rule should really have been
> called landlock_add_rules...?)
A rule is indeed a set of access rights tied to an object. However,
when the same object is used with two calls to landlock_add_rule(), the
ruleset's rule is the union of both access rights.
What I wanted to highlight is that the quiet flag is unrelated to the
access rights, which makes sense because the otherwise it would never be
denied and then never logged.
>
> >
> > So, the question is, what do we really need to mute?
> >
> > I think the current approach is enough. We could still add a new flag in
> > the future, or maybe even a new field to each rule type. However, we
> > should rename the flag to make it clear that the it's related to the
> > rule's object which is muted instead of the whole rule. Maybe something
> > like LANDLOCK_ADD_RULE_QUIET_OBJECT?
>
> I don't see what benefit a new field to each rule type would bring, since
> different rule types targets different objects and live in different
> rbtrees, and so they are already separable.
Adding a new field to rule types would allow to only mute denied
requests if they match an access right, instead of muting all
non-allowed access rights. For instance, if an app tries to do an ioctl
because of legacy/compatibility reasons, we may want to ignore such
request, but we may still want to log malicious attempts to modify
regular files (in the same file hierarchy).
>
> >
> > If we want to have a more fine-grained control, a complementary patch
> > could add a bitfield for each access right type to quiet a denied access
> > right iff the object is also quiet (where rules are possible). That
> > could be a follow up to complete this quiet feature, but this patch
> > series is good on its own.
>
> Worth noting that if one really wants to suppress logging for only some
> access bits and we do not add support for it (due to the extra overhead
> etc), that is still doable with just this patch by using two layers - the
This hack should definitely not be encouraged, otherwise the limit of 16
layers will shrink to 8 (or less) in practice.
> outer one would contain the intended rules but not have any quiet flags,
> whereas the inner one would contain the same set of rules but with quiet
> flags set, except that for access bits which the sandbox does not want to
> be quiet, it would also "allow" them in the second layer (access would
> still be denied by the first layer, but would get audit logged due to
> quiet flag not applying when the younger layer allows the access).
>
> But this gets very tricky in the context of mutable domains, and does not
> work at all for the purpose of controlling whether supervisor mode would
> delegate to the supervisor or deny outright, since supervisors are
> "accumulative". Therefore if this (different "quietness" for different
> access bits) becomes a strong need, we should probably consider some way
> of implementing it, rather than expecting a sandboxer to do this two-layer
> hack. (But implementing this does have the potential to result in needing
> to have a (number of access bits) x (number of layers) matrix...)
Yes, that will indeed increase the size of rules, which is why I'm not
sure if it worth it.
The alternative I was thinking about is to only increase the size of
struct landlock_ruleset (which would be negligible) to include sets of
quiet access rights. A request to such access right *on* a quiet object
would never be logged. I think this approach is flexible enough and
doesn't increase much the complexity. This would also be useful to not
log access rights that don't have associated rules (e.g. scopes), and
then no identified objects. To avoid the kind of hack you pointed out,
this feature should probably be part of this patch series though. What
do you think?
>
> > [...]
>
> Will add suggested changes in v2. Thanks for the review :)
>
> Kind regards,
> Tingmao
>
^ permalink raw reply
* Re: [PATCH v10 1/4] tpm: Make TPM buffer allocations more robust
From: Stefano Garzarella @ 2025-09-24 10:10 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, Jarkko Sakkinen, Stefan Berger, Peter Huewe,
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: <aNKu_Mm0tupokMmZ@kernel.org>
On Tue, Sep 23, 2025 at 05:30:20PM +0300, Jarkko Sakkinen wrote:
>On Mon, Sep 22, 2025 at 10:44:34AM +0200, Stefano Garzarella wrote:
>> On Sun, Sep 21, 2025 at 05:08:01AM +0300, Jarkko Sakkinen wrote:
>> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> >
>> > Create more ergonomic primitives to work with TPM buffers, where
>> > 'tpm_buf_init*' initialize the memory and 'tpm_buf_reset*' (re)set a buffer
>> > for a particular use and purpose. The new primitives are ubiquitos when
>> > it comes to heap and stack usage.
>> >
>> > Given that the kzalloc is decoupled, a managed allocation can be done
>> > trivially:
>> >
>> > struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUF_MAX_SIZE,
>> > GFP_KERNEL);
>> >
>> > This effectively zeros out the odds having any memory leaks with TPM
>> > buffers. The new structures can be later used to widen the use of stack
>> > over heap in the subsystem in the critical code paths..
>> >
>> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> > ---
>> > v10:
>> > - No changes.
>> > v9:
>> > - Rename pre-existing TPM_BUFSIZE as TPM_BUF_MAX_SIZE and redeclare
>> > it in include/linux/tpm.h, and use this constant instead of
>> > PAGE_SIZE in tpm_buf_init*. Define TPM_BUF_MIN_SIZE to be a
>> > sane placeholder value for stack allocations.
>> > - Fix and restructure invariant for the sake of clarity.
>> > - memset buffers to zero in tpm_buf_init* in order to guarantee a legit
>> > initial state. This does not cause any regressions but once tpm_buf
>> > is allocaed from stack at some site, this will zero out chance of
>> > corrupted state.
>> > v8:
>> > - Decouple memory init i.e. bring tpm_init* back but with new fresh
>> > form.
>> > - Cap buf_size to page size and also make checks in tpm_buf_append
>> > safe:
>> > https://lore.kernel.org/linux-integrity/hwsx2t2tkbos3g7k2syemxbvc6sfrsbviwm64ubcdl6ms7ljvo@toetomkhheht/
>> > - Fix trusted_tpm_send() and simplify the flow.
>> > v7:
>> > - Additional function comments and invariant was unfortunately left to
>> > my staging area so here's the addition (does not affect functionality).
>> > v6:
>> > - Removed two empty lines as requested by Stefan:
>> > https://lore.kernel.org/linux-integrity/be1c5bef-7c97-4173-b417-986dc90d779c@linux.ibm.com/
>> > - Add 'capacity' field as this makes easy to stretch tpm_buf into stack
>> > allocation.
>> > v5:
>> > - I tested this version also with TPM 1.2 by booting up and checking
>> > that sysfs attributes work.
>> > - Fixed the length check against capacity (James) with TPM_BUF_CAPACITY.
>> > - Fixed declaration style: do it at the site (Jason).
>> > - Improved commit message (Stefan).
>> > - Removed "out" label from tpm2_pcr_read() (Stefan).
>> > - Removed spurious "return rc;" from tpm2_get_pcr_allocation() (Stefan).
>> > v4:
>> > - Wrote a more a descriptive short summary and improved description.
>> > - Fixed the error in documentation: there is 4090 bytes of space left
>> > for the payload - not 4088 bytes.
>> > - Turned tpm_buf_alloc() into inline function.
>> > v3:
>> > - Removed the cleanup class and moved on using __free(kfree) instead.
>> > - Removed `buf_size` (James).
>> > - I'm open for the idea of splitting still (Jason) but I'll hold
>> > at least this revision just to check that my core idea here
>> > is correct.
>> > v2:
>> > - Implement also memory allocation using the cleanup class.
>> > - Rewrote the commit message.
>> > - Implemented CLASS_TPM_BUF() helper macro.
>> > ---
>> > drivers/char/tpm/tpm-buf.c | 137 +++++++----
>> > drivers/char/tpm/tpm-dev-common.c | 4 +-
>> > drivers/char/tpm/tpm-dev.h | 2 +-
>> > drivers/char/tpm/tpm-interface.c | 4 +-
>> > drivers/char/tpm/tpm-sysfs.c | 20 +-
>> > drivers/char/tpm/tpm.h | 3 +-
>> > drivers/char/tpm/tpm1-cmd.c | 149 ++++++------
>> > drivers/char/tpm/tpm2-cmd.c | 282 ++++++++++------------
>> > drivers/char/tpm/tpm2-sessions.c | 121 +++++-----
>> > drivers/char/tpm/tpm2-space.c | 44 ++--
>> > drivers/char/tpm/tpm_tis_i2c.c | 4 +-
>> > drivers/char/tpm/tpm_vtpm_proxy.c | 32 ++-
>> > include/linux/tpm.h | 28 ++-
>> > security/keys/trusted-keys/trusted_tpm1.c | 34 ++-
>> > security/keys/trusted-keys/trusted_tpm2.c | 156 ++++++------
>> > 15 files changed, 493 insertions(+), 527 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
>> > index dc882fc9fa9e..82dce0350a41 100644
>> > --- a/drivers/char/tpm/tpm-buf.c
>> > +++ b/drivers/char/tpm/tpm-buf.c
>> > @@ -7,82 +7,107 @@
>> > #include <linux/module.h>
>> > #include <linux/tpm.h>
>> >
>> > -/**
>> > - * tpm_buf_init() - Allocate and initialize a TPM command
>> > - * @buf: A &tpm_buf
>> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal: A command ordinal
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > - */
>> > -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_size_invariant(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > - if (!buf->data)
>> > - return -ENOMEM;
>> > -
>> > - tpm_buf_reset(buf, tag, ordinal);
>> > - return 0;
>> > + if (!buf->capacity) {
>> > + if (buf_size > TPM_BUF_MAX_SIZE) {
>> > + WARN(1, "%s: size overflow: %u\n", __func__, buf_size);
>>
>> IIUC here we will always print something like:
>> "__tpm_buf_size_invariant: size overflow: XXX"
>>
>> So, should we just remove `__func__` or maybe use a macro to print the name
>> of the caller?
>
>It also prints a stack trace.
Oh, I see, thanks!
>
>>
>> > + buf->flags |= TPM_BUF_ERROR;
>> > + }
>> > + } else {
>> > + if (buf_size != buf->capacity + sizeof(*buf)) {
>> > + WARN(1, "%s: size mismatch: %u != %u\n", __func__, buf_size,
>> > + buf->capacity + sizeof(*buf));
>> > + buf->flags |= TPM_BUF_ERROR;
>> > + }
>> > + }
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > -/**
>> > - * tpm_buf_reset() - Initialize a TPM command
>> > - * @buf: A &tpm_buf
>> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal: A command ordinal
>> > - */
>> > -void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
>> > {
>> > struct tpm_header *head = (struct tpm_header *)buf->data;
>> >
>> > + __tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > + if (buf->flags & TPM_BUF_ERROR)
>> > + return;
>> > +
>> > WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
>> > tag != TPM2_ST_SESSIONS && tag != 0);
>> >
>> > buf->flags = 0;
>> > buf->length = sizeof(*head);
>> > + buf->capacity = buf_size - sizeof(*buf);
>> > + buf->handles = 0;
>> > head->tag = cpu_to_be16(tag);
>> > head->length = cpu_to_be32(sizeof(*head));
>> > head->ordinal = cpu_to_be32(ordinal);
>> > +}
>> > +
>> > +static void __tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > + __tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > + if (buf->flags & TPM_BUF_ERROR)
>> > + return;
>> > +
>> > + buf->flags = TPM_BUF_TPM2B;
>> > + buf->length = 2;
>> > + buf->capacity = buf_size - sizeof(*buf);
>> > buf->handles = 0;
>> > + buf->data[0] = 0;
>> > + buf->data[1] = 0;
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > /**
>> > - * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
>> > - * @buf: A @tpm_buf
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > + * tpm_buf_init() - Initialize a TPM command
>> > + * @buf: A &tpm_buf
>> > + * @buf_size: Size of the buffer.
>> > */
>> > -int tpm_buf_init_sized(struct tpm_buf *buf)
>> > +void tpm_buf_init(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > - if (!buf->data)
>> > - return -ENOMEM;
>> > + memset(buf, 0, buf_size);
>> > + __tpm_buf_reset(buf, buf_size, TPM_TAG_RQU_COMMAND, 0);
>> > +}
>> > +EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > - tpm_buf_reset_sized(buf);
>> > - return 0;
>> > +/**
>> > + * tpm_buf_init_sized() - Initialize a sized buffer
>> > + * @buf: A &tpm_buf
>> > + * @buf_size: Size of the buffer.
>> > + */
>> > +void tpm_buf_init_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > + memset(buf, 0, buf_size);
>> > + __tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
>> >
>> > /**
>> > - * tpm_buf_reset_sized() - Initialize a sized buffer
>> > + * tpm_buf_reset() - Re-initialize a TPM command
>> > * @buf: A &tpm_buf
>> > + * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > + * @ordinal: A command ordinal
>> > */
>> > -void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > {
>> > - buf->flags = TPM_BUF_TPM2B;
>> > - buf->length = 2;
>> > - buf->data[0] = 0;
>> > - buf->data[1] = 0;
>> > + u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > + __tpm_buf_reset(buf, buf_size, tag, ordinal);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > -void tpm_buf_destroy(struct tpm_buf *buf)
>> > +/**
>> > + * tpm_buf_reset_sized() - Re-initialize a sized buffer
>> > + * @buf: A &tpm_buf
>> > + */
>> > +void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > {
>> > - free_page((unsigned long)buf->data);
>> > + u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > + __tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> >
>> > /**
>> > * tpm_buf_length() - Return the number of bytes consumed by the data
>> > @@ -92,6 +117,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> > */
>> > u32 tpm_buf_length(struct tpm_buf *buf)
>> > {
>> > + if (buf->flags & TPM_BUF_ERROR)
>> > + return 0;
>> > +
>> > return buf->length;
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_length);
>> > @@ -104,13 +132,14 @@ 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)
>> > + u32 total_length = (u32)buf->length + (u32)new_length;
>> > +
>> > + if (buf->flags & TPM_BUF_ERROR)
>> > return;
>> >
>> > - if ((buf->length + new_length) > PAGE_SIZE) {
>> > + if (total_length > (u32)buf->capacity) {
>> > WARN(1, "tpm_buf: write overflow\n");
>> > - buf->flags |= TPM_BUF_OVERFLOW;
>> > + buf->flags |= TPM_BUF_ERROR;
>> > return;
>> > }
>> >
>> > @@ -157,8 +186,12 @@ 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_ERROR)
>> > + return;
>> > +
>> > if (buf->flags & TPM_BUF_TPM2B) {
>> > - dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
>> > + dev_err(&chip->dev, "%s: invalid for buffer type: TPM2B\n", __func__);
>> > + buf->flags |= TPM_BUF_ERROR;
>> > return;
>> > }
>> >
>> > @@ -178,13 +211,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_ERROR)
>> > 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_ERROR;
>> > return;
>> > }
>> >
>> > @@ -242,5 +275,3 @@ u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
>> > return be32_to_cpu(value);
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_read_u32);
>> > -
>> > -
>> > diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
>> > index f2a5e09257dd..4f5893555fb7 100644
>> > --- a/drivers/char/tpm/tpm-dev-common.c
>> > +++ b/drivers/char/tpm/tpm-dev-common.c
>> > @@ -147,7 +147,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>> >
>> > rc = copy_to_user(buf, priv->data_buffer + *off, ret_size);
>> > if (rc) {
>> > - memset(priv->data_buffer, 0, TPM_BUFSIZE);
>> > + memset(priv->data_buffer, 0, TPM_BUF_MAX_SIZE);
>> > priv->response_length = 0;
>> > ret_size = -EFAULT;
>> > } else {
>> > @@ -173,7 +173,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>> > struct file_priv *priv = file->private_data;
>> > int ret = 0;
>> >
>> > - if (size > TPM_BUFSIZE)
>> > + if (size > TPM_BUF_MAX_SIZE)
>> > return -E2BIG;
>> >
>> > mutex_lock(&priv->buffer_mutex);
>> > diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
>> > index f3742bcc73e3..700e3d9d8b64 100644
>> > --- a/drivers/char/tpm/tpm-dev.h
>> > +++ b/drivers/char/tpm/tpm-dev.h
>> > @@ -18,7 +18,7 @@ struct file_priv {
>> > bool response_read;
>> > bool command_enqueued;
>> >
>> > - u8 data_buffer[TPM_BUFSIZE];
>> > + u8 data_buffer[TPM_BUF_MAX_SIZE];
>> > };
>> >
>> > void tpm_common_open(struct file *file, struct tpm_chip *chip,
>> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> > index c9f173001d0e..b0d5098fb92b 100644
>> > --- a/drivers/char/tpm/tpm-interface.c
>> > +++ b/drivers/char/tpm/tpm-interface.c
>> > @@ -100,8 +100,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> > if (bufsiz < TPM_HEADER_SIZE)
>> > return -EINVAL;
>> >
>> > - if (bufsiz > TPM_BUFSIZE)
>> > - bufsiz = TPM_BUFSIZE;
>> > + if (bufsiz > TPM_BUF_MAX_SIZE)
>> > + bufsiz = TPM_BUF_MAX_SIZE;
>> >
>> > count = be32_to_cpu(header->length);
>> > ordinal = be32_to_cpu(header->ordinal);
>> > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
>> > index 94231f052ea7..4213a8285ed0 100644
>> > --- a/drivers/char/tpm/tpm-sysfs.c
>> > +++ b/drivers/char/tpm/tpm-sysfs.c
>> > @@ -32,28 +32,30 @@ struct tpm_readpubek_out {
>> > static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
>> > char *buf)
>> > {
>> > - struct tpm_buf tpm_buf;
>> > struct tpm_readpubek_out *out;
>> > int i;
>> > char *str = buf;
>> > struct tpm_chip *chip = to_tpm_chip(dev);
>> > char anti_replay[20];
>> >
>> > + struct tpm_buf *tpm_buf __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
>>
>> We're using PAGE_SIZE instead of TPM_BUF_MAX_SIZE to reduce the pressure on
>> the allocator, right?
>>
>> I was wondering if we could create an inline function or a macro that calls
>> kzalloc() and tpm_buf_init().
>> Just because we do it often, it's not a strong opinion, just something that
>> came to mind while doing the review.
>> I mean something like this (untested):
>>
>> struct tpm_buf *tpm_buf_alloc_max(void) {
>> struct tpm_buf *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> if (!buf)
>> return NULL;
>>
>> tpm_buf_init(buf, TPM_BUF_MAX_SIZE);
>> return buf;
>> }
>
>With or without this, about same about of boiler plate is introduced to
>the call site. It's easier to understand the code later on when kzalloc
>alloc is clearly at site. Finally I think for most uses the driver could
>favor stack over heap (but that requires prepare the tpm_buf like this).
Yep, I see.
Stefano
^ permalink raw reply
* Re: [syzbot] [fs?] BUG: sleeping function called from invalid context in hook_sb_delete
From: Jan Kara @ 2025-09-24 11:05 UTC (permalink / raw)
To: syzbot
Cc: brauner, jack, linux-fsdevel, linux-kernel, syzkaller-bugs, viro,
Mickaël Salaün, linux-security-module,
Günther Noack
In-Reply-To: <68d32659.a70a0220.4f78.0012.GAE@google.com>
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().
Honza
> security_sb_delete+0x80/0x150 security/security.c:1467
> generic_shutdown_super+0xaa/0x2c0 fs/super.c:634
> kill_anon_super fs/super.c:1281 [inline]
> kill_litter_super+0x76/0xb0 fs/super.c:1291
> deactivate_locked_super+0xbc/0x130 fs/super.c:473
> cleanup_mnt+0x425/0x4c0 fs/namespace.c:1327
> task_work_run+0x1d4/0x260 kernel/task_work.c:227
> resume_user_mode_work include/linux/resume_user_mode.h:50 [inline]
> exit_to_user_mode_loop+0xe9/0x130 kernel/entry/common.c:43
> exit_to_user_mode_prepare include/linux/irq-entry-common.h:225 [inline]
> syscall_exit_to_user_mode_work include/linux/entry-common.h:175 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:210 [inline]
> do_syscall_64+0x2bd/0xfa0 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7fc08e18eec9
> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007ffcd5efff18 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
> RAX: 0000000000000000 RBX: 00007fc08e3e5fa0 RCX: 00007fc08e18eec9
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00002000000002c0
> RBP: 00007fc08e211f91 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007fc08e3e5fa0 R14: 00007fc08e3e5fa0 R15: 0000000000000002
> </TASK>
>
>
> ---
> 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.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
>
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
>
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
>
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
>
> If you want to undo deduplication, reply with:
> #syz undup
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 4/4] keys, trusted: Remove redundant helper
From: Jarkko Sakkinen @ 2025-09-24 17:12 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, Jarkko Sakkinen, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <aNOr4_xLQ30iTRSe@earth.li>
On Wed, Sep 24, 2025 at 09:29:23AM +0100, Jonathan McDowell wrote:
> On Mon, Sep 22, 2025 at 07:43:17PM +0300, Jarkko Sakkinen wrote:
> > 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").
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Seems like a reasonable cleanup.
>
> Reviewed-by: Jonathan McDowell <noodles@earth.li>
Thank you!
BTW, I applied your patches to master:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/
I did not yet bother to test them other than compilation and checkpatch
but I'm testing this and tpm_buf-series so they will get a lot of stress
testing. I'll rebase 'next' once they are actually tested (before PR).
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 1/4] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Jarkko Sakkinen @ 2025-09-24 17:16 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, Stefano Garzarella, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNOsoVOZPXns6WB2@earth.li>
On Wed, Sep 24, 2025 at 09:32:33AM +0100, Jonathan McDowell wrote:
> On Mon, Sep 22, 2025 at 07:43:14PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Using -EFAULT here was not the best idea for tpm_ret_to_err as the fallback
> > error code as it is no concise with trusted keys.
> >
> > Change the fallback as -EPERM, process TPM_RC_HASH also in tpm_ret_to_err,
> > and by these changes make the helper applicable for trusted keys.
> >
> > Fixes: 539fbab37881 ("tpm: Mask TPM RC in tpm2_start_auth_session()")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > include/linux/tpm.h | 9 +++++---
> > security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
> > 2 files changed, 13 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index dc0338a783f3..667d290789ca 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -449,13 +449,16 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
> > if (ret < 0)
> > return ret;
> >
> > - switch (tpm2_rc_value(ret)) {
> > - case TPM2_RC_SUCCESS:
> > + if (!ret)
> > return 0;
>
> Fold this into the check above to get:
>
> if (ret <= 0)
>
> ?
This is really a glitch in this patch, and I think following what
Stefano suggested is the right call:
https://lore.kernel.org/linux-integrity/tnxfamnvxoanaihka3em7ktmzkervoea43zn2l3mqxvnuivb6n@p5nn34vns3zf/
I.e., a random change to something that was not broken in the first
place :-) Never a good idea (not even in microscale), except something
super cosmetic like maybe grouping constants and stylistic stuff like
that.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 2/4] tpm2-sessions: Remove unused parameter from tpm_buf_append_auth
From: Jarkko Sakkinen @ 2025-09-24 17:18 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
Mimi Zohar, Roberto Sassu, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <aNOwP99WNsDgBVof@earth.li>
On Wed, Sep 24, 2025 at 09:47:59AM +0100, Jonathan McDowell wrote:
> On Mon, Sep 22, 2025 at 07:43:15PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > In earlier bug fix, an extra parameter was by mistake to the function.
> >
> > Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > drivers/char/tpm/tpm2-cmd.c | 2 +-
> > drivers/char/tpm/tpm2-sessions.c | 5 ++---
> > include/linux/tpm.h | 25 +------------------------
> > 3 files changed, 4 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 7d77f6fbc152..61a4daaef292 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;
> > }
>
> This chunk below should be in patch 3/4 where you open code
> tpm_buf_append_hmac_session_opt, rather than here:
True, thanks for catching this.
>
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 667d290789ca..a8984d273c28 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -533,30 +533,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);
> > -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);
> > - }
> > -}
> > + u8 *passphrase, int passphraselen);
> >
> > #ifdef CONFIG_TCG_TPM2_HMAC
> >
> > --
> > 2.39.5
> >
> >
>
> J.
>
> --
> 101 things you can't have too much of : 19 - A Good Thing.
> This .sig brought to you by the letter V and the number 13
> Product of the Republic of HuggieTag
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] tpm2-sessions: Remove unnecessary wrapper
From: Jarkko Sakkinen @ 2025-09-24 17:26 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe, 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: <aNOtTIRBrzN_AAMa@earth.li>
On Wed, Sep 24, 2025 at 09:35:24AM +0100, Jonathan McDowell wrote:
> On Mon, Sep 22, 2025 at 02:50:09PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Open code tpm_buf_append_hmac_session_opt() because it adds unnecessary
> > disperancy to the call sites (and reduces the amount of code).
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> I don't have a strong opinion on whether this is significantly better, but
> for 2 call sites it's not unreasonable, so:
>
> Reviewed-By: Jonathan McDowell <noodles@earth.li>
I'll explain a bit of context.
I'm opening tpm2-session knot by knot and once it is like out and naked
my grand plan is to create a single function tpm2_authenticate or similar
function that wraps a TPM command into an authenticated TPM command.
The main priority with each patch is to do at least microscopic amount
of more than just pure clean up. The tpm_buf series and this series
sort of converge towards the same common goal. I think this is a good
way to move forward and refactor complex functionality without causing
major risks in production.
They key topic on this all maximizing the decoupling and it goes beyond
just "fixing tpm2-sessions". By doing this process we can more easily
re-use parts of the driver code in early boot code and sort of create
enablers for e.g., Trenchboot.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH kvm-next V11 0/7] Add NUMA mempolicy support for KVM guest-memfd
From: David Hildenbrand @ 2025-09-24 18:19 UTC (permalink / raw)
To: Shivank Garg, willy, akpm, pbonzini, shuah, seanjc, vbabka
Cc: brauner, viro, dsterba, xiang, chao, jaegeuk, clm, josef,
kent.overstreet, zbestahu, jefflexu, dhavale, lihongbo22,
lorenzo.stoakes, Liam.Howlett, rppt, surenb, mhocko, ziy,
matthew.brost, joshua.hahnjy, rakie.kim, byungchul, gourry,
ying.huang, apopple, tabba, ackerleytng, paul, jmorris, serge,
pvorel, bfoster, vannapurve, chao.gao, bharata, nikunj,
michael.day, shdhiman, yan.y.zhao, Neeraj.Upadhyay,
thomas.lendacky, michael.roth, aik, jgg, kalyazin, peterx, jack,
hch, cgzones, ira.weiny, rientjes, roypat, chao.p.peng, amit,
ddutile, dan.j.williams, ashish.kalra, gshan, jgowans,
pankaj.gupta, papaluri, yuzhao, suzuki.poulose, quic_eberman,
linux-bcachefs, linux-btrfs, linux-erofs, linux-f2fs-devel,
linux-fsdevel, linux-mm, linux-kernel, linux-security-module, kvm,
linux-kselftest, linux-coco
In-Reply-To: <20250827175247.83322-2-shivankg@amd.com>
On 27.08.25 19:52, Shivank Garg wrote:
> This series introduces NUMA-aware memory placement support for KVM guests
> with guest_memfd memory backends. It builds upon Fuad Tabba's work (V17)
> that enabled host-mapping for guest_memfd memory [1] and can be applied
> directly applied on KVM tree [2] (branch kvm-next, base commit: a6ad5413,
> Merge branch 'guest-memfd-mmap' into HEAD)
>
Heads-up: I'll queue this (incl. the replacement patch for #4 from the
reply) and send it tomorrow as a PR against kvm/next to Paolo.
--
Cheers
David / dhildenb
^ permalink raw reply
* Re: [RFC PATCH v2] lsm,selinux: introduce LSM_ATTR_UNSHARE and wire it up for SELinux
From: Casey Schaufler @ 2025-09-24 18:11 UTC (permalink / raw)
To: Dr. Greg, Stephen Smalley
Cc: linux-security-module, selinux, paul, omosnace, john.johansen,
serge, Casey Schaufler
In-Reply-To: <20250924080840.GA16185@wind.enjellic.com>
On 9/24/2025 1:08 AM, Dr. Greg wrote:
> On Thu, Sep 18, 2025 at 09:59:05AM -0400, Stephen Smalley wrote:
>
> Good morning, I hope the week is going well for everyone.
>
>> 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.
>>
>> 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.
>>
>> 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.
>>
>> 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).
>>
>> 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.
> We had a number of exchanges regarding LSM namespacing in the thread
> that Paul Moore started on this issue:
>
> https://lore.kernel.org/linux-security-module/CAHC9VhRGMmhxbajwQNfGFy+ZFF1uN=UEBjqQZQ4UBy7yds3eVQ@mail.gmail.com/
>
> The one issue that seemed to achieve universal consensus was that
> every LSM was going to have different requirements for namespacing.
>
> At the risk of playing devil's advocate, this seems to raise the
> question as to whether or not there is a need to have a common API for
> requesting security namespace separation or leave the issue to LSM
> specific implementations.
It does raise the question. I, for one, would like to see the
LSM infrastructure move toward being a bit more helpful regarding
things that the various LSMs do that can be done commonly. One
example of this is moving blob allocations out of the individual
modules. Another is the introduction of lsm properties
(struct lsm_prop) to replace the SELinux specific secid model.
When there where five LSMs total, only two to be used at a time,
leaving everything to the individual module was mildly tolerable.
Today we see that there's a heap of waste in data management and
duplicate code when even a base Fedora uses eight at a time.
I don't see this as a matter of taste. I see this as a case where
we can make the LSM infrastructure a little less chaotic.
> The primary rationale for some modicum of centralized infrastructure
> would seem to be to have a system call rather than an LSM specific
> pseudo-filesystem interface to control security namespaces. Since
> creating a system call interface is going to lock the API in stone it
> would seem that we would want to get this right, or at least as
> generic as possibe.
OK.
> So some comments to that end.
>
> If we use the lsm_set_self_attr(2) system call as our approach, the
> namespace separation process needs to be split into two separate
> calls. One to request the creation of a namespace and a second call
> to request that the process join the new namespace.
>
> This is required in order to support the ability for an orchestration
> process to load a policy or model and have it in place before the new
> namespace is allowed to enforce the policy or model.
>
> So we would need something like an LSM_ATTR_UNSHARE_INIT as well as
> the LSM_ATTR_UNSHARE attribute.
Stephen does not require this in his SELinux implementation.
Nor did the abandoned Smack namespaces. On the other hand, I
can see where a system with a dynamic security configuration
such as yours might indeed need this.
> So the model would be for a process to issue an LSM_ATTR_UNSHARE_INIT
> call to create the new security context namespace. That namespace
> context can then be configured through either an LSM specific
> pseudo-filesystem interface or alternatively through additional calls
> to lsm_set_self_attr(2).
>
> Once the configuration process is complete, the process would be set
> free in its new namespace with the LSM_ATTR_UNSHARE attribute.
>
> Separating from a security policy namespace to a new namespace will be
> one of the most security sensitive operations that a system can
> execute. As such it has to be gated by some type of security control.
>
> At a minimum this needs to be uid-0 or posession of CAP_MAC_ADMIN.
> Given the current LSM concept of stacking, there needs to be an LSM
> security hooks assigned, so as to give all of the LSM's an opportunity
> to accept or deny the attribute operations.
You've got paradigm conflict developing here. SELinux (for example)
would prefer to make its own decisions regarding "security relevant"
operations. Landlock, which is "unprivileged", ought to be able to
perform its namespace operations without undue interference. While
I generally think that all LSMs ought to integrate with capabilities,
but I have to admit to the status quo.
> For example, it would seem entirely reasonable that the lockdown LSM
> may want to deny the ability to create any departures from the current
> security configuration.
Bluntly, that's not gonna happen. It would break Smack for one thing.
> Making this generic for any security namespace will require some
> additional plumbing, most notably the ability for any LSM to register
> for the ability to receive namespace event notifications. If we
> create new security_task_secns_init() and
> security_task_secns_unshare() hooks we could use those as both
> notification and security control mechanisms.
>
> So lots of details to discuss but the above should be about the most
> generic implementation that can be leveraged by all of the LSM's.
>
> Comments/suggestion welcome.
>
> Have a good remainder of the week.
If I must. :)
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity
> https://github.com/Quixote-Project
^ permalink raw reply
* Re: [PATCH kvm-next V11 0/7] Add NUMA mempolicy support for KVM guest-memfd
From: Kalra, Ashish @ 2025-09-24 20:35 UTC (permalink / raw)
To: David Hildenbrand, Shivank Garg, willy, akpm, pbonzini, shuah,
seanjc, vbabka
Cc: brauner, viro, dsterba, xiang, chao, jaegeuk, clm, josef,
kent.overstreet, zbestahu, jefflexu, dhavale, lihongbo22,
lorenzo.stoakes, Liam.Howlett, rppt, surenb, mhocko, ziy,
matthew.brost, joshua.hahnjy, rakie.kim, byungchul, gourry,
ying.huang, apopple, tabba, ackerleytng, paul, jmorris, serge,
pvorel, bfoster, vannapurve, chao.gao, bharata, nikunj,
michael.day, shdhiman, yan.y.zhao, Neeraj.Upadhyay,
thomas.lendacky, michael.roth, aik, jgg, kalyazin, peterx, jack,
hch, cgzones, ira.weiny, rientjes, roypat, chao.p.peng, amit,
ddutile, dan.j.williams, gshan, jgowans, pankaj.gupta, papaluri,
yuzhao, suzuki.poulose, quic_eberman, linux-bcachefs, linux-btrfs,
linux-erofs, linux-f2fs-devel, linux-fsdevel, linux-mm,
linux-kernel, linux-security-module, kvm, linux-kselftest,
linux-coco
In-Reply-To: <1b01ebab-a43e-4344-ae38-50f0a031332f@redhat.com>
Tested the patch series by auditing the actual userspace (HVA) mappings and seeing if the
corresponding physical PFNs correspond to the expected NUMA node.
Enabled QEMU's kvm_set_user_memory tracepoint to dump the HVA/guest_memfd/guest_memfd_offset/base GPA/size.
This helped determine the HVAs and the memslot that QEMU registers with KVM via the kvm_set_user_memory_region() helper.
After that dumped the PFNs getting mapped into the guest for a particular GPA via enabling the
kvm_mmu_set_spte kernel trace events, performed the GPA->memslot->HVA mapping (via QEMU traces above) and then looked in
/proc/<qemu_pid>/numa_maps to validate the HVA is bound to the NUMA node associated with that memslot/guest_memfd.
Additionally, looked up the PFN (from kernel traces) in /proc/zoneinfo to validate that the physical page belongs to the
NUMA node associated with the memslot/guest_memfd.
This testing/validation is based on the following trees:
Host Kernel:
https://github.com/AMDESE/linux/commits/snp-hugetlb-v2-wip0/
This tree is based on commit 27cb583e25d0 from David Hildenbrand's guestmemfd_preview tree
(which already includes base mmap support) with Google's HugeTLB v2 patches rebased on top of those
(which include both in-place conversion and hugetlb infrastructure), along with additional
patches to enable in-place conversion and hugetlb for SNP.
QEMU:
https://github.com/AMDESE/qemu/commits/snp-hugetlb-dev-wip0/
QEMU command line used for testing/validation:
qemu-system-x86_64 --enable-kvm -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,convert-in-place=true
-object memory-backend-memfd,id=ram0,host-nodes=0,policy=bind,size=150000M,prealloc=false
-numa node,nodeid=0,memdev=ram0,cpus=0-31,cpus=64-95
-object memory-backend-memfd,id=ram1,host-nodes=1,policy=bind,size=150000M,prealloc=false
-numa node,nodeid=1,memdev=ram1,cpus=32-63,cpus=96-127
(guest NUMA configuration mapped 1:1 to host NUMA configuration).
Tested-by: Ashish Kalra <ashish.kalra@amd.com>
Thanks,
Ashish
On 9/24/2025 1:19 PM, David Hildenbrand wrote:
> On 27.08.25 19:52, Shivank Garg wrote:
>> This series introduces NUMA-aware memory placement support for KVM guests
>> with guest_memfd memory backends. It builds upon Fuad Tabba's work (V17)
>> that enabled host-mapping for guest_memfd memory [1] and can be applied
>> directly applied on KVM tree [2] (branch kvm-next, base commit: a6ad5413,
>> Merge branch 'guest-memfd-mmap' into HEAD)
>>
>
> Heads-up: I'll queue this (incl. the replacement patch for #4 from the reply) and send it tomorrow as a PR against kvm/next to Paolo.
>
^ permalink raw reply
* Re: [bug report] [regression?] bpf lsm breaks /proc/*/attr/current with security= on commandline
From: Paul Moore @ 2025-09-24 21:24 UTC (permalink / raw)
To: Filip Hejsek
Cc: linux-security-module, James Morris, Serge E. Hallyn, bpf,
linux-kernel, regressions
In-Reply-To: <e5d594d0aee93da67a22a42d0e2b4e6e463ab894.camel@gmail.com>
On Sat, Sep 13, 2025 at 1:01 PM Filip Hejsek <filip.hejsek@gmail.com> wrote:
>
> Hello,
>
> TLDR: because of bpf lsm, putting security=selinux on commandline
> results in /proc/*/attr/current returning errors.
>
> When the legacy security= commandline option is used, the specified lsm
> is added to the end of the lsm list. For example, security=apparmor
> results in the following order of security modules:
>
> capability,landlock,lockdown,yama,bpf,apparmor
>
> In particular, the bpf lsm will be ordered before the chosen major lsm.
>
> This causes reads and writes of /proc/*/attr/current to fail, because
> the bpf hook overrides the apparmor/selinux hook.
What kernel are you using? Things appear to work correctly on my
kernel that is tracking upstream (Fedora Rawhide + some unrelated
bits):
% uname -a
Linux dev-rawhide-1.lan 6.17.0-0.rc7.250923gd1ab3.57.1.secnext.fc44.x86_64 #1 SM
P PREEMPT_DYNAMIC Tue Sep 23 10:07:14 EDT 2025 x86_64 GNU/Linux
% cat /proc/cmdline
BOOT_IMAGE=(hd0,gpt4)/boot/vmlinuz-6.17.0-0.rc7.250923gd1ab3.57.1.secnext.fc44.x
86_64 root=UUID=285029fa-4431-45e9-af1b-298ab0caf16a ro console=ttyS0 mitigation
s=off security=selinux
% cat /sys/kernel/security/lsm; echo ""
lockdown,capability,yama,selinux,bpf,landlock,ipe,ima,evm
% id -Z
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
% cat /proc/self/attr/current; echo ""
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
I even ran it against the LSM initialization rework that has been
proposed, but has not yet been accepted/merged, and that worked the
same as above.
Is this a distro kernel with a lot of "special" patches which aren't
present upstream?
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes
From: Sean Christopherson @ 2025-09-25 2:50 UTC (permalink / raw)
To: Ackerley Tng
Cc: Shivank Garg, willy, akpm, david, pbonzini, shuah, vbabka,
brauner, viro, dsterba, xiang, chao, jaegeuk, clm, josef,
kent.overstreet, zbestahu, jefflexu, dhavale, lihongbo22,
lorenzo.stoakes, Liam.Howlett, rppt, surenb, mhocko, ziy,
matthew.brost, joshua.hahnjy, rakie.kim, byungchul, gourry,
ying.huang, apopple, tabba, paul, jmorris, serge, pvorel, bfoster,
vannapurve, chao.gao, bharata, nikunj, michael.day, shdhiman,
yan.y.zhao, Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik,
jgg, kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes,
roypat, chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra,
gshan, jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <diqztt1sbd2v.fsf@google.com>
My apologies for the super late feedback. None of this is critical (mechanical
things that can be cleaned up after the fact), so if there's any urgency to
getting this series into 6.18, just ignore it.
On Wed, Aug 27, 2025, Ackerley Tng wrote:
> Shivank Garg <shivankg@amd.com> writes:
> @@ -463,11 +502,70 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
> return true;
> }
>
> +static struct inode *kvm_gmem_inode_create(const char *name, loff_t size,
> + u64 flags)
> +{
> + struct inode *inode;
> +
> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
> + if (IS_ERR(inode))
> + return inode;
> +
> + inode->i_private = (void *)(unsigned long)flags;
> + inode->i_op = &kvm_gmem_iops;
> + inode->i_mapping->a_ops = &kvm_gmem_aops;
> + inode->i_mode |= S_IFREG;
> + inode->i_size = size;
> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> + mapping_set_inaccessible(inode->i_mapping);
> + /* Unmovable mappings are supposed to be marked unevictable as well. */
> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> +
> + return inode;
> +}
> +
> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
> + u64 flags)
> +{
> + static const char *name = "[kvm-gmem]";
> + struct inode *inode;
> + struct file *file;
> + int err;
> +
> + err = -ENOENT;
> + /* __fput() will take care of fops_put(). */
> + if (!fops_get(&kvm_gmem_fops))
> + goto err;
> +
> + inode = kvm_gmem_inode_create(name, size, flags);
> + if (IS_ERR(inode)) {
> + err = PTR_ERR(inode);
> + goto err_fops_put;
> + }
> +
> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
> + &kvm_gmem_fops);
> + if (IS_ERR(file)) {
> + err = PTR_ERR(file);
> + goto err_put_inode;
> + }
> +
> + file->f_flags |= O_LARGEFILE;
> + file->private_data = priv;
> +
> + return file;
> +
> +err_put_inode:
> + iput(inode);
> +err_fops_put:
> + fops_put(&kvm_gmem_fops);
> +err:
> + return ERR_PTR(err);
> +}
I don't see any reason to add two helpers. It requires quite a bit more lines
of code due to adding more error paths and local variables, and IMO doesn't make
the code any easier to read.
Passing in "gmem" as @priv is especially ridiculous, as it adds code and
obfuscates what file->private_data is set to.
I get the sense that the code was written to be a "replacement" for common APIs,
but that is nonsensical (no pun intended).
> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> {
> - const char *anon_name = "[kvm-gmem]";
> struct kvm_gmem *gmem;
> - struct inode *inode;
> struct file *file;
> int fd, err;
>
> @@ -481,32 +579,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> goto err_fd;
> }
>
> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
> - O_RDWR, NULL);
> + file = kvm_gmem_inode_create_getfile(gmem, size, flags);
> if (IS_ERR(file)) {
> err = PTR_ERR(file);
> goto err_gmem;
> }
>
> - file->f_flags |= O_LARGEFILE;
> -
> - inode = file->f_inode;
> - WARN_ON(file->f_mapping != inode->i_mapping);
> -
> - inode->i_private = (void *)(unsigned long)flags;
> - inode->i_op = &kvm_gmem_iops;
> - inode->i_mapping->a_ops = &kvm_gmem_aops;
> - inode->i_mode |= S_IFREG;
> - inode->i_size = size;
> - mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> - mapping_set_inaccessible(inode->i_mapping);
> - /* Unmovable mappings are supposed to be marked unevictable as well. */
> - WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> -
> kvm_get_kvm(kvm);
> gmem->kvm = kvm;
> xa_init(&gmem->bindings);
> - list_add(&gmem->entry, &inode->i_mapping->i_private_list);
> + list_add(&gmem->entry, &file_inode(file)->i_mapping->i_private_list);
I don't understand this change? Isn't file_inode(file) == inode?
Compile tested only, and again not critical, but it's -40 LoC...
---
include/uapi/linux/magic.h | 1 +
virt/kvm/guest_memfd.c | 75 ++++++++++++++++++++++++++++++++------
virt/kvm/kvm_main.c | 7 +++-
virt/kvm/kvm_mm.h | 9 +++--
4 files changed, 76 insertions(+), 16 deletions(-)
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index bb575f3ab45e..638ca21b7a90 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -103,5 +103,6 @@
#define DEVMEM_MAGIC 0x454d444d /* "DMEM" */
#define SECRETMEM_MAGIC 0x5345434d /* "SECM" */
#define PID_FS_MAGIC 0x50494446 /* "PIDF" */
+#define GUEST_MEMFD_MAGIC 0x474d454d /* "GMEM" */
#endif /* __LINUX_MAGIC_H__ */
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 08a6bc7d25b6..73c9791879d5 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -1,12 +1,16 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/anon_inodes.h>
#include <linux/backing-dev.h>
#include <linux/falloc.h>
+#include <linux/fs.h>
#include <linux/kvm_host.h>
+#include <linux/pseudo_fs.h>
#include <linux/pagemap.h>
-#include <linux/anon_inodes.h>
#include "kvm_mm.h"
+static struct vfsmount *kvm_gmem_mnt;
+
struct kvm_gmem {
struct kvm *kvm;
struct xarray bindings;
@@ -385,9 +389,45 @@ static struct file_operations kvm_gmem_fops = {
.fallocate = kvm_gmem_fallocate,
};
-void kvm_gmem_init(struct module *module)
+static int kvm_gmem_init_fs_context(struct fs_context *fc)
+{
+ if (!init_pseudo(fc, GUEST_MEMFD_MAGIC))
+ return -ENOMEM;
+
+ fc->s_iflags |= SB_I_NOEXEC;
+ fc->s_iflags |= SB_I_NODEV;
+
+ return 0;
+}
+
+static struct file_system_type kvm_gmem_fs = {
+ .name = "guest_memfd",
+ .init_fs_context = kvm_gmem_init_fs_context,
+ .kill_sb = kill_anon_super,
+};
+
+static int kvm_gmem_init_mount(void)
+{
+ kvm_gmem_mnt = kern_mount(&kvm_gmem_fs);
+
+ if (IS_ERR(kvm_gmem_mnt))
+ return PTR_ERR(kvm_gmem_mnt);
+
+ kvm_gmem_mnt->mnt_flags |= MNT_NOEXEC;
+ return 0;
+}
+
+int kvm_gmem_init(struct module *module)
{
kvm_gmem_fops.owner = module;
+
+ return kvm_gmem_init_mount();
+}
+
+void kvm_gmem_exit(void)
+{
+ kern_unmount(kvm_gmem_mnt);
+ kvm_gmem_mnt = NULL;
}
static int kvm_gmem_migrate_folio(struct address_space *mapping,
@@ -465,7 +505,7 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
{
- const char *anon_name = "[kvm-gmem]";
+ static const char *name = "[kvm-gmem]";
struct kvm_gmem *gmem;
struct inode *inode;
struct file *file;
@@ -481,17 +521,17 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
goto err_fd;
}
- file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
- O_RDWR, NULL);
- if (IS_ERR(file)) {
- err = PTR_ERR(file);
+ /* __fput() will take care of fops_put(). */
+ if (!fops_get(&kvm_gmem_fops)) {
+ err = -ENOENT;
goto err_gmem;
}
- file->f_flags |= O_LARGEFILE;
-
- inode = file->f_inode;
- WARN_ON(file->f_mapping != inode->i_mapping);
+ inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
+ if (IS_ERR(inode)) {
+ err = PTR_ERR(inode);
+ goto err_fops;
+ }
inode->i_private = (void *)(unsigned long)flags;
inode->i_op = &kvm_gmem_iops;
@@ -503,6 +543,15 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
/* Unmovable mappings are supposed to be marked unevictable as well. */
WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
+ file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR, &kvm_gmem_fops);
+ if (IS_ERR(file)) {
+ err = PTR_ERR(file);
+ goto err_inode;
+ }
+
+ file->f_flags |= O_LARGEFILE;
+ file->private_data = gmem;
+
kvm_get_kvm(kvm);
gmem->kvm = kvm;
xa_init(&gmem->bindings);
@@ -511,6 +560,10 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
fd_install(fd, file);
return fd;
+err_inode:
+ iput(inode);
+err_fops:
+ fops_put(&kvm_gmem_fops);
err_gmem:
kfree(gmem);
err_fd:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 18f29ef93543..301d48d6e00d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -6489,7 +6489,9 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
if (WARN_ON_ONCE(r))
goto err_vfio;
- kvm_gmem_init(module);
+ r = kvm_gmem_init(module);
+ if (r)
+ goto err_gmem;
r = kvm_init_virtualization();
if (r)
@@ -6510,6 +6512,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
err_register:
kvm_uninit_virtualization();
err_virt:
+ kvm_gmem_exit();
+err_gmem:
kvm_vfio_ops_exit();
err_vfio:
kvm_async_pf_deinit();
@@ -6541,6 +6545,7 @@ void kvm_exit(void)
for_each_possible_cpu(cpu)
free_cpumask_var(per_cpu(cpu_kick_mask, cpu));
kmem_cache_destroy(kvm_vcpu_cache);
+ kvm_gmem_exit();
kvm_vfio_ops_exit();
kvm_async_pf_deinit();
kvm_irqfd_exit();
diff --git a/virt/kvm/kvm_mm.h b/virt/kvm/kvm_mm.h
index 31defb08ccba..9fcc5d5b7f8d 100644
--- a/virt/kvm/kvm_mm.h
+++ b/virt/kvm/kvm_mm.h
@@ -68,17 +68,18 @@ static inline void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm,
#endif /* HAVE_KVM_PFNCACHE */
#ifdef CONFIG_KVM_GUEST_MEMFD
-void kvm_gmem_init(struct module *module);
+int kvm_gmem_init(struct module *module);
+void kvm_gmem_exit(void);
int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
unsigned int fd, loff_t offset);
void kvm_gmem_unbind(struct kvm_memory_slot *slot);
#else
-static inline void kvm_gmem_init(struct module *module)
+static inline int kvm_gmem_init(struct module *module)
{
-
+ return 0;
}
-
+static inline void kvm_gmem_exit(void) {};
static inline int kvm_gmem_bind(struct kvm *kvm,
struct kvm_memory_slot *slot,
unsigned int fd, loff_t offset)
base-commit: d133892dddd6607de651b7e32510359a6af97c4c
--
^ permalink raw reply related
* Re: [PATCH] ima: setting security.ima to fix security.evm for a file with IMA signature
From: Mimi Zohar @ 2025-09-25 3:27 UTC (permalink / raw)
To: Coiby Xu
Cc: linux-integrity, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <4l7l4duxfximhzixruim3lnij5mhnlqs5srzycc6j6c2bu5zda@ogoj3ade37fd>
On Wed, 2025-09-24 at 16:03 +0800, Coiby Xu wrote:
> On Mon, Sep 15, 2025 at 05:05:42PM -0400, Mimi Zohar wrote:
> > On Wed, 2025-09-10 at 09:20 +0800, Coiby Xu wrote:
> > > On Tue, Sep 09, 2025 at 11:31:20AM -0400, Mimi Zohar wrote:
> > > > On Tue, 2025-09-09 at 12:19 +0800, Coiby Xu wrote:
> > > > > When both IMA and EVM fix modes are enabled, accessing a file with IMA
> > > > > signature won't cause security.evm to be fixed. But this doesn't happen
> > > > > to a file with correct IMA hash already set because accessing it will
> > > > > cause setting security.ima again which triggers fixing security.evm
> > > > > thanks to security_inode_post_setxattr->evm_update_evmxattr.
> > > > >
> > > > > Let's use the same mechanism to fix security.evm for a file with IMA
> > > > > signature.
> > > > >
> [...]
> > > > > ---
> > > > > security/integrity/ima/ima_appraise.c | 27 +++++++++++++++++++++------
> > > > > 1 file changed, 21 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > > > index f435eff4667f..18c3907c5e44 100644
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -595,12 +595,27 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
> > > > > integrity_audit_msg(audit_msgno, inode, filename,
> > > > > op, cause, rc, 0);
> > > > > } else if (status != INTEGRITY_PASS) {
> > > > > - /* Fix mode, but don't replace file signatures. */
> > > > > - if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
> > > > > - (!xattr_value ||
> > > > > - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> > > > > - if (!ima_fix_xattr(dentry, iint))
> > > > > - status = INTEGRITY_PASS;
> > > > > + /*
> > > > > + * Fix mode, but don't replace file signatures.
> > > > > + *
> > > > > + * When EVM fix mode is also enabled, security.evm will be
> > > > > + * fixed automatically when security.ima is set because of
> > > > > + * security_inode_post_setxattr->evm_update_evmxattr.
> > > > > + */
> > > > > + if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
> > > > > + if (!xattr_value ||
> > > > > + xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
> > > > > + if (ima_fix_xattr(dentry, iint))
> > > > > + status = INTEGRITY_PASS;
> > > > > + } else if (xattr_value->type == EVM_IMA_XATTR_DIGSIG &&
> > > > > + evm_revalidate_status(XATTR_NAME_IMA)) {
> > > > > + if (!__vfs_setxattr_noperm(&nop_mnt_idmap,
> > > > > + dentry,
> > > > > + XATTR_NAME_IMA,
> > > > > + xattr_value,
> > > > > + xattr_len, 0))
> > > > > + status = INTEGRITY_PASS;
> > > > > + }
> > > > > }
> >
> > Instead of re-writing the IMA signature without a clear explanation, define a
> > new EVM function named evm_fix_hmac() and add a call here in IMA. Only in EVM
> > fix mode would evm_fix_hmac() update the EVM hmac.
> >
> > } else if (status != INTEGRITY_PASS) {
> > /*
> > * IMA fix mode updates the IMA file hash, which triggers EVM
> > * to update security.evm. ....
> > *
> > * Similarly, trigger fixing EVM HMAC for IMA file signatures.
> > */
> > if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
> > if (!xattr_value ||
> > xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
> > if (ima_fix_xattr(dentry, iint))
> > status = INTEGRITY_PASS;
> > } else if (status == INTEGRITY_NOLABEL) {
> > evm_fix_hmac(dentry, XATTR_NAME_IMA, ....);
> > }
> > }
>
> Thanks for the advice! I wonder if we should use existing
> evm_update_evmxattr instead of defining a new EVM function.
>
> /*
> * Calculate the hmac and update security.evm xattr
> *
> * Expects to be called with i_mutex locked.
> */
> int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> const char *xattr_value, size_t xattr_value_len)
> {
> }
>
>
> I already tried evm_update_evmxattr and can confirm it works. But later
> I switched to __vfs_setxattr_noperm because I thought it's consistent
> with current logic of adding security.evm when there is already correct
> security.ima and it's a slightly smaller change.
Calling evm_inode_updatexattr() is limited to EVM. Only after verifying the
existing EVM value is evm_inode_updatexattr() called. For example, in
evm_inode_setxattr() the existing EVM value is verified and then updated in
evm_inode_post_setxattr(), by calling evm_inode_updatexattr().
In this case, the new function evm_fix_hmac() would call evm_update_evmxattr()
only after verifying the EVM mode is set to fix.
^ permalink raw reply
* Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes
From: Garg, Shivank @ 2025-09-25 11:44 UTC (permalink / raw)
To: Sean Christopherson, Ackerley Tng
Cc: willy, akpm, david, pbonzini, shuah, vbabka, brauner, viro,
dsterba, xiang, chao, jaegeuk, clm, josef, kent.overstreet,
zbestahu, jefflexu, dhavale, lihongbo22, lorenzo.stoakes,
Liam.Howlett, rppt, surenb, mhocko, ziy, matthew.brost,
joshua.hahnjy, rakie.kim, byungchul, gourry, ying.huang, apopple,
tabba, paul, jmorris, serge, pvorel, bfoster, vannapurve,
chao.gao, bharata, nikunj, michael.day, shdhiman, yan.y.zhao,
Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik, jgg,
kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes, roypat,
chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra, gshan,
jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <aNSt9QT8dmpDK1eE@google.com>
On 9/25/2025 8:20 AM, Sean Christopherson wrote:
> My apologies for the super late feedback. None of this is critical (mechanical
> things that can be cleaned up after the fact), so if there's any urgency to
> getting this series into 6.18, just ignore it.
>
> On Wed, Aug 27, 2025, Ackerley Tng wrote:
>> Shivank Garg <shivankg@amd.com> writes:
>> @@ -463,11 +502,70 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
>> return true;
>> }
>>
>> +static struct inode *kvm_gmem_inode_create(const char *name, loff_t size,
>> + u64 flags)
>> +{
>> + struct inode *inode;
>> +
>> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
>> + if (IS_ERR(inode))
>> + return inode;
>> +
>> + inode->i_private = (void *)(unsigned long)flags;
>> + inode->i_op = &kvm_gmem_iops;
>> + inode->i_mapping->a_ops = &kvm_gmem_aops;
>> + inode->i_mode |= S_IFREG;
>> + inode->i_size = size;
>> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>> + mapping_set_inaccessible(inode->i_mapping);
>> + /* Unmovable mappings are supposed to be marked unevictable as well. */
>> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>> +
>> + return inode;
>> +}
>> +
>> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>> + u64 flags)
>> +{
>> + static const char *name = "[kvm-gmem]";
>> + struct inode *inode;
>> + struct file *file;
>> + int err;
>> +
>> + err = -ENOENT;
>> + /* __fput() will take care of fops_put(). */
>> + if (!fops_get(&kvm_gmem_fops))
>> + goto err;
>> +
>> + inode = kvm_gmem_inode_create(name, size, flags);
>> + if (IS_ERR(inode)) {
>> + err = PTR_ERR(inode);
>> + goto err_fops_put;
>> + }
>> +
>> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
>> + &kvm_gmem_fops);
>> + if (IS_ERR(file)) {
>> + err = PTR_ERR(file);
>> + goto err_put_inode;
>> + }
>> +
>> + file->f_flags |= O_LARGEFILE;
>> + file->private_data = priv;
>> +
>> + return file;
>> +
>> +err_put_inode:
>> + iput(inode);
>> +err_fops_put:
>> + fops_put(&kvm_gmem_fops);
>> +err:
>> + return ERR_PTR(err);
>> +}
>
> I don't see any reason to add two helpers. It requires quite a bit more lines
> of code due to adding more error paths and local variables, and IMO doesn't make
> the code any easier to read.
>
> Passing in "gmem" as @priv is especially ridiculous, as it adds code and
> obfuscates what file->private_data is set to.
>
> I get the sense that the code was written to be a "replacement" for common APIs,
> but that is nonsensical (no pun intended).
>
>> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>> {
>> - const char *anon_name = "[kvm-gmem]";
>> struct kvm_gmem *gmem;
>> - struct inode *inode;
>> struct file *file;
>> int fd, err;
>>
>> @@ -481,32 +579,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>> goto err_fd;
>> }
>>
>> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
>> - O_RDWR, NULL);
>> + file = kvm_gmem_inode_create_getfile(gmem, size, flags);
>> if (IS_ERR(file)) {
>> err = PTR_ERR(file);
>> goto err_gmem;
>> }
>>
>> - file->f_flags |= O_LARGEFILE;
>> -
>> - inode = file->f_inode;
>> - WARN_ON(file->f_mapping != inode->i_mapping);
>> -
>> - inode->i_private = (void *)(unsigned long)flags;
>> - inode->i_op = &kvm_gmem_iops;
>> - inode->i_mapping->a_ops = &kvm_gmem_aops;
>> - inode->i_mode |= S_IFREG;
>> - inode->i_size = size;
>> - mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>> - mapping_set_inaccessible(inode->i_mapping);
>> - /* Unmovable mappings are supposed to be marked unevictable as well. */
>> - WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>> -
>> kvm_get_kvm(kvm);
>> gmem->kvm = kvm;
>> xa_init(&gmem->bindings);
>> - list_add(&gmem->entry, &inode->i_mapping->i_private_list);
>> + list_add(&gmem->entry, &file_inode(file)->i_mapping->i_private_list);
>
> I don't understand this change? Isn't file_inode(file) == inode?
>
> Compile tested only, and again not critical, but it's -40 LoC...
>
>
Thanks.
I did functional testing and it works fine.
> ---
> include/uapi/linux/magic.h | 1 +
> virt/kvm/guest_memfd.c | 75 ++++++++++++++++++++++++++++++++------
> virt/kvm/kvm_main.c | 7 +++-
> virt/kvm/kvm_mm.h | 9 +++--
> 4 files changed, 76 insertions(+), 16 deletions(-)
>
> diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
> index bb575f3ab45e..638ca21b7a90 100644
> --- a/include/uapi/linux/magic.h
> +++ b/include/uapi/linux/magic.h
> @@ -103,5 +103,6 @@
> #define DEVMEM_MAGIC 0x454d444d /* "DMEM" */
> #define SECRETMEM_MAGIC 0x5345434d /* "SECM" */
> #define PID_FS_MAGIC 0x50494446 /* "PIDF" */
> +#define GUEST_MEMFD_MAGIC 0x474d454d /* "GMEM" */
>
> #endif /* __LINUX_MAGIC_H__ */
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 08a6bc7d25b6..73c9791879d5 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -1,12 +1,16 @@
> // SPDX-License-Identifier: GPL-2.0
> +#include <linux/anon_inodes.h>
> #include <linux/backing-dev.h>
> #include <linux/falloc.h>
> +#include <linux/fs.h>
> #include <linux/kvm_host.h>
> +#include <linux/pseudo_fs.h>
> #include <linux/pagemap.h>
> -#include <linux/anon_inodes.h>
>
> #include "kvm_mm.h"
>
> +static struct vfsmount *kvm_gmem_mnt;
> +
> struct kvm_gmem {
> struct kvm *kvm;
> struct xarray bindings;
> @@ -385,9 +389,45 @@ static struct file_operations kvm_gmem_fops = {
> .fallocate = kvm_gmem_fallocate,
> };
>
> -void kvm_gmem_init(struct module *module)
> +static int kvm_gmem_init_fs_context(struct fs_context *fc)
> +{
> + if (!init_pseudo(fc, GUEST_MEMFD_MAGIC))
> + return -ENOMEM;
> +
> + fc->s_iflags |= SB_I_NOEXEC;
> + fc->s_iflags |= SB_I_NODEV;
> +
> + return 0;
> +}
> +
> +static struct file_system_type kvm_gmem_fs = {
> + .name = "guest_memfd",
> + .init_fs_context = kvm_gmem_init_fs_context,
> + .kill_sb = kill_anon_super,
> +};
> +
> +static int kvm_gmem_init_mount(void)
> +{
> + kvm_gmem_mnt = kern_mount(&kvm_gmem_fs);
> +
> + if (IS_ERR(kvm_gmem_mnt))
> + return PTR_ERR(kvm_gmem_mnt);
> +
> + kvm_gmem_mnt->mnt_flags |= MNT_NOEXEC;
> + return 0;
> +}
> +
> +int kvm_gmem_init(struct module *module)
> {
> kvm_gmem_fops.owner = module;
> +
> + return kvm_gmem_init_mount();
> +}
> +
> +void kvm_gmem_exit(void)
> +{
> + kern_unmount(kvm_gmem_mnt);
> + kvm_gmem_mnt = NULL;
> }
>
> static int kvm_gmem_migrate_folio(struct address_space *mapping,
> @@ -465,7 +505,7 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
>
> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> {
> - const char *anon_name = "[kvm-gmem]";
> + static const char *name = "[kvm-gmem]";
> struct kvm_gmem *gmem;
> struct inode *inode;
> struct file *file;
> @@ -481,17 +521,17 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> goto err_fd;
> }
>
> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
> - O_RDWR, NULL);
> - if (IS_ERR(file)) {
> - err = PTR_ERR(file);
> + /* __fput() will take care of fops_put(). */
> + if (!fops_get(&kvm_gmem_fops)) {
> + err = -ENOENT;
> goto err_gmem;
> }
>
> - file->f_flags |= O_LARGEFILE;
> -
> - inode = file->f_inode;
> - WARN_ON(file->f_mapping != inode->i_mapping);
> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
> + if (IS_ERR(inode)) {
> + err = PTR_ERR(inode);
> + goto err_fops;
> + }
>
> inode->i_private = (void *)(unsigned long)flags;
> inode->i_op = &kvm_gmem_iops;
> @@ -503,6 +543,15 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> /* Unmovable mappings are supposed to be marked unevictable as well. */
> WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>
> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR, &kvm_gmem_fops);
> + if (IS_ERR(file)) {
> + err = PTR_ERR(file);
> + goto err_inode;
> + }
> +
> + file->f_flags |= O_LARGEFILE;
> + file->private_data = gmem;
> +
> kvm_get_kvm(kvm);
> gmem->kvm = kvm;
> xa_init(&gmem->bindings);
> @@ -511,6 +560,10 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> fd_install(fd, file);
> return fd;
>
> +err_inode:
> + iput(inode);
> +err_fops:
> + fops_put(&kvm_gmem_fops);
> err_gmem:
> kfree(gmem);
> err_fd:
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 18f29ef93543..301d48d6e00d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -6489,7 +6489,9 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
> if (WARN_ON_ONCE(r))
> goto err_vfio;
>
> - kvm_gmem_init(module);
> + r = kvm_gmem_init(module);
> + if (r)
> + goto err_gmem;
>
> r = kvm_init_virtualization();
> if (r)
> @@ -6510,6 +6512,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
> err_register:
> kvm_uninit_virtualization();
> err_virt:
> + kvm_gmem_exit();
> +err_gmem:
> kvm_vfio_ops_exit();
> err_vfio:
> kvm_async_pf_deinit();
> @@ -6541,6 +6545,7 @@ void kvm_exit(void)
> for_each_possible_cpu(cpu)
> free_cpumask_var(per_cpu(cpu_kick_mask, cpu));
> kmem_cache_destroy(kvm_vcpu_cache);
> + kvm_gmem_exit();
> kvm_vfio_ops_exit();
> kvm_async_pf_deinit();
> kvm_irqfd_exit();
> diff --git a/virt/kvm/kvm_mm.h b/virt/kvm/kvm_mm.h
> index 31defb08ccba..9fcc5d5b7f8d 100644
> --- a/virt/kvm/kvm_mm.h
> +++ b/virt/kvm/kvm_mm.h
> @@ -68,17 +68,18 @@ static inline void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm,
> #endif /* HAVE_KVM_PFNCACHE */
>
> #ifdef CONFIG_KVM_GUEST_MEMFD
> -void kvm_gmem_init(struct module *module);
> +int kvm_gmem_init(struct module *module);
> +void kvm_gmem_exit(void);
> int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
> int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> unsigned int fd, loff_t offset);
> void kvm_gmem_unbind(struct kvm_memory_slot *slot);
> #else
> -static inline void kvm_gmem_init(struct module *module)
> +static inline int kvm_gmem_init(struct module *module)
> {
> -
> + return 0;
> }
> -
> +static inline void kvm_gmem_exit(void) {};
> static inline int kvm_gmem_bind(struct kvm *kvm,
> struct kvm_memory_slot *slot,
> unsigned int fd, loff_t offset)
>
> base-commit: d133892dddd6607de651b7e32510359a6af97c4c
> --
^ permalink raw reply
* Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes
From: David Hildenbrand @ 2025-09-25 11:55 UTC (permalink / raw)
To: Garg, Shivank, Sean Christopherson, Ackerley Tng
Cc: willy, akpm, pbonzini, shuah, vbabka, brauner, viro, dsterba,
xiang, chao, jaegeuk, clm, josef, kent.overstreet, zbestahu,
jefflexu, dhavale, lihongbo22, lorenzo.stoakes, Liam.Howlett,
rppt, surenb, mhocko, ziy, matthew.brost, joshua.hahnjy,
rakie.kim, byungchul, gourry, ying.huang, apopple, tabba, paul,
jmorris, serge, pvorel, bfoster, vannapurve, chao.gao, bharata,
nikunj, michael.day, shdhiman, yan.y.zhao, Neeraj.Upadhyay,
thomas.lendacky, michael.roth, aik, jgg, kalyazin, peterx, jack,
hch, cgzones, ira.weiny, rientjes, roypat, chao.p.peng, amit,
ddutile, dan.j.williams, ashish.kalra, gshan, jgowans,
pankaj.gupta, papaluri, yuzhao, suzuki.poulose, quic_eberman,
linux-bcachefs, linux-btrfs, linux-erofs, linux-f2fs-devel,
linux-fsdevel, linux-mm, linux-kernel, linux-security-module, kvm,
linux-kselftest, linux-coco
In-Reply-To: <dc6eb85f-87b6-43a1-b1f7-4727c0b834cc@amd.com>
On 25.09.25 13:44, Garg, Shivank wrote:
>
>
> On 9/25/2025 8:20 AM, Sean Christopherson wrote:
>> My apologies for the super late feedback. None of this is critical (mechanical
>> things that can be cleaned up after the fact), so if there's any urgency to
>> getting this series into 6.18, just ignore it.
>>
>> On Wed, Aug 27, 2025, Ackerley Tng wrote:
>>> Shivank Garg <shivankg@amd.com> writes:
>>> @@ -463,11 +502,70 @@ bool __weak kvm_arch_supports_gmem_mmap(struct kvm *kvm)
>>> return true;
>>> }
>>>
>>> +static struct inode *kvm_gmem_inode_create(const char *name, loff_t size,
>>> + u64 flags)
>>> +{
>>> + struct inode *inode;
>>> +
>>> + inode = anon_inode_make_secure_inode(kvm_gmem_mnt->mnt_sb, name, NULL);
>>> + if (IS_ERR(inode))
>>> + return inode;
>>> +
>>> + inode->i_private = (void *)(unsigned long)flags;
>>> + inode->i_op = &kvm_gmem_iops;
>>> + inode->i_mapping->a_ops = &kvm_gmem_aops;
>>> + inode->i_mode |= S_IFREG;
>>> + inode->i_size = size;
>>> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>>> + mapping_set_inaccessible(inode->i_mapping);
>>> + /* Unmovable mappings are supposed to be marked unevictable as well. */
>>> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>>> +
>>> + return inode;
>>> +}
>>> +
>>> +static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>>> + u64 flags)
>>> +{
>>> + static const char *name = "[kvm-gmem]";
>>> + struct inode *inode;
>>> + struct file *file;
>>> + int err;
>>> +
>>> + err = -ENOENT;
>>> + /* __fput() will take care of fops_put(). */
>>> + if (!fops_get(&kvm_gmem_fops))
>>> + goto err;
>>> +
>>> + inode = kvm_gmem_inode_create(name, size, flags);
>>> + if (IS_ERR(inode)) {
>>> + err = PTR_ERR(inode);
>>> + goto err_fops_put;
>>> + }
>>> +
>>> + file = alloc_file_pseudo(inode, kvm_gmem_mnt, name, O_RDWR,
>>> + &kvm_gmem_fops);
>>> + if (IS_ERR(file)) {
>>> + err = PTR_ERR(file);
>>> + goto err_put_inode;
>>> + }
>>> +
>>> + file->f_flags |= O_LARGEFILE;
>>> + file->private_data = priv;
>>> +
>>> + return file;
>>> +
>>> +err_put_inode:
>>> + iput(inode);
>>> +err_fops_put:
>>> + fops_put(&kvm_gmem_fops);
>>> +err:
>>> + return ERR_PTR(err);
>>> +}
>>
>> I don't see any reason to add two helpers. It requires quite a bit more lines
>> of code due to adding more error paths and local variables, and IMO doesn't make
>> the code any easier to read.
>>
>> Passing in "gmem" as @priv is especially ridiculous, as it adds code and
>> obfuscates what file->private_data is set to.
>>
>> I get the sense that the code was written to be a "replacement" for common APIs,
>> but that is nonsensical (no pun intended).
>>
>>> static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>>> {
>>> - const char *anon_name = "[kvm-gmem]";
>>> struct kvm_gmem *gmem;
>>> - struct inode *inode;
>>> struct file *file;
>>> int fd, err;
>>>
>>> @@ -481,32 +579,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
>>> goto err_fd;
>>> }
>>>
>>> - file = anon_inode_create_getfile(anon_name, &kvm_gmem_fops, gmem,
>>> - O_RDWR, NULL);
>>> + file = kvm_gmem_inode_create_getfile(gmem, size, flags);
>>> if (IS_ERR(file)) {
>>> err = PTR_ERR(file);
>>> goto err_gmem;
>>> }
>>>
>>> - file->f_flags |= O_LARGEFILE;
>>> -
>>> - inode = file->f_inode;
>>> - WARN_ON(file->f_mapping != inode->i_mapping);
>>> -
>>> - inode->i_private = (void *)(unsigned long)flags;
>>> - inode->i_op = &kvm_gmem_iops;
>>> - inode->i_mapping->a_ops = &kvm_gmem_aops;
>>> - inode->i_mode |= S_IFREG;
>>> - inode->i_size = size;
>>> - mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
>>> - mapping_set_inaccessible(inode->i_mapping);
>>> - /* Unmovable mappings are supposed to be marked unevictable as well. */
>>> - WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
>>> -
>>> kvm_get_kvm(kvm);
>>> gmem->kvm = kvm;
>>> xa_init(&gmem->bindings);
>>> - list_add(&gmem->entry, &inode->i_mapping->i_private_list);
>>> + list_add(&gmem->entry, &file_inode(file)->i_mapping->i_private_list);
>>
>> I don't understand this change? Isn't file_inode(file) == inode?
>>
>> Compile tested only, and again not critical, but it's -40 LoC...
>>
>>
>
> Thanks.
> I did functional testing and it works fine.
I can queue this instead. I guess I can reuse the patch description and
add Sean as author + add his SOB (if he agrees).
Let me take a look at the patch later in more detail.
--
Cheers
David / dhildenb
^ permalink raw reply
* Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes
From: Sean Christopherson @ 2025-09-25 13:41 UTC (permalink / raw)
To: David Hildenbrand
Cc: Shivank Garg, Ackerley Tng, willy, akpm, pbonzini, shuah, vbabka,
brauner, viro, dsterba, xiang, chao, jaegeuk, clm, josef,
kent.overstreet, zbestahu, jefflexu, dhavale, lihongbo22,
lorenzo.stoakes, Liam.Howlett, rppt, surenb, mhocko, ziy,
matthew.brost, joshua.hahnjy, rakie.kim, byungchul, gourry,
ying.huang, apopple, tabba, paul, jmorris, serge, pvorel, bfoster,
vannapurve, chao.gao, bharata, nikunj, michael.day, shdhiman,
yan.y.zhao, Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik,
jgg, kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes,
roypat, chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra,
gshan, jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <b67dd7cd-2c1c-4566-badf-32082d8cd952@redhat.com>
On Thu, Sep 25, 2025, David Hildenbrand wrote:
> On 25.09.25 13:44, Garg, Shivank wrote:
> > On 9/25/2025 8:20 AM, Sean Christopherson wrote:
> > I did functional testing and it works fine.
>
> I can queue this instead. I guess I can reuse the patch description and add
> Sean as author + add his SOB (if he agrees).
Eh, Ackerley and Fuad did all the work. If I had provided feedback earlier,
this would have been handled in a new version. If they are ok with the changes,
I would prefer they remain co-authors.
Regarding timing, how much do people care about getting this into 6.18 in
particular? AFAICT, this hasn't gotten any coverage in -next, which makes me a
little nervous.
^ permalink raw reply
* Re: [PATCH kvm-next V11 4/7] KVM: guest_memfd: Use guest mem inodes instead of anonymous inodes
From: Fuad Tabba @ 2025-09-25 13:44 UTC (permalink / raw)
To: Sean Christopherson
Cc: David Hildenbrand, Shivank Garg, Ackerley Tng, willy, akpm,
pbonzini, shuah, vbabka, brauner, viro, dsterba, xiang, chao,
jaegeuk, clm, josef, kent.overstreet, zbestahu, jefflexu, dhavale,
lihongbo22, lorenzo.stoakes, Liam.Howlett, rppt, surenb, mhocko,
ziy, matthew.brost, joshua.hahnjy, rakie.kim, byungchul, gourry,
ying.huang, apopple, paul, jmorris, serge, pvorel, bfoster,
vannapurve, chao.gao, bharata, nikunj, michael.day, shdhiman,
yan.y.zhao, Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik,
jgg, kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes,
roypat, chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra,
gshan, jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <aNVFrZDAkHmgNNci@google.com>
On Thu, 25 Sept 2025 at 14:41, Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 25, 2025, David Hildenbrand wrote:
> > On 25.09.25 13:44, Garg, Shivank wrote:
> > > On 9/25/2025 8:20 AM, Sean Christopherson wrote:
> > > I did functional testing and it works fine.
> >
> > I can queue this instead. I guess I can reuse the patch description and add
> > Sean as author + add his SOB (if he agrees).
>
> Eh, Ackerley and Fuad did all the work. If I had provided feedback earlier,
> this would have been handled in a new version. If they are ok with the changes,
> I would prefer they remain co-authors.
These changes are ok by me.
/fuad
> Regarding timing, how much do people care about getting this into 6.18 in
> particular? AFAICT, this hasn't gotten any coverage in -next, which makes me a
> little nervous.
^ permalink raw reply
* Re: [PATCH kvm-next V11 5/7] KVM: guest_memfd: Add slab-allocated inode cache
From: Sean Christopherson @ 2025-09-25 14:05 UTC (permalink / raw)
To: Shivank Garg
Cc: willy, akpm, david, pbonzini, shuah, vbabka, brauner, viro,
dsterba, xiang, chao, jaegeuk, clm, josef, kent.overstreet,
zbestahu, jefflexu, dhavale, lihongbo22, lorenzo.stoakes,
Liam.Howlett, rppt, surenb, mhocko, ziy, matthew.brost,
joshua.hahnjy, rakie.kim, byungchul, gourry, ying.huang, apopple,
tabba, ackerleytng, paul, jmorris, serge, pvorel, bfoster,
vannapurve, chao.gao, bharata, nikunj, michael.day, shdhiman,
yan.y.zhao, Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik,
jgg, kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes,
roypat, chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra,
gshan, jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <20250827175247.83322-8-shivankg@amd.com>
On Wed, Aug 27, 2025, Shivank Garg wrote:
> Add dedicated inode structure (kvm_gmem_inode_info) and slab-allocated
> inode cache for guest memory backing, similar to how shmem handles inodes.
>
> This adds the necessary allocation/destruction functions and prepares
> for upcoming guest_memfd NUMA policy support changes.
>
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
> virt/kvm/guest_memfd.c | 70 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 6c66a0974055..356947d36a47 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -17,6 +17,15 @@ struct kvm_gmem {
> struct list_head entry;
> };
>
> +struct kvm_gmem_inode_info {
What about naming this simply gmem_inode?
> + struct inode vfs_inode;
> +};
> +
> +static inline struct kvm_gmem_inode_info *KVM_GMEM_I(struct inode *inode)
And then GMEM_I()?
And then (in a later follow-up if we target this for 6.18, or as a prep patch if
we push this out to 6.19), rename kvm_gmem to gmem_file?
That would make guest_memfd look a bit more like other filesystems, and I don't
see a need to preface the local structures and helpers with "kvm_", e.g. GMEM_I()
is analogous to x86's to_vmx() and to_svm().
As for renaming kvm_gmem => gmem_file, I wandered back into this code via Ackerley's
in-place conversion series, and it took me a good long while to remember the roles
of files vs. inodes in gmem. That's probably a sign that the code needs clarification
given that I wrote the original code. :-)
Leveraging an old discussion[*], my thought is to get to this:
/*
* A guest_memfd instance can be associated multiple VMs, each with its own
* "view" of the underlying physical memory.
*
* The gmem's inode is effectively the raw underlying physical storage, and is
* used to track properties of the physical memory, while each gmem file is
* effectively a single VM's view of that storage, and is used to track assets
* specific to its associated VM, e.g. memslots=>gmem bindings.
*/
struct gmem_file {
struct kvm *kvm;
struct xarray bindings;
struct list_head entry;
};
struct gmem_inode {
struct shared_policy policy;
struct inode vfs_inode;
};
[*] https://lore.kernel.org/all/ZLGiEfJZTyl7M8mS@google.com
^ permalink raw reply
* Re: [PATCH kvm-next V11 5/7] KVM: guest_memfd: Add slab-allocated inode cache
From: Sean Christopherson @ 2025-09-25 14:17 UTC (permalink / raw)
To: Shivank Garg
Cc: willy, akpm, david, pbonzini, shuah, vbabka, brauner, viro,
dsterba, xiang, chao, jaegeuk, clm, josef, kent.overstreet,
zbestahu, jefflexu, dhavale, lihongbo22, lorenzo.stoakes,
Liam.Howlett, rppt, surenb, mhocko, ziy, matthew.brost,
joshua.hahnjy, rakie.kim, byungchul, gourry, ying.huang, apopple,
tabba, ackerleytng, paul, jmorris, serge, pvorel, bfoster,
vannapurve, chao.gao, bharata, nikunj, michael.day, shdhiman,
yan.y.zhao, Neeraj.Upadhyay, thomas.lendacky, michael.roth, aik,
jgg, kalyazin, peterx, jack, hch, cgzones, ira.weiny, rientjes,
roypat, chao.p.peng, amit, ddutile, dan.j.williams, ashish.kalra,
gshan, jgowans, pankaj.gupta, papaluri, yuzhao, suzuki.poulose,
quic_eberman, linux-bcachefs, linux-btrfs, linux-erofs,
linux-f2fs-devel, linux-fsdevel, linux-mm, linux-kernel,
linux-security-module, kvm, linux-kselftest, linux-coco
In-Reply-To: <aNVMIRels8iCldOj@google.com>
On Thu, Sep 25, 2025, Sean Christopherson wrote:
> On Wed, Aug 27, 2025, Shivank Garg wrote:
> > Add dedicated inode structure (kvm_gmem_inode_info) and slab-allocated
> > inode cache for guest memory backing, similar to how shmem handles inodes.
> >
> > This adds the necessary allocation/destruction functions and prepares
> > for upcoming guest_memfd NUMA policy support changes.
> >
> > Signed-off-by: Shivank Garg <shivankg@amd.com>
> > ---
> > virt/kvm/guest_memfd.c | 70 ++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 6c66a0974055..356947d36a47 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -17,6 +17,15 @@ struct kvm_gmem {
> > struct list_head entry;
> > };
> >
> > +struct kvm_gmem_inode_info {
>
> What about naming this simply gmem_inode?
Heh, after looking through other filesystems, they're fairly even on appending
_info or not. My vote is definitely for gmem_inode.
Before we accumulate more inode usage, e.g. for in-place conversion (which is
actually why I started looking at this code), I think we should also settle on
naming for gmem_file and gmem_inode variables.
As below, "struct kvm_gmem *gmem" gets quite confusing once inodes are in the
picture, especially since that structure isn't _the_ gmem instance, rather it's
a VM's view of that gmem instance. And on the other side, "info" for the inode
is a bit imprecise, e.g. doesn't immediately make me think of inodes.
A few ideas:
(a)
struct gmem_inode *gmem;
struct gmem_file *f;
(b)
struct gmem_inode *gi;
struct gmem_file *f;
(c)
struct gmem_inode *gi;
struct gmem_file *gf;
(d)
struct gmem_inode *gmem_i;
struct gmem_file *gmem_f;
I think my would be for (a) or (b). Option (c) seems like it would be hard to
visually differentiate between "gi" and "gf", and gmem_{i,f} are a bit verbose
IMO.
> > + struct inode vfs_inode;
> > +};
> > +
> > +static inline struct kvm_gmem_inode_info *KVM_GMEM_I(struct inode *inode)
>
> And then GMEM_I()?
>
> And then (in a later follow-up if we target this for 6.18, or as a prep patch if
> we push this out to 6.19), rename kvm_gmem to gmem_file?
>
> That would make guest_memfd look a bit more like other filesystems, and I don't
> see a need to preface the local structures and helpers with "kvm_", e.g. GMEM_I()
> is analogous to x86's to_vmx() and to_svm().
>
> As for renaming kvm_gmem => gmem_file, I wandered back into this code via Ackerley's
> in-place conversion series, and it took me a good long while to remember the roles
> of files vs. inodes in gmem. That's probably a sign that the code needs clarification
> given that I wrote the original code. :-)
>
> Leveraging an old discussion[*], my thought is to get to this:
>
> /*
> * A guest_memfd instance can be associated multiple VMs, each with its own
> * "view" of the underlying physical memory.
> *
> * The gmem's inode is effectively the raw underlying physical storage, and is
> * used to track properties of the physical memory, while each gmem file is
> * effectively a single VM's view of that storage, and is used to track assets
> * specific to its associated VM, e.g. memslots=>gmem bindings.
> */
> struct gmem_file {
> struct kvm *kvm;
> struct xarray bindings;
> struct list_head entry;
> };
>
> struct gmem_inode {
> struct shared_policy policy;
> struct inode vfs_inode;
> };
>
> [*] https://lore.kernel.org/all/ZLGiEfJZTyl7M8mS@google.com
^ 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