* Re: [PATCH RESEND] apparmor: Replace deprecated strcpy with memcpy in gen_symlink_name
From: John Johansen @ 2025-11-27 1:32 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20251126165701.97158-2-thorsten.blum@linux.dev>
On 11/26/25 08:57, Thorsten Blum wrote:
> strcpy() is deprecated; use memcpy() instead. Unlike strcpy(), memcpy()
> does not copy the NUL terminator from the source string, which would be
> overwritten anyway on every iteration when using strcpy(). snprintf()
> then ensures that 'char *s' is NUL-terminated.
>
> Replace the hard-coded path length to remove the magic number 6, and add
> a comment explaining the extra 11 bytes.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
hey Thorsten,
sorry I have actually pulled these in, and tested them. I didn't send out
the acks yet because I have another patch that I was waiting on a proper
signed-off-by: on.
I am going to have to pull that one so I can push. I'll add acks now but
the push isn't going to happen for a few hours.
Acked-by: John Johansen <john.johansen@canonical.com>
> security/apparmor/apparmorfs.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 391a586d0557..4b2752200ce2 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -1602,16 +1602,20 @@ static char *gen_symlink_name(int depth, const char *dirname, const char *fname)
> {
> char *buffer, *s;
> int error;
> - int size = depth * 6 + strlen(dirname) + strlen(fname) + 11;
> + const char *path = "../../";
> + size_t path_len = strlen(path);
> + int size;
>
> + /* Extra 11 bytes: "raw_data" (9) + two slashes "//" (2) */
> + size = depth * path_len + strlen(dirname) + strlen(fname) + 11;
> s = buffer = kmalloc(size, GFP_KERNEL);
> if (!buffer)
> return ERR_PTR(-ENOMEM);
>
> for (; depth > 0; depth--) {
> - strcpy(s, "../../");
> - s += 6;
> - size -= 6;
> + memcpy(s, path, path_len);
> + s += path_len;
> + size -= path_len;
> }
>
> error = snprintf(s, size, "raw_data/%s/%s", dirname, fname);
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: Replace sprintf/strcpy with scnprintf/strscpy in aa_policy_init
From: John Johansen @ 2025-11-27 1:33 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20251122115549.448042-3-thorsten.blum@linux.dev>
On 11/22/25 03:55, Thorsten Blum wrote:
> strcpy() is deprecated and sprintf() does not perform bounds checking
> either. Although an overflow is unlikely, it's better to proactively
> avoid it by using the safer strscpy() and scnprintf(), respectively.
>
> Additionally, unify memory allocation for 'hname' to simplify and
> improve aa_policy_init().
>
> Link: https://github.com/KSPP/linux/issues/88
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
I have pulled this into my tree
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/lib.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
> index 82dbb97ad406..acf7f5189bec 100644
> --- a/security/apparmor/lib.c
> +++ b/security/apparmor/lib.c
> @@ -478,19 +478,17 @@ bool aa_policy_init(struct aa_policy *policy, const char *prefix,
> const char *name, gfp_t gfp)
> {
> char *hname;
> + size_t hname_sz;
>
> + hname_sz = (prefix ? strlen(prefix) + 2 : 0) + strlen(name) + 1;
> /* freed by policy_free */
> - if (prefix) {
> - hname = aa_str_alloc(strlen(prefix) + strlen(name) + 3, gfp);
> - if (hname)
> - sprintf(hname, "%s//%s", prefix, name);
> - } else {
> - hname = aa_str_alloc(strlen(name) + 1, gfp);
> - if (hname)
> - strcpy(hname, name);
> - }
> + hname = aa_str_alloc(hname_sz, gfp);
> if (!hname)
> return false;
> + if (prefix)
> + scnprintf(hname, hname_sz, "%s//%s", prefix, name);
> + else
> + strscpy(hname, name, hname_sz);
> policy->hname = hname;
> /* base.name is a substring of fqname */
> policy->name = basename(policy->hname);
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: replace sprintf with snprintf in aa_new_learning_profile
From: John Johansen @ 2025-11-27 1:34 UTC (permalink / raw)
To: Thorsten Blum, Paul Moore, James Morris, Serge E. Hallyn
Cc: apparmor, linux-security-module, linux-kernel
In-Reply-To: <20251122115446.447925-1-thorsten.blum@linux.dev>
On 11/22/25 03:54, Thorsten Blum wrote:
> Replace unbounded sprintf() calls with snprintf() to prevent potential
> buffer overflows in aa_new_learning_profile(). While the current code
> works correctly, snprintf() is safer and follows secure coding best
> practices. No functional changes.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
I have pulled this into my tree
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/policy.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 50d5345ff5cb..b09323867fea 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -697,24 +697,27 @@ struct aa_profile *aa_new_learning_profile(struct aa_profile *parent, bool hat,
> struct aa_profile *p, *profile;
> const char *bname;
> char *name = NULL;
> + size_t name_sz;
>
> AA_BUG(!parent);
>
> if (base) {
> - name = kmalloc(strlen(parent->base.hname) + 8 + strlen(base),
> - gfp);
> + name_sz = strlen(parent->base.hname) + 8 + strlen(base);
> + name = kmalloc(name_sz, gfp);
> if (name) {
> - sprintf(name, "%s//null-%s", parent->base.hname, base);
> + snprintf(name, name_sz, "%s//null-%s",
> + parent->base.hname, base);
> goto name;
> }
> /* fall through to try shorter uniq */
> }
>
> - name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, gfp);
> + name_sz = strlen(parent->base.hname) + 2 + 7 + 8;
> + name = kmalloc(name_sz, gfp);
> if (!name)
> return NULL;
> - sprintf(name, "%s//null-%x", parent->base.hname,
> - atomic_inc_return(&parent->ns->uniq_null));
> + snprintf(name, name_sz, "%s//null-%x", parent->base.hname,
> + atomic_inc_return(&parent->ns->uniq_null));
>
> name:
> /* lookup to see if this is a dup creation */
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-27 9:25 UTC (permalink / raw)
To: Helge Deller, John Johansen
Cc: david laight, Helge Deller, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <aSdfyGv2T88T5FEu@carbonx1>
Hi Helge,
On Wed, 2025-11-26 at 21:15 +0100, Helge Deller wrote:
> So, here is a (untested) v3:
>
>
> [PATCH v3] apparmor: Optimize table creation from possibly unaligned memory
>
> Source blob may come from userspace and might be unaligned.
> Try to optize the copying process by avoiding unaligned memory accesses.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..19e72b3e8f49 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,18 @@ struct aa_dfa {
> struct table_header *tables[YYTD_ID_TSIZE];
> };
>
> -#define byte_to_byte(X) (X)
> -
> #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
> do { \
> typeof(LEN) __i; \
> TTYPE *__t = (TTYPE *) TABLE; \
> BTYPE *__b = (BTYPE *) BLOB; \
> - for (__i = 0; __i < LEN; __i++) { \
> - __t[__i] = NTOHX(__b[__i]); \
> - } \
> + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
> + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> + else /* copy & convert convert from big-endian */ \
> + for (__i = 0; __i < LEN; __i++) { \
> + __t[__i] = NTOHX(&__b[__i]); \
> + } \
> } while (0)
>
> static inline size_t table_size(size_t len, size_t el_size)
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index c5a91600842a..1e32c8ba14ae 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -15,6 +15,7 @@
> #include <linux/vmalloc.h>
> #include <linux/err.h>
> #include <linux/kref.h>
> +#include <linux/unaligned.h>
>
> #include "include/lib.h"
> #include "include/match.h"
> @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> table->td_flags = th.td_flags;
> table->td_lolen = th.td_lolen;
> if (th.td_flags == YYTD_DATA8)
> - UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> - u8, u8, byte_to_byte);
> + memcpy(table->td_data, blob, th.td_lolen);
> else if (th.td_flags == YYTD_DATA16)
> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> - u16, __be16, be16_to_cpu);
> + u16, __be16, get_unaligned_be16);
> else if (th.td_flags == YYTD_DATA32)
> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> - u32, __be32, be32_to_cpu);
> + u32, __be32, get_unaligned_be32);
> else
> goto fail;
> /* if table was vmalloced make sure the page tables are synced
This one does not apply:
glaubitz@node54:/data/home/glaubitz/linux> git am ../20251125_app_armor_unalign_2nd.mbx
Applying: apparmor unaligned memory fixes
error: patch failed: security/apparmor/match.c:15
error: security/apparmor/match.c: patch does not apply
Patch failed at 0001 apparmor unaligned memory fixes
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
glaubitz@node54:/data/home/glaubitz/linux>
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 9:32 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jann Horn, Konstantin Meskhidze, Tingmao Wang, Paul Moore,
linux-security-module
In-Reply-To: <20251017.ohthoos9Ogha@digikod.net>
Hello!
Thank you for the review! It has taken a while, but now V3 is almost
ready and I have the answers to these questions.
On Fri, Oct 17, 2025 at 05:04:23PM +0200, Mickaël Salaün wrote:
> On Wed, Oct 01, 2025 at 01:18:06PM +0200, Günther Noack wrote:
> > Introduce the LANDLOCK_RESTRICT_SELF_TSYNC flag. With this flag, a
> > given Landlock ruleset is applied to all threads of the calling
> > process, instead of only the current one.
> >
> > Without this flag, multithreaded userspace programs currently resort
> > to using the nptl(7)/libpsx hack for multithreaded policy enforcement,
> > which is also used by libcap and for setuid(2). Using this scheme,
> > the threads of a process enforce the same Landlock ruleset, but the
> > resulting Landlock domains are still separate, which makes a
> > difference for Landlock's "scoped" access rights, where the domain
> > identity and nesting is used. As a result, when using
> > LANLDOCK_SCOPE_SIGNAL, signaling between sibling threads stops
> > working. This is a problem for programming languages and frameworks
> > which are inherently multithreaded (e.g. Go).
>
> Having different Landlock domains also means that we get different
> domains ID, and the audit logs might confuse users.
True, I added that to the commit message.
> > Cc: Mickaël Salaün <mic@digikod.net>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: linux-security-module@vger.kernel.org
> > Suggested-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > ---
> > include/uapi/linux/landlock.h | 4 +
> > security/landlock/cred.h | 12 +
> > security/landlock/limits.h | 2 +-
> > security/landlock/syscalls.c | 433 +++++++++++++++++++++++++++++++++-
>
> You can move most of this code to a new tsync.c file.
Done.
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -117,11 +117,15 @@ struct landlock_ruleset_attr {
> > * future nested domains, not the one being created. It can also be used
> > * with a @ruleset_fd value of -1 to mute subdomain logs without creating a
> > * domain.
> > + *
> > + * %LANDLOCK_RESTRICT_SELF_TSYNC
> > + * Apply the given ruleset atomically to all threads of the current process.
>
> Please bump the Landlock ABI version and update the doc.
Done.
Docs: I added flag description to the landlock.h header so that it
appears in the documentation for the system call, and added an entry
to the section where we describe the differences between the ABI
versions.
> > diff --git a/security/landlock/cred.h b/security/landlock/cred.h
> > index c82fe63ec598..eb28eeade760 100644
> > --- a/security/landlock/cred.h
> > +++ b/security/landlock/cred.h
> > @@ -65,6 +65,18 @@ landlock_cred(const struct cred *cred)
> > return cred->security + landlock_blob_sizes.lbs_cred;
> > }
> >
> > +static inline void landlock_cred_copy(struct landlock_cred_security *dst,
> > + const struct landlock_cred_security *src)
> > +{
>
> You can simplify by removing the domain checks which are already done by
> landlock_put/get_ruleset().
>
> > + if (dst->domain)
> > + landlock_put_ruleset(dst->domain);
> > +
> > + *dst = *src;
> > +
> > + if (dst->domain)
> > + landlock_get_ruleset(src->domain);
Done.
> > +/*
> > + * restrict_one_thread - update a thread's Landlock domain in lockstep with the
> > + * other threads in the same process
> > + *
> > + * When this is run, the same function gets run in all other threads in the same
> > + * process (except for the calling thread which called landlock_restrict_self).
> > + * The concurrently running invocations of restrict_one_thread coordinate
> > + * through the shared ctx object to do their work in lockstep to implement
> > + * all-or-nothing semantics for enforcing the new Landlock domain.
> > + *
> > + * Afterwards, depending on the presence of an error, all threads either commit
> > + * or abort the prepared credentials. The commit operation can not fail any more.
> > + */
> > +static void restrict_one_thread(struct tsync_shared_context *ctx)
> > +{
> > + int res;
>
> For consistency with existing code, please use "err" instead of "res".
Done.
> > + struct cred *cred = NULL;
> > + const struct cred *current_cred = current_cred();
>
> No need for this variable.
Done.
> > +
> > + if (current_cred == ctx->old_cred) {
> > + /*
> > + * As a shortcut, switch out old_cred with new_cred, if
> > + * possible.
> > + *
> > + * Note: We are intentionally dropping the const qualifier here,
> > + * because it is required by commit_creds() and abort_creds().
> > + */
> > + cred = (struct cred *)get_cred(ctx->new_cred);
>
> Good. You can extend the comment to explain that this optimization
> avoid creating new credentials in most cases, and then save memory.
Sounds good, I extended that comment a bit.
> > + } else {
> > + /* Else, prepare new creds and populate them. */
> > + cred = prepare_creds();
> > +
> > + if (!cred) {
> > + atomic_set(&ctx->preparation_error, -ENOMEM);
> > +
> > + /*
> > + * Even on error, we need to adhere to the protocol and
> > + * coordinate with concurrently running invocations.
> > + */
> > + if (atomic_dec_return(&ctx->num_preparing) == 0)
> > + complete_all(&ctx->all_prepared);
> > +
> > + goto out;
> > + }
> > +
> > + landlock_cred_copy(landlock_cred(cred),
> > + landlock_cred(ctx->new_cred));
> > + }
> > +
> > + /*
> > + * Barrier: Wait until all threads are done preparing.
> > + * After this point, we can have no more failures.
> > + */
> > + if (atomic_dec_return(&ctx->num_preparing) == 0)
> > + complete_all(&ctx->all_prepared);
> > +
> > + /*
> > + * Wait for signal from calling thread that it's safe to read the
> > + * preparation error now and we are ready to commit (or abort).
> > + */
> > + wait_for_completion(&ctx->ready_to_commit);
> > +
> > + /* Abort the commit if any of the other threads had an error. */
> > + res = atomic_read(&ctx->preparation_error);
> > + if (res) {
> > + abort_creds(cred);
> > + goto out;
> > + }
> > +
> > + /* If needed, establish enforcement prerequisites. */
> > + if (!ns_capable_noaudit(current_user_ns(), CAP_SYS_ADMIN))
> > + task_set_no_new_privs(current);
>
> We should always set PR_SET_NO_NEW_PRIVS if it is set on the calling
> thread as done by seccomp. We should just store the result of
> task_no_new_privs() in tsync_shared_context and use it as condition here.
> This should be explained in the documentation.
>
> This also mean that if the calling thread has CAP_SYS_ADMIN but not
> PR_SET_NO_NEW_PRIVS, then a sibling thread could not have CAP_SYS_ADMIN
> nor PR_SET_NO_NEW_PRIVS. This would be a risky state but mainly because
> of the CAP_SYS_ADMIN inconsistency, not really the missing
> PR_SET_NO_NEW_PRIVS.
OK, done. In line with seccomp now, where the logic is (pseudocode):
if (task_no_new_privs(caller))
task_set_no_new_privs(current);
As you are saying as well, the situation where the caller has
CAP_SYS_ADMIN but not PR_SET_NO_NEW_PRIVS results in sibling threads
that will also end up without PR_SET_NO_NEW_PRIVS. But this is also
consistent with Seccomp.
> > +
> > + commit_creds(cred);
> > +
> > +out:
> > + /* Notify the calling thread once all threads are done */
> > + if (atomic_dec_return(&ctx->num_unfinished) == 0)
> > + complete_all(&ctx->all_finished);
> > +}
> > +
> > +/*
> > + * restrict_one_thread_callback - task_work callback for restricting a thread
> > + *
> > + * Calls restrict_one_thread with the struct landlock_shared_tsync_context.
> > + */
> > +static void restrict_one_thread_callback(struct callback_head *work)
> > +{
> > + struct tsync_work *ctx = container_of(work, struct tsync_work, work);
> > +
> > + restrict_one_thread(ctx->shared_ctx);
> > +}
> > +
> > +/*
> > + * struct tsync_works - a growable array of per-task contexts
> > + *
> > + * The zero-initialized struct represents the empty array.
> > + */
> > +struct tsync_works {
> > + struct tsync_work **works;
> > + size_t size;
> > + size_t capacity;
> > +};
> > +
> > +/*
> > + * tsync_works_provide - provides a preallocated tsync_work for the given task
> > + *
> > + * This also stores a task pointer in the context and increments the reference
> > + * count of the task.
> > + *
> > + * Returns:
> > + * A pointer to the preallocated context struct, with task filled in.
> > + *
> > + * NULL, if we ran out of preallocated context structs.
> > + */
> > +static struct tsync_work *tsync_works_provide(struct tsync_works *s,
> > + struct task_struct *task)
> > +{
> > + struct tsync_work *ctx;
> > +
> > + if (s->size >= s->capacity)
>
> In which case can this happen? Should we wrap this in a WARN_ON_ONCE()?
As Jann also said in his reply, it can happen if the number of
existing threads differs between the first for_each_thread() loop
where we count the threads and the second for_each_thread() loop where
we fill the allocated slots in the array. If it happens, we deal with
it by doing an additional loop to discover new threads.
> > + return NULL;
> > +
> > + ctx = s->works[s->size];
> > + s->size++;
> > +
> > + ctx->task = get_task_struct(task);
> > + return ctx;
> > +}
> > +
> > +/*
> > + * tsync_works_grow_by - preallocates space for n more contexts in s
> > + *
> > + * Returns:
> > + * -ENOMEM if the (re)allocation fails
> > + * 0 if the allocation succeeds, partially succeeds, or no reallocation was needed
> > + */
> > +static int tsync_works_grow_by(struct tsync_works *s, size_t n, gfp_t flags)
> > +{
> > + int i;
>
> size_t i;
Done.
> > + size_t new_capacity = s->capacity + n;
>
> > + struct tsync_work **works;
> > +
> > + if (new_capacity <= s->capacity)
>
> This integer overflow check should return an error instead.
Oh, this was not meant to check overflow, actually.
As Jann is remarking in [1], what should really have happened here is
that the capacity should have been increased to s->size + n instead of
s->capacity + n a few lines above. With that logic, it also makes
more sense to check whether the new capacity is below-or-equal the old
capacity, as it can legitimately happen in cases where the previously
allocated size was not used up in its entirety. In that case, it is
possible that we already have sufficient space for the additional n
items, and it is reasonable to return 0 in that case. (The semantics
of the function are "make sure that I have enough space for n
additional items", and that is fulfilled without error.)
[1] http://lore.kernel.org/all/CAG48ez1oS9kANZBq1bt+D76MX03DPHAFp76GJt7z5yx-Na1VLQ@mail.gmail.com
I did add an overflow check as well, for good measure (it's
practically for free).
>
> > + return 0;
> > +
> > + works = krealloc_array(s->works, new_capacity, sizeof(s->works[0]),
> > + flags);
> > + if (IS_ERR(works))
> > + return PTR_ERR(works);
> > +
> > + s->works = works;
> > +
> > + for (i = s->capacity; i < new_capacity; i++) {
> > + s->works[i] = kzalloc(sizeof(*s->works[i]), flags);
>
> We should use a local variable to avoid storing an error code in
> s->works[i] and potentially dereferencing it later (e.g. in
> tsync_work_free).
As Jann also pointed out in the other mail, this was a mistake -
kzalloc returns NULL on failure, not an error code. (Fixed)
I extracted a local variable anyway, it's a bit clearer to read.
> Why can't we avoid this loop entirely and allocate a flat array with
> only one call to krealloc_array()? Why struct tsync_works->works needs
> to be a pointer to a pointer?
We pass out pointers to the struct tsync_work elements when calling
task_work_add(), and these pointers are used by the task_work logic
and by our own implementation of that task_work,
restrict_one_thread_callback(). If these elements were directly in
the array, realloc would move them to new locations and these pointers
would then point into freed memory.
Discussed by Jann in [2] as well.
[2] https://lore.kernel.org/all/CAG48ez2KoF6hVSJwdPfUpN3oroMww6Mu1+-bsBSbO8C5f91P6Q@mail.gmail.com/
If we were to take a very close look at all the affected code, we
could potentially convince ourselves that we can discard these objects
after the all_prepared point, but this all seems like a brittle coding
pattern that would go against normal conventions and could break in a
future version. (e.g. when task_work_run() would invoke
"work->func(work)", the memory behind that "work" pointer would get
freed halfway throughout that invocation. It seems a bit brittle.)
I'd prefer to not base our code correctness on such complicated
assumptions about other corners of the code.
> > + if (IS_ERR(s->works[i])) {
> > + /*
> > + * Leave the object in a consistent state,
> > + * but return an error.
> > + */
> > + s->capacity = i;
> > + return PTR_ERR(s->works[i]);
> > + }
> > + }
> > + s->capacity = new_capacity;
> > + return 0;
> > +}
> > +
> > +/*
> > + * tsync_works_contains - checks for presence of task in s
> > + */
> > +static bool tsync_works_contains_task(struct tsync_works *s,
> > + struct task_struct *task)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < s->size; i++)
> > + if (s->works[i]->task == task)
> > + return true;
> > + return false;
> > +}
> > +
> > +/*
> > + * tsync_works_free - free memory held by s and drop all task references
> > + */
> > +static void tsync_works_free(struct tsync_works *s)
>
> tsync_works_put() would be more appropriate since this function doesn't
> free s.
put() is usually a reference count decrement, and we don't do that either.
After a bit of thinking, I settled on tsync_works_release(), which is
a slightly more generic wording that does not sound like a refcount.
> > @@ -566,5 +987,13 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
> > new_llcred->domain_exec |= BIT(new_dom->num_layers - 1);
> > #endif /* CONFIG_AUDIT */
> >
> > + if (flags & LANDLOCK_RESTRICT_SELF_TSYNC) {
> > + res = restrict_sibling_threads(current_cred(), new_cred);
> > + if (res != 0) {
>
> if (!err) {
Done.
(Corrected it in another place in landlock_restrict_sibling_threads() as well)
—Günther
^ permalink raw reply
* Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 9:34 UTC (permalink / raw)
To: Jann Horn
Cc: Mickaël Salaün, Konstantin Meskhidze, Tingmao Wang,
Paul Moore, linux-security-module
In-Reply-To: <CAG48ez2KoF6hVSJwdPfUpN3oroMww6Mu1+-bsBSbO8C5f91P6Q@mail.gmail.com>
Thank you for the thorough review! You are correct about all three
points. I replied in the response to the mail one above to have the
conversation in one place.
On Fri, Oct 24, 2025 at 11:18:39PM +0200, Jann Horn wrote:
> [...]
—Günther
^ permalink raw reply
* Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 9:36 UTC (permalink / raw)
To: Jann Horn
Cc: Mickaël Salaün, Konstantin Meskhidze, Tingmao Wang,
Paul Moore, linux-security-module
In-Reply-To: <CAG48ez3MxN524ge_sZeTwL0FEDASaSTb-gm1vPO8UwpijTeHqw@mail.gmail.com>
On Fri, Oct 24, 2025 at 11:29:51PM +0200, Jann Horn wrote:
> On Mon, Oct 20, 2025 at 10:12 PM Mickaël Salaün <mic@digikod.net> wrote:
> > Is it safe to prevent inconsistencies wrt execve? seccomp uses
> > cred_guard_mutex (new code should probably use exec_update_lock), why
> > should Landlock not do the same?
>
> We don't have to worry about interactions with execve because, unlike
> seccomp, we don't directly change properties of another running
> thread; we ask other threads to change their credentials _themselves_.
> From a locking context, restrict_one_thread() essentially runs in the
> same kind of context as a syscall, and doesn't need any more locking
> than the existing landlock_restrict_self().
>
> > Why shouldn't we lock sighand as well?
>
> seccomp uses siglock for the following reasons:
>
> 1. to protect against concurrent access to one thread's seccomp filter
> information from multiple threads; we don't do anything like that
> 2. to protect the for_each_thread() loop; we use RCU for that (we
> could also use siglock but there's no reason to do that, and RCU is
> more lightweight than the siglock which requires disabling interrupts)
> 3. to ensure that threads' seccomp states don't change between its two
> loops over other threads (seccomp_can_sync_threads() and
> seccomp_sync_threads()); we don't do anything like that
Thanks for digging this up! I used a (reworded) variant of these three
points to document the locking rationale in the code.
—Günther
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-27 9:43 UTC (permalink / raw)
To: Helge Deller, John Johansen
Cc: david laight, Helge Deller, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <6d80f9bc5fd6d91ed2451d140227b866d6273af4.camel@physik.fu-berlin.de>
Hi Helge,
On Thu, 2025-11-27 at 10:25 +0100, John Paul Adrian Glaubitz wrote:
> Hi Helge,
>
> On Wed, 2025-11-26 at 21:15 +0100, Helge Deller wrote:
> > So, here is a (untested) v3:
> >
> >
> > [PATCH v3] apparmor: Optimize table creation from possibly unaligned memory
> >
> > Source blob may come from userspace and might be unaligned.
> > Try to optize the copying process by avoiding unaligned memory accesses.
> >
> > Signed-off-by: Helge Deller <deller@gmx.de>
> >
> > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > index 1fbe82f5021b..19e72b3e8f49 100644
> > --- a/security/apparmor/include/match.h
> > +++ b/security/apparmor/include/match.h
> > @@ -104,16 +104,18 @@ struct aa_dfa {
> > struct table_header *tables[YYTD_ID_TSIZE];
> > };
> >
> > -#define byte_to_byte(X) (X)
> > -
> > #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
> > do { \
> > typeof(LEN) __i; \
> > TTYPE *__t = (TTYPE *) TABLE; \
> > BTYPE *__b = (BTYPE *) BLOB; \
> > - for (__i = 0; __i < LEN; __i++) { \
> > - __t[__i] = NTOHX(__b[__i]); \
> > - } \
> > + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
> > + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> > + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> > + else /* copy & convert convert from big-endian */ \
> > + for (__i = 0; __i < LEN; __i++) { \
> > + __t[__i] = NTOHX(&__b[__i]); \
> > + } \
> > } while (0)
> >
> > static inline size_t table_size(size_t len, size_t el_size)
> > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > index c5a91600842a..1e32c8ba14ae 100644
> > --- a/security/apparmor/match.c
> > +++ b/security/apparmor/match.c
> > @@ -15,6 +15,7 @@
> > #include <linux/vmalloc.h>
> > #include <linux/err.h>
> > #include <linux/kref.h>
> > +#include <linux/unaligned.h>
> >
> > #include "include/lib.h"
> > #include "include/match.h"
> > @@ -66,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > table->td_flags = th.td_flags;
> > table->td_lolen = th.td_lolen;
> > if (th.td_flags == YYTD_DATA8)
> > - UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> > - u8, u8, byte_to_byte);
> > + memcpy(table->td_data, blob, th.td_lolen);
> > else if (th.td_flags == YYTD_DATA16)
> > UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> > - u16, __be16, be16_to_cpu);
> > + u16, __be16, get_unaligned_be16);
> > else if (th.td_flags == YYTD_DATA32)
> > UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> > - u32, __be32, be32_to_cpu);
> > + u32, __be32, get_unaligned_be32);
> > else
> > goto fail;
> > /* if table was vmalloced make sure the page tables are synced
>
> This one does not apply:
>
> glaubitz@node54:/data/home/glaubitz/linux> git am ../20251125_app_armor_unalign_2nd.mbx
> Applying: apparmor unaligned memory fixes
> error: patch failed: security/apparmor/match.c:15
> error: security/apparmor/match.c: patch does not apply
> Patch failed at 0001 apparmor unaligned memory fixes
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> hint: When you have resolved this problem, run "git am --continue".
> hint: If you prefer to skip this patch, run "git am --skip" instead.
> hint: To restore the original branch and stop patching, run "git am --abort".
> hint: Disable this message with "git config set advice.mergeConflict false"
> glaubitz@node54:/data/home/glaubitz/linux>
The patch alone applies, i.e without your previous patch, but it does not fix the problem:
[ 73.961582] Kernel unaligned access at TPC[8dabdc] aa_dfa_unpack+0x3c/0x6e0
[ 74.053195] Kernel unaligned access at TPC[8dabec] aa_dfa_unpack+0x4c/0x6e0
[ 74.144814] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[ 74.237538] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
[ 74.330296] Kernel unaligned access at TPC[8dacd0] aa_dfa_unpack+0x130/0x6e0
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Roberto Sassu @ 2025-11-27 9:45 UTC (permalink / raw)
To: Gregory Lumen
Cc: Anirudh Venkataramanan, linux-integrity, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Lakshmi Ramasubramanian, Sush Shringarputale
In-Reply-To: <bbafa611-3a6c-5cf8-631c-20f72f651d9@linux.microsoft.com>
On Wed, 2025-11-26 at 15:40 -0800, Gregory Lumen wrote:
> Greetings Roberto,
>
> If I may chime in a bit:
>
> > The only way to make the verification of measurements list snapshots
> > work is that the verification state is stored outside the system to
> > evaluate (which can be assumed to be trusted), so that you are sure
> > that the system is not advancing the PCR starting value by itself.
>
> You are correct; to make the described approach work, an external source
> of trust is required in order to detect unexpected or unauthorized
> trimming of the event log (for example, by signing the trim-to PCR values
> from the previous verification/attestation cycle). This should be true
> regardless of the mechanism of trimming. More generally, I will go so far
> as to suggest that any attempt to attest the integrity of a system using
> IMA will likely fall into one of two general approaches: either the entire
> IMA event log is retained (either in kernel or user space) from boot and
> claims of system integrity are built by validating and examining the
> entire log for signs of tampering, or an external source of trust is
> introduced to allow incremental validation and examination of the log.
> Other more innovative approaches may exist, but we make no such claims.
>
> I will also say that it should be possible to implement either approach to
> attestation (retaining the entire log, or relying on an external source of
> trust) with any sane implementation for IMA log trimming.
>
> As for our proposed implementation, storing the starting PCR values in the
> kernel preserving the ability for any arbitrary user space entity to
> validate the retained portion of the IMA event log against the TPM PCRs at
> any time, without requiring awareness of other user space mechanisms
> implemented by other entities that may be initiating IMA trimming
> operations. My personal sense is that this capability is worth preserving,
> but it is entirely possible the general consensus is that the value
> offered does not balance against the additional technical complexity when
> compared to simpler alternatives (discussed in a moment). To stress the
> point, this capability would only enable validation of the integrity of
> the retained portion of the event log and its continuity with the PCRs,
> and could not be used to make any claims as to the overall integrity of
> the system since, as you observed, an attacker who has successfully
> compromised the system could simply trim the event log in order to discard
> evidence of the compromise.
Hi Gregory
all you said can be implemented by maintaining the PCR starting value
outside the system, in a trusted entity. This would allow the
functionality you are hoping for to validate the retained portion of
the measurement list.
Keeping the PCR starting value in the kernel has the potential of
misleading users that this is an information they can rely on. I would
rather prefer to not run in such risk.
> If the ability to validate the retained portion of the IMA event log is
> not worth designing for, we could instead go with a simpler "Trim-to-N"
> approach, where the user space interface allows for the specification of
> an absolute index into the IMA log to be used as the trim position (as
> opposed to using calculated PCR values to indicate trim position in our
> current proposal). To protect against unexpected behavior in the event of
From implementation point of view, it looks much simpler to me to
specify N relative to the current measurement list.
> concurrent trims, index counting would need to be fixed (hence absolute)
> such that index 0 would always refer to the very first entry written
> during boot, even if that entry has already been trimmed, with the number
> of trimmed entries (and thus starting index of the retained log) exposed
> to use space via a pseudo-file.
In my draft patch [1] (still need to support trimming N entries instead
of the full measurement list), the risk of concurrent trims does not
exist because opening of the snapshot interface is exclusive (no one
else can request trimming concurrently).
If a more elaborated contention of remote attestation agent is
required, that could be done at user space level. I'm hoping to keep in
the kernel only the minimum code necessary for the remote attestation
to work.
Roberto
[1] https://github.com/robertosassu/linux/commit/b0bd002b6caa9d5d4f4d0db2a041b1fd91f33f8a
> With such a trim approach, it should be possible to implement either
> general attestation approach: retaining the entire log (copy the log to
> user space, then trim the copied entries), or relying on an external
> source of trust (quote, determine the log index corresponding to the quote
> plus PCRs, trim, then securely store the trim position/starting PCRs for
> future cycles).
>
> -Gregory Lumen
^ permalink raw reply
* Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 9:56 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Konstantin Meskhidze, Tingmao Wang, Paul Moore,
linux-security-module, Jann Horn
In-Reply-To: <20251020.fohbo6Iecahz@digikod.net>
Hello!
On Mon, Oct 20, 2025 at 10:12:44PM +0200, Mickaël Salaün wrote:
> On Wed, Oct 01, 2025 at 01:18:06PM +0200, Günther Noack wrote:
> > +/*
> > + * restrict_sibling_threads - enables a Landlock policy for all sibling threads
> > + */
> > +static int restrict_sibling_threads(const struct cred *old_cred,
> > + const struct cred *new_cred)
> > +{
> > + int res;
> > + struct task_struct *thread, *caller;
> > + struct tsync_shared_context shared_ctx;
> > + struct tsync_works works = {};
> > + size_t newly_discovered_threads;
> > + bool found_more_threads;
> > + struct tsync_work *ctx;
> > +
> > + atomic_set(&shared_ctx.preparation_error, 0);
> > + init_completion(&shared_ctx.all_prepared);
> > + init_completion(&shared_ctx.ready_to_commit);
> > + atomic_set(&shared_ctx.num_unfinished, 0);
> > + init_completion(&shared_ctx.all_finished);
> > + shared_ctx.old_cred = old_cred;
> > + shared_ctx.new_cred = new_cred;
> > +
> > + caller = current;
> > +
> > + /*
> > + * We schedule a pseudo-signal task_work for each of the calling task's
> > + * sibling threads. In the task work, each thread:
> > + *
> > + * 1) runs prepare_creds() and writes back the error to
> > + * shared_ctx.preparation_error, if needed.
> > + *
> > + * 2) signals that it's done with prepare_creds() to the calling task.
> > + * (completion "all_prepared").
> > + *
> > + * 3) waits for the completion "ready_to_commit". This is sent by the
> > + * calling task after ensuring that all sibling threads have done
> > + * with the "preparation" stage.
> > + *
> > + * After this barrier is reached, it's safe to read
> > + * shared_ctx.preparation_error.
> > + *
> > + * 4) reads shared_ctx.preparation_error and then either does
> > + * commit_creds() or abort_creds().
> > + *
> > + * 5) signals that it's done altogether (barrier synchronization
> > + * "all_finished")
> > + */
> > + do {
> > + found_more_threads = false;
> > +
> > + /*
> > + * The "all_prepared" barrier is used locally to the inner loop,
> > + * this use of for_each_thread(). We can reset it on each loop
> > + * iteration because all previous loop iterations are done with
> > + * it already.
> > + *
> > + * num_preparing is initialized to 1 so that the counter can not
> > + * go to 0 and mark the completion as done before all task works
> > + * are registered. (We decrement it at the end of this loop.)
> > + */
> > + atomic_set(&shared_ctx.num_preparing, 1);
> > + reinit_completion(&shared_ctx.all_prepared);
> > +
>
> > + /* In RCU read-lock, count the threads we need. */
> > + newly_discovered_threads = 0;
> > + rcu_read_lock();
> > + for_each_thread(caller, thread) {
> > + /* Skip current, since it is initiating the sync. */
> > + if (thread == caller)
> > + continue;
> > +
> > + /* Skip exited threads. */
> > + if (thread->flags & PF_EXITING)
> > + continue;
> > +
> > + /* Skip threads that we have already seen. */
> > + if (tsync_works_contains_task(&works, thread))
> > + continue;
> > +
> > + newly_discovered_threads++;
> > + }
> > + rcu_read_unlock();
>
> This RCU block could be moved in a dedicated helper that will return the
> number of newly discovered threads. In this helper, we could use
> guard()(rcu).
Done.
> > +
> > + if (newly_discovered_threads == 0)
> > + break; /* done */
> > +
> > + res = tsync_works_grow_by(&works, newly_discovered_threads,
> > + GFP_KERNEL_ACCOUNT);
> > + if (res) {
> > + atomic_set(&shared_ctx.preparation_error, res);
> > + break;
> > + }
> > +
> > + rcu_read_lock();
> > + for_each_thread(caller, thread) {
> > + /* Skip current, since it is initiating the sync. */
> > + if (thread == caller)
> > + continue;
> > +
> > + /* Skip exited threads. */
> > + if (thread->flags & PF_EXITING)
> > + continue;
> > +
> > + /* Skip threads that we already looked at. */
> > + if (tsync_works_contains_task(&works, thread))
> > + continue;
> > +
> > + /*
> > + * We found a sibling thread that is not doing its
> > + * task_work yet, and which might spawn new threads
> > + * before our task work runs, so we need at least one
> > + * more round in the outer loop.
> > + */
> > + found_more_threads = true;
> > +
> > + ctx = tsync_works_provide(&works, thread);
> > + if (!ctx) {
> > + /*
> > + * We ran out of preallocated contexts -- we
> > + * need to try again with this thread at a later
> > + * time! found_more_threads is already true
> > + * at this point.
> > + */
> > + break;
> > + }
> > +
> > + ctx->shared_ctx = &shared_ctx;
> > +
> > + atomic_inc(&shared_ctx.num_preparing);
> > + atomic_inc(&shared_ctx.num_unfinished);
> > +
> > + init_task_work(&ctx->work,
> > + restrict_one_thread_callback);
> > + res = task_work_add(thread, &ctx->work, TWA_SIGNAL);
> > + if (res) {
> > + /*
> > + * Remove the task from ctx so that we will
> > + * revisit the task at a later stage, if it
> > + * still exists.
> > + */
> > + put_task_struct_rcu_user(ctx->task);
> > + ctx->task = NULL;
> > +
> > + atomic_set(&shared_ctx.preparation_error, res);
> > + atomic_dec(&shared_ctx.num_preparing);
> > + atomic_dec(&shared_ctx.num_unfinished);
> > + }
> > + }
> > + rcu_read_unlock();
>
> As for the other RCU block, it might help to move this RCU block into a
> dedicated helper. It seems that it would look easier to read and
> maintain.
Done.
> > + /*
> > + * Decrement num_preparing for current, to undo that we
> > + * initialized it to 1 at the beginning of the inner loop.
> > + */
> > + if (atomic_dec_return(&shared_ctx.num_preparing) > 0)
> > + wait_for_completion(&shared_ctx.all_prepared);
> > + } while (found_more_threads &&
> > + !atomic_read(&shared_ctx.preparation_error));
>
> Is it safe to prevent inconsistencies wrt execve? seccomp uses
> cred_guard_mutex (new code should probably use exec_update_lock), why
> should Landlock not do the same?
>
> Why shouldn't we lock sighand as well?
>
> Answers to these questions should be explained in comments.
Added something to the top of the loop, based on Jann's explanation in [1].
[1] https://lore.kernel.org/all/CAG48ez3MxN524ge_sZeTwL0FEDASaSTb-gm1vPO8UwpijTeHqw@mail.gmail.com/
—Günther
^ permalink raw reply
* Re: [PATCH RESEND] apparmor: Replace deprecated strcpy with memcpy in gen_symlink_name
From: Thorsten Blum @ 2025-11-27 10:18 UTC (permalink / raw)
To: John Johansen
Cc: Paul Moore, James Morris, Serge E. Hallyn, apparmor,
linux-security-module, linux-kernel
In-Reply-To: <1da23c89-dc2c-41cb-8260-098deb8ae917@canonical.com>
On 27. Nov 2025, at 02:32, John Johansen wrote:
> hey Thorsten,
>
> sorry I have actually pulled these in, and tested them. I didn't send out
> the acks yet because I have another patch that I was waiting on a proper
> signed-off-by: on.
>
> I am going to have to pull that one so I can push. I'll add acks now but
> the push isn't going to happen for a few hours.
>
> Acked-by: John Johansen <john.johansen@canonical.com>
Ah sorry for resending then, didn't know you looked at them already.
Thanks,
Thorsten
^ permalink raw reply
* [PATCH] net: ipv6: fix spelling typos in comments
From: Shi Hao @ 2025-11-27 10:31 UTC (permalink / raw)
To: kuba
Cc: davem, pabeni, dsahern, edumazet, horms, netdev,
linux-security-module, linux-kernel, steffen.klassert, herbert,
i.shihao.999, pablo
Correct misspelled typos in comments
- informations -> information
- wont -> won't
- upto -> up to
- destionation -> destination
Signed-off-by: Shi Hao <i.shihao.999@gmail.com>
---
net/ipv6/ah6.c | 2 +-
net/ipv6/calipso.c | 4 ++--
net/ipv6/ip6_fib.c | 2 +-
net/ipv6/ip6_vti.c | 2 +-
net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +-
net/ipv6/reassembly.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 95372e0f1d21..92e1cf90a6be 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -169,7 +169,7 @@ static bool zero_out_mutable_opts(struct ipv6_opt_hdr *opthdr)
/**
* ipv6_rearrange_destopt - rearrange IPv6 destination options header
* @iph: IPv6 header
- * @destopt: destionation options header
+ * @destopt: destination options header
*/
static void ipv6_rearrange_destopt(struct ipv6hdr *iph, struct ipv6_opt_hdr *destopt)
{
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index df1986973430..220dd432c5bd 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -43,7 +43,7 @@
#define CALIPSO_HDR_LEN (2 + 8)
/* Maximum size of the calipso option including
- * the two-byte TLV header and upto 3 bytes of
+ * the two-byte TLV header and up to 3 bytes of
* leading pad and 7 bytes of trailing pad.
*/
#define CALIPSO_OPT_LEN_MAX_WITH_PAD (3 + CALIPSO_OPT_LEN_MAX + 7)
@@ -713,7 +713,7 @@ static int calipso_pad_write(unsigned char *buf, unsigned int offset,
*
* Description:
* Generate a CALIPSO option using the DOI definition and security attributes
- * passed to the function. This also generates upto three bytes of leading
+ * passed to the function. This also generates up to three bytes of leading
* padding that ensures that the option is 4n + 2 aligned. It returns the
* number of bytes written (including any initial padding).
*/
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 02c16909f618..19eefd48e744 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1010,7 +1010,7 @@ static int fib6_nh_drop_pcpu_from(struct fib6_nh *nh, void *_arg)
static void fib6_drop_pcpu_from(struct fib6_info *f6i)
{
- /* Make sure rt6_make_pcpu_route() wont add other percpu routes
+ /* Make sure rt6_make_pcpu_route() won't add other percpu routes
* while we are cleaning them here.
*/
f6i->fib6_destroying = 1;
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index ad5290be4dd6..cc8d0b142224 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -435,7 +435,7 @@ static bool vti6_state_check(const struct xfrm_state *x,
* vti6_xmit - send a packet
* @skb: the outgoing socket buffer
* @dev: the outgoing tunnel device
- * @fl: the flow informations for the xfrm_lookup
+ * @fl: the flow information for the xfrm_lookup
**/
static int
vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 64ab23ff559b..fcb17308c7e7 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -251,7 +251,7 @@ static int nf_ct_frag6_queue(struct frag_queue *fq, struct sk_buff *skb,
/* Note : skb->rbnode and skb->dev share the same location. */
dev = skb->dev;
- /* Makes sure compiler wont do silly aliasing games */
+ /* Makes sure compiler won't do silly aliasing games */
barrier();
prev = fq->q.fragments_tail;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 25ec8001898d..13540779a2c7 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -187,7 +187,7 @@ static int ip6_frag_queue(struct net *net,
/* Note : skb->rbnode and skb->dev share the same location. */
dev = skb->dev;
- /* Makes sure compiler wont do silly aliasing games */
+ /* Makes sure compiler won't do silly aliasing games */
barrier();
prev_tail = fq->q.fragments_tail;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 10:32 UTC (permalink / raw)
To: Jann Horn
Cc: Mickaël Salaün, Konstantin Meskhidze, Tingmao Wang,
Paul Moore, linux-security-module
In-Reply-To: <CAG48ez1oS9kANZBq1bt+D76MX03DPHAFp76GJt7z5yx-Na1VLQ@mail.gmail.com>
On Fri, Oct 24, 2025 at 11:11:10PM +0200, Jann Horn wrote:
> On Wed, Oct 1, 2025 at 1:18 PM Günther Noack <gnoack@google.com> wrote:
> > Introduce the LANDLOCK_RESTRICT_SELF_TSYNC flag. With this flag, a
> > given Landlock ruleset is applied to all threads of the calling
> > process, instead of only the current one.
> >
> > Without this flag, multithreaded userspace programs currently resort
> > to using the nptl(7)/libpsx hack for multithreaded policy enforcement,
> > which is also used by libcap and for setuid(2). Using this scheme,
> > the threads of a process enforce the same Landlock ruleset, but the
> > resulting Landlock domains are still separate, which makes a
> > difference for Landlock's "scoped" access rights, where the domain
> > identity and nesting is used. As a result, when using
> > LANLDOCK_SCOPE_SIGNAL, signaling between sibling threads stops
> > working. This is a problem for programming languages and frameworks
> > which are inherently multithreaded (e.g. Go).
>
> This looks good to me overall, though there are a couple details to fix.
>
> [...]
> > +static inline void landlock_cred_copy(struct landlock_cred_security *dst,
> > + const struct landlock_cred_security *src)
> > +{
> > + if (dst->domain)
> > + landlock_put_ruleset(dst->domain);
> > +
> > + *dst = *src;
>
> nit: I would add a short comment at the definition of struct
> landlock_cred_security noting that this function memcpy's the entire
> struct
Sounds good. I added a small remark "when updating this, also update
landlock_cred_copy() if needed".
> > +
> > + if (dst->domain)
> > + landlock_get_ruleset(src->domain);
> > +}
> [...]
> > +/*
> > + * tsync_works_grow_by - preallocates space for n more contexts in s
> > + *
> > + * Returns:
> > + * -ENOMEM if the (re)allocation fails
> > + * 0 if the allocation succeeds, partially succeeds, or no reallocation was needed
> > + */
> > +static int tsync_works_grow_by(struct tsync_works *s, size_t n, gfp_t flags)
> > +{
> > + int i;
> > + size_t new_capacity = s->capacity + n;
>
> (You only have to grow to `s->size + n` but I guess this works too.)
Thanks, well spotted. This was indeed the intended behavior, the
new_capacity <= s->capacity check also makes much more sense that
way. (I have a more detailed answer in another reply.) I fixed this
and also added an overflow check for good measure.
> > + struct tsync_work **works;
> > +
> > + if (new_capacity <= s->capacity)
> > + return 0;
> > +
> > + works = krealloc_array(s->works, new_capacity, sizeof(s->works[0]),
> > + flags);
> > + if (IS_ERR(works))
> > + return PTR_ERR(works);
>
> The kmalloc function family returns NULL on failure, so you have to
> check for NULL here instead of IS_ERR(), and then return -ENOMEM
> instead of PTR_ERR().
Thanks, fixed.
> > + s->works = works;
> > +
> > + for (i = s->capacity; i < new_capacity; i++) {
> > + s->works[i] = kzalloc(sizeof(*s->works[i]), flags);
> > + if (IS_ERR(s->works[i])) {
>
> (again, kzalloc() returns NULL on failure)
Done.
> > + /*
> > + * Leave the object in a consistent state,
> > + * but return an error.
> > + */
> > + s->capacity = i;
> > + return PTR_ERR(s->works[i]);
> > + }
> > + }
> > + s->capacity = new_capacity;
> > + return 0;
> > +}
> [...]
> > +/*
> > + * tsync_works_free - free memory held by s and drop all task references
> > + */
> > +static void tsync_works_free(struct tsync_works *s)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < s->size; i++)
> > + put_task_struct(s->works[i]->task);
>
> You'll need a NULL check before calling put_task_struct(), since the
> task_work_add() failure path can NULL out ->task. (Alternatively you
> could leave the task pointer intact in the task_work_add() failure
> path, since task_work_add() only fails if the task is already
> PF_EXITING. The &work_exited marker which causes task_work_add() to
> fail is only put on the task work list when task_work_run() runs on a
> PF_EXITING task.)
Thanks for spotting this, this is correct! I added a NULL check.
> > + for (i = 0; i < s->capacity; i++)
> > + kfree(s->works[i]);
> > + kfree(s->works);
> > + s->works = NULL;
> > + s->size = 0;
> > + s->capacity = 0;
> > +}
> > +
> > +/*
> > + * restrict_sibling_threads - enables a Landlock policy for all sibling threads
> > + */
> > +static int restrict_sibling_threads(const struct cred *old_cred,
> > + const struct cred *new_cred)
> > +{
> > + int res;
> > + struct task_struct *thread, *caller;
> > + struct tsync_shared_context shared_ctx;
> > + struct tsync_works works = {};
> > + size_t newly_discovered_threads;
> > + bool found_more_threads;
> > + struct tsync_work *ctx;
> > +
> > + atomic_set(&shared_ctx.preparation_error, 0);
> > + init_completion(&shared_ctx.all_prepared);
> > + init_completion(&shared_ctx.ready_to_commit);
> > + atomic_set(&shared_ctx.num_unfinished, 0);
>
> I think num_unfinished should be initialized to 1 here and decremented
> later on, I think, similar to how num_preparing works. Though it only
> matters in the edge case where the first thread we send task work to
> immediately fails the memory allocation. (And then you can also remove
> that "if (works.size)" check before
> "wait_for_completion(&shared_ctx.all_finished)".)
Thank you, good catch!
The works.size check was inaccurate, because in the case of an error
during task_work_add(), it wasn't actually counting the number of
scheduled task works, but overestimating it. The scenario is a bit
obscure, but initializing num_unfinished is a more robust approach
that rules out that variant of bugs.
> > + init_completion(&shared_ctx.all_finished);
> > + shared_ctx.old_cred = old_cred;
> > + shared_ctx.new_cred = new_cred;
> > +
> > + caller = current;
> [...]
> > + init_task_work(&ctx->work,
> > + restrict_one_thread_callback);
> > + res = task_work_add(thread, &ctx->work, TWA_SIGNAL);
> > + if (res) {
> > + /*
> > + * Remove the task from ctx so that we will
> > + * revisit the task at a later stage, if it
> > + * still exists.
> > + */
> > + put_task_struct_rcu_user(ctx->task);
>
> The complement to get_task_struct() is put_task_struct(), which I see
> you also used in tsync_works_free(). put_task_struct_rcu_user() is for
> a different, special type of task_struct reference.
Thanks, done.
> > + ctx->task = NULL;
> > +
> > + atomic_set(&shared_ctx.preparation_error, res);
>
> I think you don't want to set preparation_error here - that would
> cause the syscall to return -ESRCH if we happen to race with an
> exiting thread. Just remove that line - in the next iteration, we'll
> skip this thread even if it still exists, because it has PF_EXITING
> set by this point.
Thanks, that is correct and I fixed it as you suggested. -- The thread
exiting is the only reason why task_work_add() can fail. In the
(perfectly valid) case where one of the sibling threads happens to
exit, we do not want the landlock_restrict_self() syscall to fail just
because of that.
> > + atomic_dec(&shared_ctx.num_preparing);
> > + atomic_dec(&shared_ctx.num_unfinished);
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + /*
> > + * Decrement num_preparing for current, to undo that we
> > + * initialized it to 1 at the beginning of the inner loop.
> > + */
> > + if (atomic_dec_return(&shared_ctx.num_preparing) > 0)
> > + wait_for_completion(&shared_ctx.all_prepared);
>
> I'm sorry, because this will make the patch a little bit more
> complicated, but... I don't think you can use wait_for_completion()
> here. Consider the scenario where two userspace threads of the same
> process call this functionality (or a kernel subsystem that does
> something similar) simultaneously. Each thread will wait for the other
> indefinitely, and userspace won't even be able to resolve the deadlock
> by killing the processes.
> Similar issues would probably apply if, for example, GDB tried to
> attach to the process with bad timing - if GDB ptrace-stops another
> thread before you schedule task work for it, and then tries to
> ptrace-stop this thread, I think this thread could essentially be in a
> deadlock with GDB.
>
> You'll have to do something else here. I think the best solution would
> be to use wait_for_completion_interruptible() instead; then if that
> fails, tear down all the task work stuff that was already scheduled,
> and return with error -ERESTARTNOINTR. Something like (entirely
> untested):
>
> /* interruptible wait to avoid deadlocks while waiting for other tasks
> to enter our task work */
> if (wait_for_completion_interruptible(&shared_ctx.all_prepared)) {
> atomic_set(&shared_ctx.preparation_error, -ERESTARTNOINTR);
> for (int i=0; i<works.size; i++) {
> if (task_work_cancel(works.works[i]->task, &works.works[i]->work))
> if (atomic_dec_return(&shared_ctx.num_preparing))
> complete_all(&shared_ctx.all_prepared);
> }
> /* at this point we're only waiting for tasks that are already
> executing the task work */
> wait_for_completion(&shared_ctx.all_prepared);
> }
>
> Note that if the syscall returns -ERESTARTNOINTR, that won't be
> visible to userspace (except for debugging tools like strace/gdb); the
> kernel ensures that the syscall will transparently re-execute
> immediately. (It literally decrements the saved userspace instruction
> pointer by the size of a syscall instruction, so that when the kernel
> returns to userspace, the next instruction that executes will redo the
> syscall.) This allows us to break the deadlock without having to write
> any ugly retry logic or throwing userspace-visible errors.
Thank you again for catching this!
I used your suggestion, with the following (minor) differences:
1. Factored it out as a function (indentation level got too high...)
2. typo: call complete_all() only if atomic_dec_return() returns 0
3. Do the same barrier synchronization dance with num_unfinished/all_finished as well.
...and I also implemented a selftest for the case where
landlock_restrict_self() gets called by two adjacent threads at the
same time.
I'll write down my reasoning why this works for reference:
The problem in V2 is that we can run into a deadlock in the case where
two thread call landlock_restrict_self(). In that case, they'll both
become uninterruptible at syscall entry and try to schedule a
task_work for each other. Then, they proceed to wait for each other's
task_work to execute, which never happens because the task_work never
gets scheduled.
This is resolved by using an interruptible wait. With the
interruptible wait, the task can detect the condition where a signal
(or task work) comes in, execute that task_work and bail out of the
system call cleanly (we un-schedule the task_works for other threads
that are still pending and we abort all the task_works that have
already started to run by setting the shared_ctx.preparation_error).
After returning from the system call with -ERESTARTNOINTR, it gets
retried automatically to recover from the problem.
In all the other cases where we wait_for_completion() uninterruptibly,
we can reason about that returning, because these happen under
circumstances where we know that the task works we are waiting for
have all been started already.
I have reproduced the deadlock and verified the fixed implementation
with the selftest, by temporarily adding mdelay() calls and a lot of
logging in strategic places.
>
> > + } while (found_more_threads &&
> > + !atomic_read(&shared_ctx.preparation_error));
> > +
> > + /*
> > + * We now have all sibling threads blocking and in "prepared" state in
> > + * the task work. Ask all threads to commit.
> > + */
> > + complete_all(&shared_ctx.ready_to_commit);
> > +
> > + if (works.size)
> > + wait_for_completion(&shared_ctx.all_finished);
> > +
> > + tsync_works_free(&works);
> > +
> > + return atomic_read(&shared_ctx.preparation_error);
> > +}
> [...]
> > @@ -566,5 +987,13 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
> > new_llcred->domain_exec |= BIT(new_dom->num_layers - 1);
> > #endif /* CONFIG_AUDIT */
> >
> > + if (flags & LANDLOCK_RESTRICT_SELF_TSYNC) {
> > + res = restrict_sibling_threads(current_cred(), new_cred);
> > + if (res != 0) {
> > + abort_creds(new_cred);
> > + return res;
> > + }
> > + }
>
> Annoyingly, there is a special-case path above for the case where
> LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF is set without actually
> applying any ruleset. In that case you won't reach this point, and so
> LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF would only affect the
> current thread in that case. I doubt it'd be very noticeable, but
> still, it might be a good idea to rearrange things here a bit... maybe
> instead of the current `if (!ruleset) return commit_creds(new_cred);`,
> put some of the subsequent stuff in a `if (ruleset) {` block?
Thanks, I fixed that one as well.
—Günther
^ permalink raw reply
* Re: [PATCH v6 00/15] Create and use APIs to centralise locking for directory ops
From: NeilBrown @ 2025-11-27 11:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Alexander Viro, Amir Goldstein, Jan Kara, linux-fsdevel,
Jeff Layton, Chris Mason, David Sterba, David Howells,
Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
Tyler Hicks, Miklos Szeredi, Chuck Lever, Olga Kornievskaia,
Dai Ngo, Namjae Jeon, Steve French, Sergey Senozhatsky,
Carlos Maiolino, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <20251114-baden-banknoten-96fb107f79d7@brauner>
On Fri, 14 Nov 2025, Christian Brauner wrote:
> On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote:
> > Following is a new version of this series:
> > - fixed a bug found by syzbot
> > - cleanup suggested by Stephen Smalley
> > - added patch for missing updates in smb/server - thanks Jeff Layton
>
> The codeflow right now is very very gnarly in a lot of places which
> obviously isn't your fault. But start_creating() and end_creating()
> would very naturally lend themselves to be CLASS() guards.
I agree that using guards would be nice. One of my earlier versions did
that but Al wants the change to use guards to be separate from other
changes. I'll suggest something at some stage if no-one else does it first.
>
> Unrelated: I'm very inclined to slap a patch on top that renames
> start_creating()/end_creating() and start_dirop()/end_dirop() to
> vfs_start_creating()/vfs_end_creating() and
> vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level
> maintained helpers and I try to be consistent with the naming in the
> codebase making it very easy to grep.
>
I don't object to adding a vfs_ prefix.
(What would be really nice is of the vfs_ code was in a separate vfs/
directory, but that is probably too intrusive to be worth it).
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH v6 00/15] Create and use APIs to centralise locking for directory ops
From: NeilBrown @ 2025-11-27 11:11 UTC (permalink / raw)
To: Christian Brauner
Cc: Jeff Layton, Alexander Viro, Amir Goldstein, Jan Kara,
linux-fsdevel, Chris Mason, David Sterba, David Howells,
Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
Tyler Hicks, Miklos Szeredi, Chuck Lever, Olga Kornievskaia,
Dai Ngo, Namjae Jeon, Steve French, Sergey Senozhatsky,
Carlos Maiolino, John Johansen, Paul Moore, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Mateusz Guzik,
Lorenzo Stoakes, Stefan Berger, Darrick J. Wong, linux-kernel,
netfs, ecryptfs, linux-nfs, linux-unionfs, linux-cifs, linux-xfs,
linux-security-module, selinux
In-Reply-To: <20251114-liedgut-eidesstattlich-8c116178202f@brauner>
On Sat, 15 Nov 2025, Christian Brauner wrote:
> On Fri, Nov 14, 2025 at 01:24:41PM +0100, Christian Brauner wrote:
> > On Thu, Nov 13, 2025 at 11:18:23AM +1100, NeilBrown wrote:
> > > Following is a new version of this series:
> > > - fixed a bug found by syzbot
> > > - cleanup suggested by Stephen Smalley
> > > - added patch for missing updates in smb/server - thanks Jeff Layton
> >
> > The codeflow right now is very very gnarly in a lot of places which
> > obviously isn't your fault. But start_creating() and end_creating()
> > would very naturally lend themselves to be CLASS() guards.
> >
> > Unrelated: I'm very inclined to slap a patch on top that renames
> > start_creating()/end_creating() and start_dirop()/end_dirop() to
> > vfs_start_creating()/vfs_end_creating() and
> > vfs_start_dirop()/vfs_end_dirop(). After all they are VFS level
> > maintained helpers and I try to be consistent with the naming in the
> > codebase making it very easy to grep.
>
> @Neil, @Jeff, could you please look at:
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.all
>
> and specifically at the merge conflict resolution I did for:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.all&id=f28c9935f78bffe6fee62f7fb9f6c5af7e30d9b2
>
> and tell me whether it all looks sane?
>
That merge is a7b062be95fed490d1dcd350d3b5657f243d7d4f today, and I
agree with Jeff that it looks good.
Thanks,
NeilBrown
^ permalink raw reply
* [PATCH v3 0/3] Landlock multithreaded enforcement
From: Günther Noack @ 2025-11-27 11:51 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jann Horn, Serge Hallyn, Konstantin Meskhidze, Tingmao Wang,
Günther Noack
This patch set adds the LANDLOCK_RESTRICT_SELF_TSYNC flag to
landlock_restrict_self(). With this flag, the passed Landlock ruleset
will not only be applied to the calling thread, but to all threads
which belong to the same process.
Motivation
==========
TL;DR: The libpsx/nptl(7) signal hack which we use in user space for
multi-threaded Landlock enforcement is incompatible with Landlock's
signal scoping support. Landlock can restrict the use of signals
across Landlock domains, but we need signals ourselves in user space
in ways that are not permitted any more under these restrictions.
Enabling Landlock proves to be difficult in processes that are already
multi-threaded at the time of enforcement:
* Enforcement in only one thread is usually a mistake because threads
do not normally have proper security boundaries between them.
* Also, multithreading is unavoidable in some circumstances, such as
when using Landlock from a Go program. Go programs are already
multithreaded by the time that they enter the "func main()".
So far, the approach in Go[1] was to use libpsx[2]. This library
implements the mechanism described in nptl(7) [3]: It keeps track of
all threads with a linker hack and then makes all threads do the same
syscall by registering a signal handler for them and invoking it.
With commit 54a6e6bbf3be ("landlock: Add signal scoping"), Landlock
gained the ability to restrict the use of signals across different
Landlock domains.
Landlock's signal scoping support is incompatible with the libpsx
approach of enabling Landlock:
(1) With libpsx, although all threads enforce the same ruleset object,
they technically do the operation separately and end up in
distinct Landlock domains. This breaks signaling across threads
when using LANDLOCK_SCOPE_SIGNAL.
(2) Cross-thread Signals are themselves needed to enforce further
nested Landlock domains across multiple threads. So nested
Landlock policies become impossible there.
In addition to Landlock itself, cross-thread signals are also needed
for other seemingly-harmless API calls like the setuid(2) [4] and for
the use of libcap (co-developed with libpsx), which have the same
problem where the underlying syscall only applies to the calling
thread.
Implementation details
======================
Enforcement prerequisites
-------------------------
Normally, the prerequisite for enforcing a Landlock policy is to
either have CAP_SYS_ADMIN or the no_new_privs flag. With
LANDLOCK_RESTRICT_SELF_TSYNC, the no_new_privs flag will automatically
be applied for sibling threads if the caller had it.
These prerequisites and the "TSYNC" behavior work the same as for
Seccomp and its SECCOMP_FILTER_FLAG_TSYNC flag.
Pseudo-signals
--------------
Landlock domains are stored in struct cred, and a task's struct cred
can only be modified by the task itself [6].
To make that work, we use task_work_add() to register a pseudo-signal
for each of the affected threads. At signal execution time, these
tasks will coordinate to switch out their Landlock policy in lockstep
with each other, guaranteeing all-or-nothing semantics.
This implementation can be thought of as a kernel-side implementation
of the userspace hack that glibc/NPTL use for setuid(2) [3] [4], and
which libpsx implements for libcap [2].
Finding all sibling threads
---------------------------
In order to avoid grabbing the global task_list_lock, we employ the
scheme proposed by Jann Horn in [7]:
1. Loop through the list of sibling threads
2. Schedule a pseudo-signal for each and make each thread wait in the
pseudo-signal
3. Go back to 1. and look for more sibling thread that we have not
seen yet
Do this until no more new threads are found. As all threads were
waiting in their pseudo-signals, they can not spawn additional threads
and we found them all.
Coordination between tasks
--------------------------
As tasks run their pseudo-signal task work, they coordinate through
the following completions:
- all_prepared (with counter num_preparing)
When done, all new sibling threads in the inner loop(!) of finding
new threads are now in their pseudo-signal handlers and have
prepared the struct cred object to commit (or written an error into
the shared "preparation_error").
The lifetime of all_prepared is only the inner loop of finding new
threads.
- ready_to_commit
When done, the outer loop of finding new threads is done and all
sibling threads have prepared their struct cred object. Marked
completed by the calling thread.
- all_finished
When done, all sibling threads are done executing their
pseudo-signal handlers.
Use of credentials API
----------------------
Under normal circumstances, sibling threads share the same struct cred
object. To avoid unnecessary duplication, if we find that a thread
uses the same struct cred as the calling thread, we side-step the
normal use of the credentials API [6] and place a pointer to that
existing struct cred instead of creating a new one using
prepare_creds() in the sibling thread.
Noteworthy discussion points
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* We are side-stepping the normal credentials API [6], by re-wiring an
existing struct cred object instead of calling prepare_creds().
We can technically avoid it, but it would create unnecessary
duplicate struct cred objects in multithreaded scenarios.
Change Log
==========
v3:
- bigger organizational changes
- move tsync logic into own file
- tsync: extract count_additional_threads() and
schedule_task_work()
- code style
- restrict_one_thread, syscalls.c: use err instead of res (mic)
- restrict_one_thread: inline current_cred variable
- restrict_one_thread: add comment to shortcut logic (mic)
- rsync_works helpers: use size_t i for loop vars
- landlock_cred_copy: skip redundant NULL checks
- function name: s,tsync_works_free,tsync_works_release, (mic)
- tsync_works_grow_by: kzalloc into a temporary variable for
clarity (mic)
- tsync_works_contains_task: make struct task_works const
- bugs
- handle kmalloc family failures correctly (jannh)
- tsync_works_release: check task NULL ptr before put
- s/put_task_struct_rcu_user/put_task_struct/ (jannh)
- concurrency bugs
- schedule_task_work: do not return error when encountering exiting
tasks This can happen during normal operation, we should not
error due to it (jannh)
- landlock_restrict_sibling_threads: make current hold the
num_unfinished/all_finished barrier (more robust, jannh)
- un-wedge the deadlock using wait_for_completion_interruptible
(jannh) See "testing" below and discussion in
https://lore.kernel.org/all/CAG48ez1oS9kANZBq1bt+D76MX03DPHAFp76GJt7z5yx-Na1VLQ@mail.gmail.com/
- logic
- tsync_works_grow_by(): grow to size+n, not capacity+n
- tsync_works_grow_by(): add overflow check for capacity increase
- landlock_restrict_self(): make TSYNC and LOG flags work together
- set no_new_privs in the same way as seccomp,
whenever the calling thread had it
- testing
- add test where multiple threads call landlock_restrict_self()
concurrently
- test that no_new_privs is implicitly enabled for sibling threads
- bump ABI version to v8
- documentation improvements
- document ABI v8
- move flag documentation into the landlock.h header
- comment: Explain why we do not need sighand->siglock or
cred_guard_mutex
- various comment improvements
- reminder above struct landlock_cred_security about updating
landlock_cred_copy on changes
v2:
- https://lore.kernel.org/all/20250221184417.27954-2-gnoack3000@gmail.com/
- Semantics:
- Threads implicitly set NO_NEW_PRIVS unless they have
CAP_SYS_ADMIN, to fulfill Landlock policy enforcement
prerequisites
- Landlock policy gets unconditionally overridden even if the
previously established Landlock domains in sibling threads were
diverging.
- Restructure discovery of all sibling threads, with the algorithm
proposed by Jann Horn [7]: Loop through threads multiple times, and
get them all stuck in the pseudo signal (task work), until no new
sibling threads show up.
- Use RCU lock when iterating over sibling threads.
- Override existing Landlock domains of other threads,
instead of applying a new Landlock policy on top
- Directly re-wire the struct cred for sibling threads,
instread of creating a new one with prepare_creds().
- Tests:
- Remove multi_threaded_failure test
(The only remaining failure case is ENOMEM,
there is no good way to provoke that in a selftest)
- Add test for success despite diverging Landlock domains.
[1] https://github.com/landlock-lsm/go-landlock
[2] https://sites.google.com/site/fullycapable/who-ordered-libpsx
[3] https://man.gnoack.org/7/nptl
[4] https://man.gnoack.org/2/setuid#VERSIONS
[5] https://lore.kernel.org/all/20240805-remove-cred-transfer-v2-0-a2aa1d45e6b8@google.com/
[6] https://www.kernel.org/doc/html/latest/security/credentials.html
[7] https://lore.kernel.org/all/CAG48ez0pWg3OTABfCKRk5sWrURM-HdJhQMcWedEppc_z1rrVJw@mail.gmail.com/
Günther Noack (3):
landlock: Multithreading support for landlock_restrict_self()
landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
landlock: Document LANDLOCK_RESTRICT_SELF_TSYNC
Documentation/userspace-api/landlock.rst | 8 +
include/uapi/linux/landlock.h | 13 +
security/landlock/Makefile | 2 +-
security/landlock/cred.h | 12 +
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 66 ++-
security/landlock/tsync.c | 555 ++++++++++++++++++
security/landlock/tsync.h | 16 +
tools/testing/selftests/landlock/base_test.c | 8 +-
tools/testing/selftests/landlock/tsync_test.c | 161 +++++
10 files changed, 810 insertions(+), 33 deletions(-)
create mode 100644 security/landlock/tsync.c
create mode 100644 security/landlock/tsync.h
create mode 100644 tools/testing/selftests/landlock/tsync_test.c
--
2.52.0.177.g9f829587af-goog
^ permalink raw reply
* [PATCH v3 1/3] landlock: Multithreading support for landlock_restrict_self()
From: Günther Noack @ 2025-11-27 11:51 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jann Horn, Serge Hallyn, Konstantin Meskhidze, Tingmao Wang,
Günther Noack, Andrew G. Morgan, John Johansen, Paul Moore
In-Reply-To: <20251127115136.3064948-1-gnoack@google.com>
Introduce the LANDLOCK_RESTRICT_SELF_TSYNC flag. With this flag, a
given Landlock ruleset is applied to all threads of the calling
process, instead of only the current one.
Without this flag, multithreaded userspace programs currently resort
to using the nptl(7)/libpsx hack for multithreaded policy enforcement,
which is also used by libcap and for setuid(2). Using this
userspace-based scheme, the threads of a process enforce the same
Landlock policy, but the resulting Landlock domains are still
separate. The domains being separate causes multiple problems:
* When using Landlock's "scoped" access rights, the domain identity is
used to determine whether an operation is permitted. As a result,
when using LANLDOCK_SCOPE_SIGNAL, signaling between sibling threads
stops working. This is a problem for programming languages and
frameworks which are inherently multithreaded (e.g. Go).
* In audit logging, the domains of separate threads in a process will
get logged with different domain IDs, even when they are based on
the same ruleset FD, which migth confuse users.
Cc: Andrew G. Morgan <morgan@kernel.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Mickaël Salaün <mic@digikod.net>
Cc: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Günther Noack <gnoack@google.com>
---
include/uapi/linux/landlock.h | 13 +
security/landlock/Makefile | 2 +-
security/landlock/cred.h | 12 +
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 66 ++-
security/landlock/tsync.c | 555 +++++++++++++++++++
security/landlock/tsync.h | 16 +
tools/testing/selftests/landlock/base_test.c | 2 +-
8 files changed, 638 insertions(+), 30 deletions(-)
create mode 100644 security/landlock/tsync.c
create mode 100644 security/landlock/tsync.h
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index f030adc462ee..a60793767c4b 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -117,11 +117,24 @@ struct landlock_ruleset_attr {
* future nested domains, not the one being created. It can also be used
* with a @ruleset_fd value of -1 to mute subdomain logs without creating a
* domain.
+ *
+ * The following flag supports policy enforcement in multithreaded processes:
+ *
+ * %LANDLOCK_RESTRICT_SELF_TSYNC
+ * Applies the new Landlock configuration atomically to all threads of the
+ * current process, including the Landlock domain and logging
+ * configuration. This overrides the Landlock configuration of sibling
+ * threads, irrespective of previously established Landlock domains and
+ * logging configurations on these threads.
+ *
+ * If the calling thread is running with no_new_privs, this operation
+ * enables no_new_privs on the sibling threads as well.
*/
/* clang-format off */
#define LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF (1U << 0)
#define LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON (1U << 1)
#define LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF (1U << 2)
+#define LANDLOCK_RESTRICT_SELF_TSYNC (1U << 3)
/* clang-format on */
/**
diff --git a/security/landlock/Makefile b/security/landlock/Makefile
index 3160c2bdac1d..74122f814cf6 100644
--- a/security/landlock/Makefile
+++ b/security/landlock/Makefile
@@ -1,6 +1,6 @@
obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
-landlock-y := setup.o syscalls.o object.o ruleset.o \
+landlock-y := setup.o syscalls.o tsync.o object.o ruleset.o \
cred.o task.o fs.o
landlock-$(CONFIG_INET) += net.o
diff --git a/security/landlock/cred.h b/security/landlock/cred.h
index c82fe63ec598..c10a06727eb1 100644
--- a/security/landlock/cred.h
+++ b/security/landlock/cred.h
@@ -26,6 +26,8 @@
* This structure is packed to minimize the size of struct
* landlock_file_security. However, it is always aligned in the LSM cred blob,
* see lsm_set_blob_size().
+ *
+ * When updating this, also update landlock_cred_copy() if needed.
*/
struct landlock_cred_security {
/**
@@ -65,6 +67,16 @@ landlock_cred(const struct cred *cred)
return cred->security + landlock_blob_sizes.lbs_cred;
}
+static inline void landlock_cred_copy(struct landlock_cred_security *dst,
+ const struct landlock_cred_security *src)
+{
+ landlock_put_ruleset(dst->domain);
+
+ *dst = *src;
+
+ landlock_get_ruleset(src->domain);
+}
+
static inline struct landlock_ruleset *landlock_get_current_domain(void)
{
return landlock_cred(current_cred())->domain;
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 65b5ff051674..eb584f47288d 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -31,7 +31,7 @@
#define LANDLOCK_MASK_SCOPE ((LANDLOCK_LAST_SCOPE << 1) - 1)
#define LANDLOCK_NUM_SCOPE __const_hweight64(LANDLOCK_MASK_SCOPE)
-#define LANDLOCK_LAST_RESTRICT_SELF LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF
+#define LANDLOCK_LAST_RESTRICT_SELF LANDLOCK_RESTRICT_SELF_TSYNC
#define LANDLOCK_MASK_RESTRICT_SELF ((LANDLOCK_LAST_RESTRICT_SELF << 1) - 1)
/* clang-format on */
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 0116e9f93ffe..22b6200283f3 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -36,6 +36,7 @@
#include "net.h"
#include "ruleset.h"
#include "setup.h"
+#include "tsync.h"
static bool is_initialized(void)
{
@@ -161,7 +162,7 @@ static const struct file_operations ruleset_fops = {
* Documentation/userspace-api/landlock.rst should be updated to reflect the
* UAPI change.
*/
-const int landlock_abi_version = 7;
+const int landlock_abi_version = 8;
/**
* sys_landlock_create_ruleset - Create a new ruleset
@@ -454,9 +455,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
* - %LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF
* - %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON
* - %LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF
+ * - %LANDLOCK_RESTRICT_SELF_TSYNC
*
- * This system call enables to enforce a Landlock ruleset on the current
- * thread. Enforcing a ruleset requires that the task has %CAP_SYS_ADMIN in its
+ * This system call enforces a Landlock ruleset on the current thread.
+ * Enforcing a ruleset requires that the task has %CAP_SYS_ADMIN in its
* namespace or is running with no_new_privs. This avoids scenarios where
* unprivileged tasks can affect the behavior of privileged children.
*
@@ -484,6 +486,7 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
struct landlock_cred_security *new_llcred;
bool __maybe_unused log_same_exec, log_new_exec, log_subdomains,
prev_log_subdomains;
+ int err;
if (!is_initialized())
return -EOPNOTSUPP;
@@ -538,33 +541,42 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
* We could optimize this case by not calling commit_creds() if this flag
* was already set, but it is not worth the complexity.
*/
- if (!ruleset)
- return commit_creds(new_cred);
+ if (ruleset) {
+ /*
+ * There is no possible race condition while copying and
+ * manipulating the current credentials because they are
+ * dedicated per thread.
+ */
+ new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset);
+ if (IS_ERR(new_dom)) {
+ abort_creds(new_cred);
+ return PTR_ERR(new_dom);
+ }
- /*
- * There is no possible race condition while copying and manipulating
- * the current credentials because they are dedicated per thread.
- */
- new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset);
- if (IS_ERR(new_dom)) {
- abort_creds(new_cred);
- return PTR_ERR(new_dom);
+#ifdef CONFIG_AUDIT
+ new_dom->hierarchy->log_same_exec = log_same_exec;
+ new_dom->hierarchy->log_new_exec = log_new_exec;
+ if ((!log_same_exec && !log_new_exec) || !prev_log_subdomains)
+ new_dom->hierarchy->log_status = LANDLOCK_LOG_DISABLED;
+#endif /* CONFIG_AUDIT */
+
+ /* Replaces the old (prepared) domain. */
+ landlock_put_ruleset(new_llcred->domain);
+ new_llcred->domain = new_dom;
+
+#ifdef CONFIG_AUDIT
+ new_llcred->domain_exec |= BIT(new_dom->num_layers - 1);
+#endif /* CONFIG_AUDIT */
}
-#ifdef CONFIG_AUDIT
- new_dom->hierarchy->log_same_exec = log_same_exec;
- new_dom->hierarchy->log_new_exec = log_new_exec;
- if ((!log_same_exec && !log_new_exec) || !prev_log_subdomains)
- new_dom->hierarchy->log_status = LANDLOCK_LOG_DISABLED;
-#endif /* CONFIG_AUDIT */
-
- /* Replaces the old (prepared) domain. */
- landlock_put_ruleset(new_llcred->domain);
- new_llcred->domain = new_dom;
-
-#ifdef CONFIG_AUDIT
- new_llcred->domain_exec |= BIT(new_dom->num_layers - 1);
-#endif /* CONFIG_AUDIT */
+ if (flags & LANDLOCK_RESTRICT_SELF_TSYNC) {
+ err = landlock_restrict_sibling_threads(current_cred(),
+ new_cred);
+ if (err != 0) {
+ abort_creds(new_cred);
+ return err;
+ }
+ }
return commit_creds(new_cred);
}
diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
new file mode 100644
index 000000000000..a8db75259a8c
--- /dev/null
+++ b/security/landlock/tsync.c
@@ -0,0 +1,555 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock LSM - Cross-thread ruleset enforcement
+ *
+ * Copyright 2025 Google LLC
+ */
+
+#include <linux/atomic.h>
+#include <linux/completion.h>
+#include <linux/task_work.h>
+
+#include "cred.h"
+#include "tsync.h"
+
+/*
+ * Shared state between multiple threads which are enforcing Landlock rulesets
+ * in lockstep with each other.
+ */
+struct tsync_shared_context {
+ /* The old and tentative new creds of the calling thread. */
+ const struct cred *old_cred;
+ const struct cred *new_cred;
+
+ /* True if sibling tasks need to set the no_new_privs flag. */
+ bool set_no_new_privs;
+
+ /* An error encountered in preparation step, or 0. */
+ atomic_t preparation_error;
+
+ /*
+ * Barrier after preparation step in restrict_one_thread.
+ * The calling thread waits for completion.
+ *
+ * Re-initialized on every round of looking for newly spawned threads.
+ */
+ atomic_t num_preparing;
+ struct completion all_prepared;
+
+ /* Sibling threads wait for completion. */
+ struct completion ready_to_commit;
+
+ /*
+ * Barrier after commit step (used by syscall impl to wait for
+ * completion).
+ */
+ atomic_t num_unfinished;
+ struct completion all_finished;
+};
+
+struct tsync_work {
+ struct callback_head work;
+ struct task_struct *task;
+ struct tsync_shared_context *shared_ctx;
+};
+
+/*
+ * restrict_one_thread - update a thread's Landlock domain in lockstep with the
+ * other threads in the same process
+ *
+ * When this is run, the same function gets run in all other threads in the same
+ * process (except for the calling thread which called landlock_restrict_self).
+ * The concurrently running invocations of restrict_one_thread coordinate
+ * through the shared ctx object to do their work in lockstep to implement
+ * all-or-nothing semantics for enforcing the new Landlock domain.
+ *
+ * Afterwards, depending on the presence of an error, all threads either commit
+ * or abort the prepared credentials. The commit operation can not fail any more.
+ */
+static void restrict_one_thread(struct tsync_shared_context *ctx)
+{
+ int err;
+ struct cred *cred = NULL;
+
+ if (current_cred() == ctx->old_cred) {
+ /*
+ * Switch out old_cred with new_cred, if possible.
+ *
+ * In the common case, where all threads initially point to the
+ * same struct cred, this optimization avoids creating separate
+ * redundant credentials objects for each, which would all have
+ * the same contents.
+ *
+ * Note: We are intentionally dropping the const qualifier here,
+ * because it is required by commit_creds() and abort_creds().
+ */
+ cred = (struct cred *)get_cred(ctx->new_cred);
+ } else {
+ /* Else, prepare new creds and populate them. */
+ cred = prepare_creds();
+
+ if (!cred) {
+ atomic_set(&ctx->preparation_error, -ENOMEM);
+
+ /*
+ * Even on error, we need to adhere to the protocol and
+ * coordinate with concurrently running invocations.
+ */
+ if (atomic_dec_return(&ctx->num_preparing) == 0)
+ complete_all(&ctx->all_prepared);
+
+ goto out;
+ }
+
+ landlock_cred_copy(landlock_cred(cred),
+ landlock_cred(ctx->new_cred));
+ }
+
+ /*
+ * Barrier: Wait until all threads are done preparing.
+ * After this point, we can have no more failures.
+ */
+ if (atomic_dec_return(&ctx->num_preparing) == 0)
+ complete_all(&ctx->all_prepared);
+
+ /*
+ * Wait for signal from calling thread that it's safe to read the
+ * preparation error now and we are ready to commit (or abort).
+ */
+ wait_for_completion(&ctx->ready_to_commit);
+
+ /* Abort the commit if any of the other threads had an error. */
+ err = atomic_read(&ctx->preparation_error);
+ if (err) {
+ abort_creds(cred);
+ goto out;
+ }
+
+ /*
+ * Make sure that all sibling tasks fulfill the no_new_privs
+ * prerequisite. (This is in line with Seccomp's
+ * SECCOMP_FILTER_FLAG_TSYNC logic in kernel/seccomp.c.)
+ */
+ if (ctx->set_no_new_privs)
+ task_set_no_new_privs(current);
+
+ commit_creds(cred);
+
+out:
+ /* Notify the calling thread once all threads are done */
+ if (atomic_dec_return(&ctx->num_unfinished) == 0)
+ complete_all(&ctx->all_finished);
+}
+
+/*
+ * restrict_one_thread_callback - task_work callback for restricting a thread
+ *
+ * Calls restrict_one_thread with the struct landlock_shared_tsync_context.
+ */
+static void restrict_one_thread_callback(struct callback_head *work)
+{
+ struct tsync_work *ctx = container_of(work, struct tsync_work, work);
+
+ restrict_one_thread(ctx->shared_ctx);
+}
+
+/*
+ * struct tsync_works - a growable array of per-task contexts
+ *
+ * The zero-initialized struct represents the empty array.
+ */
+struct tsync_works {
+ struct tsync_work **works;
+ size_t size;
+ size_t capacity;
+};
+
+/*
+ * tsync_works_provide - provides a preallocated tsync_work for the given task
+ *
+ * This also stores a task pointer in the context and increments the reference
+ * count of the task.
+ *
+ * This function may fail in the case where we did not preallocate sufficient
+ * capacity. This can legitimately happen if new threads get started after we
+ * grew the capacity.
+ *
+ * Returns:
+ * A pointer to the preallocated context struct, with task filled in.
+ *
+ * NULL, if we ran out of preallocated context structs.
+ */
+static struct tsync_work *tsync_works_provide(struct tsync_works *s,
+ struct task_struct *task)
+{
+ struct tsync_work *ctx;
+
+ if (s->size >= s->capacity)
+ return NULL;
+
+ ctx = s->works[s->size];
+ s->size++;
+
+ ctx->task = get_task_struct(task);
+ return ctx;
+}
+
+/*
+ * tsync_works_grow_by - preallocates space for n more contexts in s
+ *
+ * On a successful return, the subsequent n calls to tsync_works_provide() are
+ * guaranteed to succeed. (size + n <= capacity)
+ *
+ * Returns:
+ * -ENOMEM if the (re)allocation fails
+ * 0 if the allocation succeeds, partially succeeds, or no reallocation was needed
+ */
+static int tsync_works_grow_by(struct tsync_works *s, size_t n, gfp_t flags)
+{
+ size_t i;
+ size_t new_capacity;
+ struct tsync_work **works;
+ struct tsync_work *work;
+
+ if (check_add_overflow(s->size, n, &new_capacity))
+ return -EOVERFLOW;
+
+ /* No need to reallocate if s already has sufficient capacity. */
+ if (new_capacity <= s->capacity)
+ return 0;
+
+ works = krealloc_array(s->works, new_capacity, sizeof(s->works[0]),
+ flags);
+ if (!works)
+ return -ENOMEM;
+
+ s->works = works;
+
+ for (i = s->capacity; i < new_capacity; i++) {
+ work = kzalloc(sizeof(*work), flags);
+ if (!work) {
+ /*
+ * Leave the object in a consistent state,
+ * but return an error.
+ */
+ s->capacity = i;
+ return -ENOMEM;
+ }
+ s->works[i] = work;
+ }
+ s->capacity = new_capacity;
+ return 0;
+}
+
+/*
+ * tsync_works_contains - checks for presence of task in s
+ */
+static bool tsync_works_contains_task(const struct tsync_works *s,
+ struct task_struct *task)
+{
+ size_t i;
+
+ for (i = 0; i < s->size; i++)
+ if (s->works[i]->task == task)
+ return true;
+ return false;
+}
+
+/*
+ * tsync_works_release - frees memory held by s and drops all task references
+ *
+ * This does not free s itself, only the data structures held by it.
+ */
+static void tsync_works_release(struct tsync_works *s)
+{
+ size_t i;
+
+ for (i = 0; i < s->size; i++) {
+ if (!s->works[i]->task)
+ continue;
+
+ put_task_struct(s->works[i]->task);
+ }
+
+ for (i = 0; i < s->capacity; i++)
+ kfree(s->works[i]);
+ kfree(s->works);
+ s->works = NULL;
+ s->size = 0;
+ s->capacity = 0;
+}
+
+/*
+ * count_additional_threads - counts the sibling threads that are not in works
+ */
+static size_t count_additional_threads(const struct tsync_works *works)
+{
+ struct task_struct *thread, *caller;
+ size_t n = 0;
+
+ caller = current;
+
+ guard(rcu)();
+
+ for_each_thread(caller, thread) {
+ /* Skip current, since it is initiating the sync. */
+ if (thread == caller)
+ continue;
+
+ /* Skip exited threads. */
+ if (thread->flags & PF_EXITING)
+ continue;
+
+ /* Skip threads that we have already seen. */
+ if (tsync_works_contains_task(works, thread))
+ continue;
+
+ n++;
+ }
+ return n;
+}
+
+/*
+ * schedule_task_work - adds task_work for all eligible sibling threads
+ * which have not been scheduled yet
+ *
+ * For each added task_work, atomically increments shared_ctx->num_preparing and
+ * shared_ctx->num_unfinished.
+ *
+ * Returns:
+ * true, if at least one eligible sibling thread was found
+ */
+static bool schedule_task_work(struct tsync_works *works,
+ struct tsync_shared_context *shared_ctx)
+{
+ int err;
+ struct task_struct *thread, *caller;
+ struct tsync_work *ctx;
+ bool found_more_threads = false;
+
+ caller = current;
+
+ guard(rcu)();
+
+ for_each_thread(caller, thread) {
+ /* Skip current, since it is initiating the sync. */
+ if (thread == caller)
+ continue;
+
+ /* Skip exited threads. */
+ if (thread->flags & PF_EXITING)
+ continue;
+
+ /* Skip threads that we already looked at. */
+ if (tsync_works_contains_task(works, thread))
+ continue;
+
+ /*
+ * We found a sibling thread that is not doing its task_work
+ * yet, and which might spawn new threads before our task work
+ * runs, so we need at least one more round in the outer loop.
+ */
+ found_more_threads = true;
+
+ ctx = tsync_works_provide(works, thread);
+ if (!ctx) {
+ /*
+ * We ran out of preallocated contexts -- we need to try
+ * again with this thread at a later time!
+ * found_more_threads is already true at this point.
+ */
+ break;
+ }
+
+ ctx->shared_ctx = shared_ctx;
+
+ atomic_inc(&shared_ctx->num_preparing);
+ atomic_inc(&shared_ctx->num_unfinished);
+
+ init_task_work(&ctx->work, restrict_one_thread_callback);
+ err = task_work_add(thread, &ctx->work, TWA_SIGNAL);
+ if (err) {
+ /*
+ * task_work_add() only fails if the task is about to
+ * exit. We checked that earlier, but it can happen as
+ * a race. Resume without setting an error, as the task
+ * is probably gone in the next loop iteration. For
+ * consistency, remove the task from ctx so that it does
+ * not look like we handed it a task_work.
+ */
+ put_task_struct(ctx->task);
+ ctx->task = NULL;
+
+ atomic_dec(&shared_ctx->num_preparing);
+ atomic_dec(&shared_ctx->num_unfinished);
+ }
+ }
+
+ return found_more_threads;
+}
+
+/*
+ * cancel_tsync_works - cancel all task works where it is possible
+ *
+ * Task works can be canceled as long as they are still queued and have not
+ * started running. If they get canceled, we decrement
+ * shared_ctx->num_preparing and shared_ctx->num_unfished and mark the two
+ * completions if needed, as if the task was never scheduled.
+ */
+static void cancel_tsync_works(struct tsync_works *works,
+ struct tsync_shared_context *shared_ctx)
+{
+ int i;
+
+ for (i = 0; i < works->size; i++) {
+ if (!task_work_cancel(works->works[i]->task,
+ &works->works[i]->work))
+ continue;
+
+ /* After dequeueing, act as if the task work had executed. */
+
+ if (atomic_dec_return(&shared_ctx->num_preparing) == 0)
+ complete_all(&shared_ctx->all_prepared);
+
+ if (atomic_dec_return(&shared_ctx->num_unfinished) == 0)
+ complete_all(&shared_ctx->all_finished);
+ }
+}
+
+/*
+ * restrict_sibling_threads - enables a Landlock policy for all sibling threads
+ */
+int landlock_restrict_sibling_threads(const struct cred *old_cred,
+ const struct cred *new_cred)
+{
+ int err;
+ struct tsync_shared_context shared_ctx;
+ struct tsync_works works = {};
+ size_t newly_discovered_threads;
+ bool found_more_threads;
+
+ atomic_set(&shared_ctx.preparation_error, 0);
+ init_completion(&shared_ctx.all_prepared);
+ init_completion(&shared_ctx.ready_to_commit);
+ atomic_set(&shared_ctx.num_unfinished, 1);
+ init_completion(&shared_ctx.all_finished);
+ shared_ctx.old_cred = old_cred;
+ shared_ctx.new_cred = new_cred;
+ shared_ctx.set_no_new_privs = task_no_new_privs(current);
+
+ /*
+ * We schedule a pseudo-signal task_work for each of the calling task's
+ * sibling threads. In the task work, each thread:
+ *
+ * 1) runs prepare_creds() and writes back the error to
+ * shared_ctx.preparation_error, if needed.
+ *
+ * 2) signals that it's done with prepare_creds() to the calling task.
+ * (completion "all_prepared").
+ *
+ * 3) waits for the completion "ready_to_commit". This is sent by the
+ * calling task after ensuring that all sibling threads have done
+ * with the "preparation" stage.
+ *
+ * After this barrier is reached, it's safe to read
+ * shared_ctx.preparation_error.
+ *
+ * 4) reads shared_ctx.preparation_error and then either does
+ * commit_creds() or abort_creds().
+ *
+ * 5) signals that it's done altogether (barrier synchronization
+ * "all_finished")
+ *
+ * Unlike seccomp, which modifies sibling tasks directly, we do not need
+ * to acquire the cred_guard_mutex and sighand->siglock:
+ *
+ * * As in our case, all threads are themselves exchanging their own
+ * struct cred through the credentials API, no locks are needed for
+ * that.
+ * * Our for_each_thread() loops are protected by RCU.
+ * * We do not acquire a lock to keep the list of sibling threads stable
+ * between our for_each_thread loops. If the list of available
+ * sibling threads changes between these for_each_thread loops, we
+ * make up for that by continuing to look for threads until they are
+ * all discovered and have entered their task_work, where they are
+ * unable to spawn new threads.
+ */
+ do {
+ /* In RCU read-lock, count the threads we need. */
+ newly_discovered_threads = count_additional_threads(&works);
+
+ if (newly_discovered_threads == 0)
+ break; /* done */
+
+ err = tsync_works_grow_by(&works, newly_discovered_threads,
+ GFP_KERNEL_ACCOUNT);
+ if (err) {
+ atomic_set(&shared_ctx.preparation_error, err);
+ break;
+ }
+
+ /*
+ * The "all_prepared" barrier is used locally to the loop body,
+ * this use of for_each_thread(). We can reset it on each loop
+ * iteration because all previous loop iterations are done with
+ * it already.
+ *
+ * num_preparing is initialized to 1 so that the counter can not
+ * go to 0 and mark the completion as done before all task works
+ * are registered. We decrement it at the end of the loop body.
+ */
+ atomic_set(&shared_ctx.num_preparing, 1);
+ reinit_completion(&shared_ctx.all_prepared);
+
+ /* In RCU read-lock, schedule task work on newly discovered sibling tasks. */
+ found_more_threads = schedule_task_work(&works, &shared_ctx);
+
+ /*
+ * Decrement num_preparing for current, to undo that we
+ * initialized it to 1 a few lines above.
+ */
+ if (atomic_dec_return(&shared_ctx.num_preparing) > 0) {
+ if (wait_for_completion_interruptible(
+ &shared_ctx.all_prepared)) {
+ /*
+ * In case of interruption, we need to retry the
+ * system call.
+ */
+ atomic_set(&shared_ctx.preparation_error,
+ -ERESTARTNOINTR);
+
+ /*
+ * Cancel task works for tasks that did not
+ * start running yet, and decrement all_prepared
+ * and num_unfinished accordingly.
+ */
+ cancel_tsync_works(&works, &shared_ctx);
+
+ /*
+ * The remaining task works have started
+ * running, so waiting for their completion will
+ * finish.
+ */
+ wait_for_completion(&shared_ctx.all_prepared);
+ }
+ }
+ } while (found_more_threads &&
+ !atomic_read(&shared_ctx.preparation_error));
+
+ /*
+ * We now have all sibling threads blocking and in "prepared" state in
+ * the task work. Ask all threads to commit.
+ */
+ complete_all(&shared_ctx.ready_to_commit);
+
+ /*
+ * Decrement num_unfinished for current, to undo that we initialized it
+ * to 1 at the beginning.
+ */
+ if (atomic_dec_return(&shared_ctx.num_unfinished) > 0)
+ wait_for_completion(&shared_ctx.all_finished);
+
+ tsync_works_release(&works);
+
+ return atomic_read(&shared_ctx.preparation_error);
+}
diff --git a/security/landlock/tsync.h b/security/landlock/tsync.h
new file mode 100644
index 000000000000..b85586db8b51
--- /dev/null
+++ b/security/landlock/tsync.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock LSM - Cross-thread ruleset enforcement
+ *
+ * Copyright 2025 Google LLC
+ */
+
+#ifndef _SECURITY_LANDLOCK_TSYNC_H
+#define _SECURITY_LANDLOCK_TSYNC_H
+
+#include "cred.h"
+
+int landlock_restrict_sibling_threads(const struct cred *old_cred,
+ const struct cred *new_cred);
+
+#endif /* _SECURITY_LANDLOCK_TSYNC_H */
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 7b69002239d7..f4b1a275d8d9 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -76,7 +76,7 @@ TEST(abi_version)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
};
- ASSERT_EQ(7, landlock_create_ruleset(NULL, 0,
+ ASSERT_EQ(8, landlock_create_ruleset(NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION));
ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
--
2.52.0.177.g9f829587af-goog
^ permalink raw reply related
* [PATCH v3 2/3] landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
From: Günther Noack @ 2025-11-27 11:51 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jann Horn, Serge Hallyn, Konstantin Meskhidze, Tingmao Wang,
Günther Noack, Andrew G. Morgan, John Johansen, Paul Moore
In-Reply-To: <20251127115136.3064948-1-gnoack@google.com>
Exercise various scenarios where Landlock domains are enforced across
all of a processes' threads.
Cc: Andrew G. Morgan <morgan@kernel.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Mickaël Salaün <mic@digikod.net>
Cc: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Günther Noack <gnoack@google.com>
---
tools/testing/selftests/landlock/base_test.c | 6 +-
tools/testing/selftests/landlock/tsync_test.c | 161 ++++++++++++++++++
2 files changed, 164 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/landlock/tsync_test.c
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index f4b1a275d8d9..0fea236ef4bd 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -288,7 +288,7 @@ TEST(restrict_self_fd)
EXPECT_EQ(EBADFD, errno);
}
-TEST(restrict_self_fd_flags)
+TEST(restrict_self_fd_logging_flags)
{
int fd;
@@ -304,9 +304,9 @@ TEST(restrict_self_fd_flags)
EXPECT_EQ(EBADFD, errno);
}
-TEST(restrict_self_flags)
+TEST(restrict_self_logging_flags)
{
- const __u32 last_flag = LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF;
+ const __u32 last_flag = LANDLOCK_RESTRICT_SELF_TSYNC;
/* Tests invalid flag combinations. */
diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
new file mode 100644
index 000000000000..3971e0f02c49
--- /dev/null
+++ b/tools/testing/selftests/landlock/tsync_test.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock tests - Enforcing the same restrictions across multiple threads
+ *
+ * Copyright © 2025 Günther Noack <gnoack3000@gmail.com>
+ */
+
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <sys/prctl.h>
+#include <linux/landlock.h>
+
+#include "common.h"
+
+/* create_ruleset - Create a simple ruleset FD common to all tests */
+static int create_ruleset(struct __test_metadata *const _metadata)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = (LANDLOCK_ACCESS_FS_WRITE_FILE |
+ LANDLOCK_ACCESS_FS_TRUNCATE),
+ };
+ const int ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+
+ ASSERT_LE(0, ruleset_fd)
+ {
+ TH_LOG("landlock_create_ruleset: %s", strerror(errno));
+ }
+ return ruleset_fd;
+}
+
+TEST(single_threaded_success)
+{
+ const int ruleset_fd = create_ruleset(_metadata);
+
+ disable_caps(_metadata);
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+ ASSERT_EQ(0, landlock_restrict_self(ruleset_fd,
+ LANDLOCK_RESTRICT_SELF_TSYNC));
+
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+void store_no_new_privs(void *data)
+{
+ bool *nnp = data;
+
+ if (!nnp)
+ return;
+ *nnp = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
+}
+
+void *idle(void *data)
+{
+ pthread_cleanup_push(store_no_new_privs, data);
+
+ while (true)
+ sleep(1);
+
+ pthread_cleanup_pop(1);
+}
+
+TEST(multi_threaded_success)
+{
+ pthread_t t1, t2;
+ bool no_new_privs1, no_new_privs2;
+ const int ruleset_fd = create_ruleset(_metadata);
+
+ disable_caps(_metadata);
+
+ ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
+ ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+
+ EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
+ LANDLOCK_RESTRICT_SELF_TSYNC));
+
+ ASSERT_EQ(0, pthread_cancel(t1));
+ ASSERT_EQ(0, pthread_cancel(t2));
+ ASSERT_EQ(0, pthread_join(t1, NULL));
+ ASSERT_EQ(0, pthread_join(t2, NULL));
+
+ /* The no_new_privs flag was implicitly enabled on all threads. */
+ EXPECT_TRUE(no_new_privs1);
+ EXPECT_TRUE(no_new_privs2);
+
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+TEST(multi_threaded_success_despite_diverging_domains)
+{
+ pthread_t t1, t2;
+ const int ruleset_fd = create_ruleset(_metadata);
+
+ disable_caps(_metadata);
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+
+ ASSERT_EQ(0, pthread_create(&t1, NULL, idle, NULL));
+ ASSERT_EQ(0, pthread_create(&t2, NULL, idle, NULL));
+
+ /*
+ * The main thread enforces a ruleset,
+ * thereby bringing the threads' Landlock domains out of sync.
+ */
+ EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
+
+ /* Still, TSYNC succeeds, bringing the threads in sync again. */
+ EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
+ LANDLOCK_RESTRICT_SELF_TSYNC));
+
+ ASSERT_EQ(0, pthread_cancel(t1));
+ ASSERT_EQ(0, pthread_cancel(t2));
+ ASSERT_EQ(0, pthread_join(t1, NULL));
+ ASSERT_EQ(0, pthread_join(t2, NULL));
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+struct thread_restrict_data {
+ pthread_t t;
+ int ruleset_fd;
+ int result;
+};
+
+void *thread_restrict(void *data)
+{
+ struct thread_restrict_data *d = data;
+
+ d->result = landlock_restrict_self(d->ruleset_fd,
+ LANDLOCK_RESTRICT_SELF_TSYNC);
+ return NULL;
+}
+
+TEST(competing_enablement)
+{
+ const int ruleset_fd = create_ruleset(_metadata);
+ struct thread_restrict_data d[] = {
+ { .ruleset_fd = ruleset_fd },
+ { .ruleset_fd = ruleset_fd },
+ };
+
+ disable_caps(_metadata);
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+ ASSERT_EQ(0, pthread_create(&d[0].t, NULL, thread_restrict, &d[0]));
+ ASSERT_EQ(0, pthread_create(&d[1].t, NULL, thread_restrict, &d[1]));
+
+ /* Wait for threads to finish. */
+ ASSERT_EQ(0, pthread_join(d[0].t, NULL));
+ ASSERT_EQ(0, pthread_join(d[1].t, NULL));
+
+ /* Expect that both succeeded. */
+ EXPECT_EQ(0, d[0].result);
+ EXPECT_EQ(0, d[1].result);
+
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+TEST_HARNESS_MAIN
--
2.52.0.177.g9f829587af-goog
^ permalink raw reply related
* [PATCH v3 3/3] landlock: Document LANDLOCK_RESTRICT_SELF_TSYNC
From: Günther Noack @ 2025-11-27 11:51 UTC (permalink / raw)
To: linux-security-module, Mickaël Salaün
Cc: Jann Horn, Serge Hallyn, Konstantin Meskhidze, Tingmao Wang,
Günther Noack, Andrew G. Morgan, John Johansen, Paul Moore
In-Reply-To: <20251127115136.3064948-1-gnoack@google.com>
Add documentation for LANDLOCK_RESTRICT_SELF_TSYNC. It does not need to go
into the main example, but it has a section in the ABI compatibility notes.
In the HTML rendering, the main reference is the system call documentation,
which is included from the landlock.h header file.
Cc: Andrew G. Morgan <morgan@kernel.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Mickaël Salaün <mic@digikod.net>
Cc: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Günther Noack <gnoack@google.com>
---
Documentation/userspace-api/landlock.rst | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 1d0c2c15c22e..f1251da9877c 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -604,6 +604,14 @@ Landlock audit events with the ``LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF``,
sys_landlock_restrict_self(). See Documentation/admin-guide/LSM/landlock.rst
for more details on audit.
+Thread synchronization (ABI < 8)
+--------------------------------
+
+Starting with the Landlock ABI version 8, it is now possible to
+enforce Landlock rulesets across all threads of the calling process
+using the ``LANDLOCK_RESTRICT_SELF_TSYNC`` flag passed to
+sys_landlock_restrict_self().
+
.. _kernel_support:
Kernel support
--
2.52.0.177.g9f829587af-goog
^ permalink raw reply related
* [PATCH v7 03/11] KEYS: trusted: remove redundant instance of tpm2_hash_map
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, James Bottomley,
Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-kernel, keyrings, linux-security-module
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
Trusted keys duplicates tpm2_hash_map from TPM driver internals. Implement
and export `tpm2_find_hash_alg()` in order to address this glitch, and
replace redundant code block with a call this new function.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v7:
- A new patch.
---
drivers/char/tpm/tpm2-cmd.c | 19 +++++++++++++++--
include/linux/tpm.h | 7 ++-----
security/keys/trusted-keys/trusted_tpm2.c | 25 +++++------------------
3 files changed, 24 insertions(+), 27 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 97501c567c34..1393bfbeca64 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -18,7 +18,10 @@ static bool disable_pcr_integrity;
module_param(disable_pcr_integrity, bool, 0444);
MODULE_PARM_DESC(disable_pcr_integrity, "Disable integrity protection of TPM2_PCR_Extend");
-static struct tpm2_hash tpm2_hash_map[] = {
+static struct {
+ unsigned int crypto_id;
+ unsigned int alg_id;
+} tpm2_hash_map[] = {
{HASH_ALGO_SHA1, TPM_ALG_SHA1},
{HASH_ALGO_SHA256, TPM_ALG_SHA256},
{HASH_ALGO_SHA384, TPM_ALG_SHA384},
@@ -26,6 +29,18 @@ static struct tpm2_hash tpm2_hash_map[] = {
{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
};
+int tpm2_find_hash_alg(unsigned int crypto_id)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++)
+ if (crypto_id == tpm2_hash_map[i].crypto_id)
+ return tpm2_hash_map[i].alg_id;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tpm2_find_hash_alg);
+
int tpm2_get_timeouts(struct tpm_chip *chip)
{
chip->timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
@@ -490,7 +505,7 @@ static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
- if (bank->alg_id != tpm2_hash_map[i].tpm_id)
+ if (bank->alg_id != tpm2_hash_map[i].alg_id)
continue;
bank->digest_size = hash_digest_size[crypto_algo];
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0e9e043f728c..e5fc7b73de2d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -410,11 +410,6 @@ enum tpm2_session_attributes {
TPM2_SA_AUDIT = BIT(7),
};
-struct tpm2_hash {
- unsigned int crypto_id;
- unsigned int tpm_id;
-};
-
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
int tpm_buf_init_sized(struct tpm_buf *buf);
@@ -465,6 +460,7 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
+unsigned int tpm2_alg_to_crypto_id(unsigned int alg_id);
extern int tpm_is_tpm2(struct tpm_chip *chip);
extern __must_check int tpm_try_get_ops(struct tpm_chip *chip);
extern void tpm_put_ops(struct tpm_chip *chip);
@@ -477,6 +473,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
extern struct tpm_chip *tpm_default_chip(void);
void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
+int tpm2_find_hash_alg(unsigned int crypto_id);
static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle)
{
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 024be262702f..3205732fb4b7 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -18,14 +18,6 @@
#include "tpm2key.asn1.h"
-static struct tpm2_hash tpm2_hash_map[] = {
- {HASH_ALGO_SHA1, TPM_ALG_SHA1},
- {HASH_ALGO_SHA256, TPM_ALG_SHA256},
- {HASH_ALGO_SHA384, TPM_ALG_SHA384},
- {HASH_ALGO_SHA512, TPM_ALG_SHA512},
- {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
-};
-
static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
static int tpm2_key_encode(struct trusted_key_payload *payload,
@@ -244,24 +236,17 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
off_t offset = TPM_HEADER_SIZE;
struct tpm_buf buf, sized;
int blob_len = 0;
- u32 hash;
+ int hash;
u32 flags;
- int i;
int rc;
- for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
- if (options->hash == tpm2_hash_map[i].crypto_id) {
- hash = tpm2_hash_map[i].tpm_id;
- break;
- }
- }
-
- if (i == ARRAY_SIZE(tpm2_hash_map))
- return -EINVAL;
-
if (!options->keyhandle)
return -EINVAL;
+ hash = tpm2_find_hash_alg(options->hash);
+ if (hash)
+ return hash;
+
rc = tpm_try_get_ops(chip);
if (rc)
return rc;
--
2.52.0
^ permalink raw reply related
* [PATCH v7 04/11] KEYS: trusted: Fix memory leak in tpm2_load()
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, stable, James Bottomley, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
tpm2_load() allocates a blob indirectly via tpm2_key_decode() but it is
not freed in all failure paths. Address this with a scope-based cleanup
helper __free(). For legacy blobs, the implicit de-allocation is gets
disable by no_free_ptr().
Cc: stable@vger.kernel.org # v5.13+
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v7:
- Fix compiler warning.
v6:
- A new patch in this version.
---
security/keys/trusted-keys/trusted_tpm2.c | 24 +++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 3205732fb4b7..00bc1afb32c8 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -98,9 +98,8 @@ struct tpm2_key_context {
u32 priv_len;
};
-static int tpm2_key_decode(struct trusted_key_payload *payload,
- struct trusted_key_options *options,
- u8 **buf)
+static void *tpm2_key_decode(struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
{
int ret;
struct tpm2_key_context ctx;
@@ -111,16 +110,15 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
payload->blob_len);
if (ret < 0)
- return ret;
+ return ERR_PTR(ret);
if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
if (!blob)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
- *buf = blob;
options->keyhandle = ctx.parent;
memcpy(blob, ctx.priv, ctx.priv_len);
@@ -128,7 +126,7 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
memcpy(blob, ctx.pub, ctx.pub_len);
- return 0;
+ return blob;
}
int tpm2_key_parent(void *context, size_t hdrlen,
@@ -372,6 +370,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
struct trusted_key_options *options,
u32 *blob_handle)
{
+ u8 *blob_ref __free(kfree) = NULL;
struct tpm_buf buf;
unsigned int private_len;
unsigned int public_len;
@@ -380,11 +379,14 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
int rc;
u32 attrs;
- rc = tpm2_key_decode(payload, options, &blob);
- if (rc) {
+ blob = tpm2_key_decode(payload, options);
+ if (IS_ERR(blob)) {
/* old form */
blob = payload->blob;
payload->old_format = 1;
+ } else {
+ /* Bind to cleanup: */
+ blob_ref = blob;
}
/* new format carries keyhandle but old format doesn't */
@@ -449,8 +451,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
(__be32 *) &buf.data[TPM_HEADER_SIZE]);
out:
- if (blob != payload->blob)
- kfree(blob);
tpm_buf_destroy(&buf);
if (rc > 0)
--
2.52.0
^ permalink raw reply related
* [PATCH v7 05/11] KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Use tpm_ret_to_err() to transmute TPM return codes in trusted_tpm2.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
---
v6:
- No changes.
v5:
- No changes.
v4:
- No changes.
v3:
- No changes.
v2:
- New patch split out from the fix.
---
security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 00bc1afb32c8..c9973ac46041 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -331,25 +331,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);
}
/**
@@ -453,10 +447,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
out:
tpm_buf_destroy(&buf);
- if (rc > 0)
- rc = -EPERM;
-
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -519,8 +510,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(
@@ -553,7 +542,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
out:
tpm_buf_destroy(&buf);
- return rc;
+ return tpm_ret_to_err(rc);
}
/**
@@ -585,6 +574,5 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
out:
tpm_put_ops(chip);
-
- return rc;
+ return tpm_ret_to_err(rc);
}
--
2.52.0
^ permalink raw reply related
* [PATCH v7 07/11] tpm2-sessions: Unmask tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, Jonathan McDowell, Peter Huewe, Jarkko Sakkinen,
Jason Gunthorpe, James Bottomley, Mimi Zohar, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Open code tpm_buf_append_hmac_session_opt() in order to unmask the code
paths in the call sites of tpm_buf_append_hmac_session().
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
---
v6:
- No changes.
v5:
- No changes.
v4:
- Fixed typo in short summary.
v3:
- No changes.
v2:
- Uncorrupt the patch.
---
drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
include/linux/tpm.h | 23 -----------------------
security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
3 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ceffa3bd2b5c..e9ee6ff32aea 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -272,9 +272,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 5acf6422e2b9..dc38fc1870f8 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -532,29 +532,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
int passphraselen);
void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
u8 *passphrase, int passphraselen);
-static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
- struct tpm_buf *buf,
- u8 attributes,
- u8 *passphrase,
- int passphraselen)
-{
- struct tpm_header *head;
- int offset;
-
- if (tpm2_chip_auth(chip)) {
- tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
- } else {
- offset = buf->handles * 4 + TPM_HEADER_SIZE;
- head = (struct tpm_header *)buf->data;
-
- /*
- * If the only sessions are optional, the command tag must change to
- * TPM2_ST_NO_SESSIONS.
- */
- if (tpm_buf_length(buf) == offset)
- head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
- }
-}
#ifdef CONFIG_TCG_TPM2_HMAC
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c9973ac46041..367bcfb59c4d 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -467,8 +467,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;
@@ -503,8 +505,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.52.0
^ permalink raw reply related
* [PATCH v7 08/11] KEYS: trusted: Open code tpm2_buf_append()
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
tpm2_buf_append_auth() has only single call site and most of its parameters
are redundant. Open code it to the call site. Remove illegit FIXME comment
as there is no categorized bug and replace it with more sane comment about
implementation (i.e. "non-opionated inline comment").
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@earth.li>
---
v6:
- No changes.
v5:
- No changes.
v4:
- No changes.
v3:
- No changes.
v2:
- No changes.
---
security/keys/trusted-keys/trusted_tpm2.c | 51 ++++-------------------
1 file changed, 9 insertions(+), 42 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 367bcfb59c4d..b1e2e1542ba2 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -188,36 +188,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
*
@@ -492,19 +462,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.52.0
^ permalink raw reply related
* [PATCH v7 09/11] tpm-buf: unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW
From: Jarkko Sakkinen @ 2025-11-27 13:54 UTC (permalink / raw)
To: linux-integrity
Cc: ross.philipson, Jonathan McDowell, Stefano Garzarella,
Jarkko Sakkinen, Jonathan McDowell, Peter Huewe, Jarkko Sakkinen,
Jason Gunthorpe, James Bottomley, Mimi Zohar, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
In-Reply-To: <20251127135445.2141241-1-jarkko@kernel.org>
From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW into TPM_BUF_INVALID,
given that they are identical. The only difference are the log messages.
In addition, add a missing TPM_BUF_INVALID check to tpm_buf_append_handle()
following the pattern from other functions in tpm-buf.c.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
---
v6:
- Change short summary to something more descriptive.
v5:
- No changes.
v4:
- No changes.
v3:
- No changes.
v2:
- A new patch.
---
drivers/char/tpm/tpm-buf.c | 14 ++++++++------
include/linux/tpm.h | 8 +++-----
security/keys/trusted-keys/trusted_tpm2.c | 6 +++---
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index dc882fc9fa9e..69ee77400539 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -104,13 +104,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
*/
void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
{
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
+ if (buf->flags & TPM_BUF_INVALID)
return;
if ((buf->length + new_length) > PAGE_SIZE) {
WARN(1, "tpm_buf: write overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
@@ -157,8 +156,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_INVALID)
+ return;
+
if (buf->flags & TPM_BUF_TPM2B) {
dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+ buf->flags |= TPM_BUF_INVALID;
return;
}
@@ -177,14 +180,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
{
off_t next_offset;
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
+ if (buf->flags & TPM_BUF_INVALID)
return;
next_offset = *offset + count;
if (next_offset > buf->length) {
WARN(1, "tpm_buf: read out of boundary\n");
- buf->flags |= TPM_BUF_BOUNDARY_ERROR;
+ buf->flags |= TPM_BUF_INVALID;
return;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index dc38fc1870f8..f3b7201863f2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -369,12 +369,10 @@ struct tpm_header {
} __packed;
enum tpm_buf_flags {
- /* the capacity exceeded: */
- TPM_BUF_OVERFLOW = BIT(0),
/* TPM2B format: */
- TPM_BUF_TPM2B = BIT(1),
- /* read out of boundary: */
- TPM_BUF_BOUNDARY_ERROR = BIT(2),
+ TPM_BUF_TPM2B = BIT(0),
+ /* The buffer is in invalid and unusable state: */
+ TPM_BUF_INVALID = BIT(1),
};
/*
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index b1e2e1542ba2..966166694e7f 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -278,7 +278,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
/* creation PCR */
tpm_buf_append_u32(&buf, 0);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
@@ -291,7 +291,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
goto out;
blob_len = tpm_buf_read_u32(&buf, &offset);
- if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
+ if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
goto out;
}
@@ -401,7 +401,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
tpm_buf_append(&buf, blob, blob_len);
- if (buf.flags & TPM_BUF_OVERFLOW) {
+ if (buf.flags & TPM_BUF_INVALID) {
rc = -E2BIG;
tpm2_end_auth_session(chip);
goto out;
--
2.52.0
^ permalink raw reply related
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