* 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: [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 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: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 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 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 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 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
* [PATCH bpf-next 3/3] selftests/bpf: Add tests for bpf_kern_path kfunc
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Add comprehensive selftests for the new bpf_kern_path and bpf_path_put
kfuncs:
1. Functional tests (prog_tests/kern_path.c, progs/test_kern_path.c):
- test_kern_path_basic: Tests successful path resolution using
/proc/self/exe and validates the resolved path with bpf_path_d_path
- test_kern_path_sb_mount: Tests bpf_kern_path with dynamic input
from LSM hook parameter (dev_name from sb_mount), demonstrating
real-world usage where BPF programs resolve paths from hook args
2. Verifier success tests (progs/verifier_kern_path.c):
- kern_path_success: Proper acquire -> use -> release pattern
- kern_path_multiple_paths: Multiple concurrent path acquisitions
3. Verifier failure tests (progs/verifier_kern_path_fail.c):
- kern_path_unreleased: Resource leak detection
- path_put_unacquired: Releasing unacquired path
- path_use_after_put: Use-after-free detection
- double_path_put: Double-free detection
- kern_path_non_lsm: Program type restrictions (LSM only)
- kern_path_non_const_str: reject none const string
These tests verify both the functionality of the kfuncs and that the
verifier properly enforces acquire/release semantics to prevent
resource leaks.
Signed-off-by: Song Liu <song@kernel.org>
---
.../testing/selftests/bpf/bpf_experimental.h | 4 +
.../selftests/bpf/prog_tests/kern_path.c | 82 ++++++++++++++++
.../selftests/bpf/progs/test_kern_path.c | 56 +++++++++++
.../selftests/bpf/progs/verifier_kern_path.c | 52 ++++++++++
.../bpf/progs/verifier_kern_path_fail.c | 97 +++++++++++++++++++
5 files changed, 291 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 2cd9165c7348..c512c9a14752 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -221,6 +221,10 @@ extern void bpf_put_file(struct file *file) __ksym;
*/
extern int bpf_path_d_path(const struct path *path, char *buf, size_t buf__sz) __ksym;
+extern struct path *bpf_kern_path(const char *pathname, unsigned int flags) __ksym;
+extern void bpf_path_put(struct path *path) __ksym;
+extern int bpf_path_d_path(const struct path *path, char *buf, size_t buf__sz) __ksym;
+
/* This macro must be used to mark the exception callback corresponding to the
* main program. For example:
*
diff --git a/tools/testing/selftests/bpf/prog_tests/kern_path.c b/tools/testing/selftests/bpf/prog_tests/kern_path.c
new file mode 100644
index 000000000000..f4cdfe202a26
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kern_path.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <test_progs.h>
+#include <sys/mount.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "test_kern_path.skel.h"
+#include "verifier_kern_path.skel.h"
+#include "verifier_kern_path_fail.skel.h"
+
+static void __test_kern_path(void (*trigger)(void))
+{
+ struct test_kern_path *skel;
+ int err;
+
+ skel = test_kern_path__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_kern_path__open_and_load"))
+ return;
+
+ skel->bss->monitored_pid = getpid();
+
+ err = test_kern_path__attach(skel);
+ if (!ASSERT_OK(err, "test_kern_path__attach"))
+ goto cleanup;
+
+ trigger();
+
+ /* Verify the bpf_path_d_path worked */
+ ASSERT_GT(skel->bss->path_len, 0, "path_len > 0");
+
+cleanup:
+ test_kern_path__destroy(skel);
+}
+
+static void trigger_file_open(void)
+{
+ int fd;
+
+ fd = open("/dev/null", O_RDONLY);
+ if (!ASSERT_OK_FD(fd, "open /dev/null"))
+ return;
+ close(fd);
+}
+
+static void trigger_sb_mount(void)
+{
+ char tmpdir[] = "/tmp/bpf_kern_path_test_XXXXXX";
+ int err;
+
+ if (!ASSERT_OK_PTR(mkdtemp(tmpdir), "mkdtemp"))
+ return;
+
+ err = mount("/tmp", tmpdir, NULL, MS_BIND, NULL);
+ if (!ASSERT_OK(err, "bind mount"))
+ goto rmdir;
+
+ umount(tmpdir);
+rmdir:
+ rmdir(tmpdir);
+}
+
+void test_kern_path(void)
+{
+ if (test__start_subtest("file_open"))
+ __test_kern_path(trigger_file_open);
+
+ if (test__start_subtest("sb_mount"))
+ __test_kern_path(trigger_sb_mount);
+}
+
+void test_verifier_kern_path(void)
+{
+ RUN_TESTS(verifier_kern_path);
+}
+
+void test_verifier_kern_path_fail(void)
+{
+ RUN_TESTS(verifier_kern_path_fail);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_kern_path.c b/tools/testing/selftests/bpf/progs/test_kern_path.c
new file mode 100644
index 000000000000..e9186a1aa990
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_kern_path.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+#define MAX_PATH_LEN 256
+
+char buf[MAX_PATH_LEN];
+int path_len = 0;
+u32 monitored_pid = 0;
+
+SEC("lsm.s/file_open")
+int BPF_PROG(test_kern_path_basic, struct file *file)
+{
+ struct path *p;
+ int ret;
+
+ if (bpf_get_current_pid_tgid() >> 32 != monitored_pid)
+ return 0;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (p) {
+ ret = bpf_path_d_path(p, buf, MAX_PATH_LEN);
+ if (ret > 0)
+ path_len = ret;
+ bpf_path_put(p);
+ }
+
+ return 0;
+}
+
+SEC("lsm.s/sb_mount")
+int BPF_PROG(test_kern_path_from_sb_mount, const char *dev_name, const struct path *path,
+ const char *type, unsigned long flags, void *data)
+{
+ struct path *p;
+ int ret;
+
+ if (bpf_get_current_pid_tgid() >> 32 != monitored_pid)
+ return 0;
+
+ p = bpf_kern_path(dev_name, 0);
+ if (p) {
+ ret = bpf_path_d_path(p, buf, MAX_PATH_LEN);
+ if (ret > 0)
+ path_len = ret;
+ bpf_path_put(p);
+ }
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_kern_path.c b/tools/testing/selftests/bpf/progs/verifier_kern_path.c
new file mode 100644
index 000000000000..0e6ccf640b64
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_kern_path.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/limits.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+static char buf[PATH_MAX];
+
+SEC("lsm.s/file_open")
+__success
+int BPF_PROG(kern_path_success)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_d_path(p, buf, sizeof(buf));
+
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__success
+int BPF_PROG(kern_path_multiple_paths)
+{
+ struct path *p1, *p2;
+
+ p1 = bpf_kern_path("/proc/self/exe", 0);
+ if (!p1)
+ return 0;
+
+ p2 = bpf_kern_path("/proc/self/cwd", 0);
+ if (!p2) {
+ bpf_path_put(p1);
+ return 0;
+ }
+
+ bpf_path_d_path(p1, buf, sizeof(buf));
+ bpf_path_d_path(p2, buf, sizeof(buf));
+
+ bpf_path_put(p2);
+ bpf_path_put(p1);
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c b/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
new file mode 100644
index 000000000000..520c227af5ca
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/limits.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+static char buf[PATH_MAX];
+
+SEC("lsm.s/file_open")
+__failure __msg("Unreleased reference")
+int BPF_PROG(kern_path_unreleased)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ /* Acquired but never released - should fail verification */
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(path_put_unacquired)
+{
+ struct path p = {};
+
+ /* Can't release an unacquired path - should fail verification */
+ bpf_path_put(&p);
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(path_use_after_put, struct file *file)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_put(p);
+
+ /* Using path after put - should fail verification */
+ bpf_path_d_path(p, buf, sizeof(buf));
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(double_path_put)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_put(p);
+ /* Double put - should fail verification */
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("fentry/vfs_open")
+__failure __msg("calling kernel function bpf_kern_path is not allowed")
+int BPF_PROG(kern_path_non_lsm)
+{
+ struct path *p;
+
+ /* Calling bpf_kern_path() from a non-LSM BPF program isn't permitted */
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (p)
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("lsm.s/sb_eat_lsm_opts")
+__failure __msg("arg#0 doesn't point to a const string")
+int BPF_PROG(kern_path_non_const_str, char *options, void **mnt_opts)
+{
+ struct path *p;
+
+ /* Calling bpf_kern_path() from a with non-const string isn't permitted */
+ p = bpf_kern_path(options, 0);
+ if (p)
+ bpf_path_put(p);
+ return 0;
+}
+
+
+char _license[] SEC("license") = "GPL";
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
by BPF LSM programs:
bpf_kern_path():
- Resolves a pathname string to a struct path
- Allocates memory for the path structure
- Returns NULL on error or if the path doesn't exist
- Marked with KF_ACQUIRE | KF_SLEEPABLE | KF_RET_NULL
bpf_path_put():
- Releases the path reference and frees the allocated memory
- Marked with KF_RELEASE to enforce acquire/release semantics
These kfuncs enable BPF LSM programs to resolve pathnames provided by
hook arguments (e.g., dev_name from sb_mount) and validate or inspect
the resolved paths. The verifier enforces proper resource management
through acquire/release tracking.
Example usage:
struct path *p = bpf_kern_path("/etc/passwd", LOOKUP_FOLLOW);
if (p) {
// Use the path...
bpf_path_put(p); // Must release
}
Signed-off-by: Song Liu <song@kernel.org>
---
fs/bpf_fs_kfuncs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 5ace2511fec5..977f8dcbc208 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -11,6 +11,7 @@
#include <linux/file.h>
#include <linux/kernfs.h>
#include <linux/mm.h>
+#include <linux/namei.h>
#include <linux/xattr.h>
__bpf_kfunc_start_defs();
@@ -96,6 +97,61 @@ __bpf_kfunc int bpf_path_d_path(const struct path *path, char *buf, size_t buf__
return len;
}
+/**
+ * bpf_kern_path - resolve a pathname to a struct path
+ * @pathname__str: pathname to resolve
+ * @flags: lookup flags (e.g., LOOKUP_FOLLOW)
+ *
+ * Resolve the pathname for the supplied *pathname__str* and return a pointer
+ * to a struct path. This is a wrapper around kern_path() that allocates and
+ * returns a struct path pointer on success.
+ *
+ * The returned struct path pointer must be released using bpf_path_put().
+ * Failing to call bpf_path_put() on the returned struct path pointer will
+ * result in the BPF program being rejected by the BPF verifier.
+ *
+ * This BPF kfunc may only be called from BPF LSM programs.
+ *
+ * Return: A pointer to an allocated struct path on success, NULL on error.
+ */
+__bpf_kfunc struct path *bpf_kern_path(const char *pathname__str, unsigned int flags)
+{
+ struct path *path;
+ int ret;
+
+ path = kmalloc(sizeof(*path), GFP_KERNEL);
+ if (!path)
+ return NULL;
+
+ ret = kern_path(pathname__str, flags, path);
+ if (ret) {
+ kfree(path);
+ return NULL;
+ }
+
+ return path;
+}
+
+/**
+ * bpf_path_put - release a struct path reference
+ * @path: struct path pointer to release
+ *
+ * Release the struct path pointer that was acquired by bpf_kern_path().
+ * This BPF kfunc calls path_put() on the supplied *path* and then frees
+ * the allocated memory.
+ *
+ * Only struct path pointers acquired by bpf_kern_path() may be passed to
+ * this BPF kfunc. Attempting to pass any other pointer will result in the
+ * BPF program being rejected by the BPF verifier.
+ *
+ * This BPF kfunc may only be called from BPF LSM programs.
+ */
+__bpf_kfunc void bpf_path_put(struct path *path)
+{
+ path_put(path);
+ kfree(path);
+}
+
static bool match_security_bpf_prefix(const char *name__str)
{
return !strncmp(name__str, XATTR_NAME_BPF_LSM, XATTR_NAME_BPF_LSM_LEN);
@@ -363,6 +419,8 @@ BTF_ID_FLAGS(func, bpf_get_task_exe_file,
KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_path_d_path, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kern_path, KF_TRUSTED_ARGS | KF_ACQUIRE | KF_SLEEPABLE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_path_put, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 1/3] bpf: Allow const char * from LSM hooks as kfunc const string arguments
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Let the BPF verifier to recognize const char * arguments from LSM hooks
(and other BPF program types) as valid const string pointers that can be
passed to kfuncs expecting KF_ARG_PTR_TO_CONST_STR.
Previously, kfuncs with KF_ARG_PTR_TO_CONST_STR only accepted
PTR_TO_MAP_VALUE from readonly maps. This was limiting for LSM programs
that receive const char * arguments from hooks like sb_mount's dev_name.
Signed-off-by: Song Liu <song@kernel.org>
---
include/linux/btf.h | 1 +
kernel/bpf/btf.c | 33 ++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 51 +++++++++++++++++++++++++++++++++----------
3 files changed, 73 insertions(+), 12 deletions(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index f06976ffb63f..bd5a32d33254 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -224,6 +224,7 @@ struct btf *btf_base_btf(const struct btf *btf);
bool btf_type_is_i32(const struct btf_type *t);
bool btf_type_is_i64(const struct btf_type *t);
bool btf_type_is_primitive(const struct btf_type *t);
+bool btf_type_is_const_char_ptr(const struct btf *btf, const struct btf_type *t);
bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
const struct btf_member *m,
u32 expected_offset, u32 expected_size);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 0de8fc8a0e0b..94a272585b97 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -897,6 +897,25 @@ bool btf_type_is_primitive(const struct btf_type *t)
btf_is_any_enum(t);
}
+bool btf_type_is_const_char_ptr(const struct btf *btf, const struct btf_type *t)
+{
+ const char *tname;
+
+ /* The type chain has to be PTR->CONST->CHAR */
+ if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR)
+ return false;
+
+ t = btf_type_by_id(btf, t->type);
+ if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)
+ return false;
+
+ t = btf_type_by_id(btf, t->type);
+ tname = btf_name_by_offset(btf, t->name_off);
+ if (tname && strcmp(tname, "char") == 0)
+ return true;
+ return false;
+}
+
/*
* Check that given struct member is a regular int with expected
* offset and size.
@@ -6746,6 +6765,20 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
/* Default prog with MAX_BPF_FUNC_REG_ARGS args */
return true;
t = btf_type_by_id(btf, args[arg].type);
+
+ /*
+ * For const string, we need to match "const char *"
+ * exactly. Therefore, do the check before the skipping
+ * modifiers.
+ */
+ if (btf_type_is_const_char_ptr(btf, t)) {
+ info->reg_type = PTR_TO_BTF_ID;
+ if (prog_args_trusted(prog))
+ info->reg_type |= PTR_TRUSTED;
+ info->btf = btf;
+ info->btf_id = args[arg].type;
+ return true;
+ }
}
/* skip modifiers */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 766695491bc5..a9757c056d4b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9598,8 +9598,12 @@ static enum bpf_dynptr_type dynptr_get_type(struct bpf_verifier_env *env,
return state->stack[spi].spilled_ptr.dynptr.type;
}
-static int check_reg_const_str(struct bpf_verifier_env *env,
- struct bpf_reg_state *reg, u32 regno)
+/*
+ * Check for const string saved in a bpf map. The caller is responsible
+ * to check reg->type == PTR_TO_MAP_VALUE.
+ */
+static int check_reg_const_str_in_map(struct bpf_verifier_env *env,
+ struct bpf_reg_state *reg, u32 regno)
{
struct bpf_map *map = reg->map_ptr;
int err;
@@ -9607,9 +9611,6 @@ static int check_reg_const_str(struct bpf_verifier_env *env,
u64 map_addr;
char *str_ptr;
- if (reg->type != PTR_TO_MAP_VALUE)
- return -EINVAL;
-
if (!bpf_map_is_rdonly(map)) {
verbose(env, "R%d does not point to a readonly map'\n", regno);
return -EACCES;
@@ -9646,6 +9647,26 @@ static int check_reg_const_str(struct bpf_verifier_env *env,
return 0;
}
+/* Check for const string passed in as input to the bpf program. */
+static int check_reg_const_str_arg(struct bpf_reg_state *reg)
+{
+ const struct btf *btf;
+ const struct btf_type *t;
+ const char *tname;
+
+ if (base_type(reg->type) != PTR_TO_BTF_ID)
+ return -EINVAL;
+
+ btf = reg->btf;
+ t = btf_type_by_id(btf, reg->btf_id);
+ if (!t)
+ return -EINVAL;
+
+ if (btf_type_is_const_char_ptr(btf, t))
+ return 0;
+ return -EINVAL;
+}
+
/* Returns constant key value in `value` if possible, else negative error */
static int get_constant_map_key(struct bpf_verifier_env *env,
struct bpf_reg_state *key,
@@ -9964,7 +9985,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
break;
case ARG_PTR_TO_CONST_STR:
{
- err = check_reg_const_str(env, reg, regno);
+ if (reg->type != PTR_TO_MAP_VALUE)
+ return -EINVAL;
+ err = check_reg_const_str_in_map(env, reg, regno);
if (err)
return err;
break;
@@ -13626,13 +13649,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
meta->arg_btf_id = reg->btf_id;
break;
case KF_ARG_PTR_TO_CONST_STR:
- if (reg->type != PTR_TO_MAP_VALUE) {
- verbose(env, "arg#%d doesn't point to a const string\n", i);
- return -EINVAL;
+ if (reg->type == PTR_TO_MAP_VALUE) {
+ ret = check_reg_const_str_in_map(env, reg, regno);
+ if (ret)
+ return ret;
+ } else {
+ ret = check_reg_const_str_arg(reg);
+ if (ret) {
+ verbose(env, "arg#%d doesn't point to a const string\n", i);
+ return ret;
+ }
}
- ret = check_reg_const_str(env, reg, regno);
- if (ret)
- return ret;
break;
case KF_ARG_PTR_TO_WORKQUEUE:
if (reg->type != PTR_TO_MAP_VALUE) {
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 0/3] Introduce bpf_kern_path and bpf_path_put
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Security solutions use LSM hook security_sb_mount to monitor mount
operations. security_sb_mount takes dev_name as a string. To get a struct
path from dev_name, in-tree LSMs use kern_path. Introduce kfuncs
bpf_kern_path so that bpf LSM can do similar operations. bpf_kern_path
takes a reference on the return value path. Also add kfunc bpf_path_put to
release path returned by bpf_kern_path. Note that, bpf_kern_path only holds
reference on the path during the duration of this bpf program. The verifier
enforces the bpf program release this reference.
Patch 1/3 prepares bpf verifier to handle const char * passed in as hook
argument. Before this change, bpf helpers and kfuncs only consider value
from read only map as const string.
Patch 2/3 adds the two kfuncs.
Patch 3/3 add tests for the new kfuncs.
Song Liu (3):
bpf: Allow const char * from LSM hooks as kfunc const string arguments
bpf: Add bpf_kern_path and bpf_path_put kfuncs
selftests/bpf: Add tests for bpf_kern_path kfunc
fs/bpf_fs_kfuncs.c | 58 +++++++++++
include/linux/btf.h | 1 +
kernel/bpf/btf.c | 33 +++++++
kernel/bpf/verifier.c | 51 +++++++---
.../testing/selftests/bpf/bpf_experimental.h | 4 +
.../selftests/bpf/prog_tests/kern_path.c | 82 ++++++++++++++++
.../selftests/bpf/progs/test_kern_path.c | 56 +++++++++++
.../selftests/bpf/progs/verifier_kern_path.c | 52 ++++++++++
.../bpf/progs/verifier_kern_path_fail.c | 97 +++++++++++++++++++
9 files changed, 422 insertions(+), 12 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
--
2.47.3
^ permalink raw reply
* [PATCH bpf-next 3/3] selftests/bpf: Add tests for bpf_kern_path kfunc
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Add comprehensive selftests for the new bpf_kern_path and bpf_path_put
kfuncs:
1. Functional tests (prog_tests/kern_path.c, progs/test_kern_path.c):
- test_kern_path_basic: Tests successful path resolution using
/proc/self/exe and validates the resolved path with bpf_path_d_path
- test_kern_path_sb_mount: Tests bpf_kern_path with dynamic input
from LSM hook parameter (dev_name from sb_mount), demonstrating
real-world usage where BPF programs resolve paths from hook args
2. Verifier success tests (progs/verifier_kern_path.c):
- kern_path_success: Proper acquire -> use -> release pattern
- kern_path_multiple_paths: Multiple concurrent path acquisitions
3. Verifier failure tests (progs/verifier_kern_path_fail.c):
- kern_path_unreleased: Resource leak detection
- path_put_unacquired: Releasing unacquired path
- path_use_after_put: Use-after-free detection
- double_path_put: Double-free detection
- kern_path_non_lsm: Program type restrictions (LSM only)
- kern_path_non_const_str: reject none const string
These tests verify both the functionality of the kfuncs and that the
verifier properly enforces acquire/release semantics to prevent
resource leaks.
Signed-off-by: Song Liu <song@kernel.org>
---
.../testing/selftests/bpf/bpf_experimental.h | 4 +
.../selftests/bpf/prog_tests/kern_path.c | 82 ++++++++++++++++
.../selftests/bpf/progs/test_kern_path.c | 56 +++++++++++
.../selftests/bpf/progs/verifier_kern_path.c | 52 ++++++++++
.../bpf/progs/verifier_kern_path_fail.c | 97 +++++++++++++++++++
5 files changed, 291 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 2cd9165c7348..c512c9a14752 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -221,6 +221,10 @@ extern void bpf_put_file(struct file *file) __ksym;
*/
extern int bpf_path_d_path(const struct path *path, char *buf, size_t buf__sz) __ksym;
+extern struct path *bpf_kern_path(const char *pathname, unsigned int flags) __ksym;
+extern void bpf_path_put(struct path *path) __ksym;
+extern int bpf_path_d_path(const struct path *path, char *buf, size_t buf__sz) __ksym;
+
/* This macro must be used to mark the exception callback corresponding to the
* main program. For example:
*
diff --git a/tools/testing/selftests/bpf/prog_tests/kern_path.c b/tools/testing/selftests/bpf/prog_tests/kern_path.c
new file mode 100644
index 000000000000..f4cdfe202a26
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kern_path.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <test_progs.h>
+#include <sys/mount.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "test_kern_path.skel.h"
+#include "verifier_kern_path.skel.h"
+#include "verifier_kern_path_fail.skel.h"
+
+static void __test_kern_path(void (*trigger)(void))
+{
+ struct test_kern_path *skel;
+ int err;
+
+ skel = test_kern_path__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_kern_path__open_and_load"))
+ return;
+
+ skel->bss->monitored_pid = getpid();
+
+ err = test_kern_path__attach(skel);
+ if (!ASSERT_OK(err, "test_kern_path__attach"))
+ goto cleanup;
+
+ trigger();
+
+ /* Verify the bpf_path_d_path worked */
+ ASSERT_GT(skel->bss->path_len, 0, "path_len > 0");
+
+cleanup:
+ test_kern_path__destroy(skel);
+}
+
+static void trigger_file_open(void)
+{
+ int fd;
+
+ fd = open("/dev/null", O_RDONLY);
+ if (!ASSERT_OK_FD(fd, "open /dev/null"))
+ return;
+ close(fd);
+}
+
+static void trigger_sb_mount(void)
+{
+ char tmpdir[] = "/tmp/bpf_kern_path_test_XXXXXX";
+ int err;
+
+ if (!ASSERT_OK_PTR(mkdtemp(tmpdir), "mkdtemp"))
+ return;
+
+ err = mount("/tmp", tmpdir, NULL, MS_BIND, NULL);
+ if (!ASSERT_OK(err, "bind mount"))
+ goto rmdir;
+
+ umount(tmpdir);
+rmdir:
+ rmdir(tmpdir);
+}
+
+void test_kern_path(void)
+{
+ if (test__start_subtest("file_open"))
+ __test_kern_path(trigger_file_open);
+
+ if (test__start_subtest("sb_mount"))
+ __test_kern_path(trigger_sb_mount);
+}
+
+void test_verifier_kern_path(void)
+{
+ RUN_TESTS(verifier_kern_path);
+}
+
+void test_verifier_kern_path_fail(void)
+{
+ RUN_TESTS(verifier_kern_path_fail);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_kern_path.c b/tools/testing/selftests/bpf/progs/test_kern_path.c
new file mode 100644
index 000000000000..e9186a1aa990
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_kern_path.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+#define MAX_PATH_LEN 256
+
+char buf[MAX_PATH_LEN];
+int path_len = 0;
+u32 monitored_pid = 0;
+
+SEC("lsm.s/file_open")
+int BPF_PROG(test_kern_path_basic, struct file *file)
+{
+ struct path *p;
+ int ret;
+
+ if (bpf_get_current_pid_tgid() >> 32 != monitored_pid)
+ return 0;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (p) {
+ ret = bpf_path_d_path(p, buf, MAX_PATH_LEN);
+ if (ret > 0)
+ path_len = ret;
+ bpf_path_put(p);
+ }
+
+ return 0;
+}
+
+SEC("lsm.s/sb_mount")
+int BPF_PROG(test_kern_path_from_sb_mount, const char *dev_name, const struct path *path,
+ const char *type, unsigned long flags, void *data)
+{
+ struct path *p;
+ int ret;
+
+ if (bpf_get_current_pid_tgid() >> 32 != monitored_pid)
+ return 0;
+
+ p = bpf_kern_path(dev_name, 0);
+ if (p) {
+ ret = bpf_path_d_path(p, buf, MAX_PATH_LEN);
+ if (ret > 0)
+ path_len = ret;
+ bpf_path_put(p);
+ }
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_kern_path.c b/tools/testing/selftests/bpf/progs/verifier_kern_path.c
new file mode 100644
index 000000000000..0e6ccf640b64
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_kern_path.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/limits.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+static char buf[PATH_MAX];
+
+SEC("lsm.s/file_open")
+__success
+int BPF_PROG(kern_path_success)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_d_path(p, buf, sizeof(buf));
+
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__success
+int BPF_PROG(kern_path_multiple_paths)
+{
+ struct path *p1, *p2;
+
+ p1 = bpf_kern_path("/proc/self/exe", 0);
+ if (!p1)
+ return 0;
+
+ p2 = bpf_kern_path("/proc/self/cwd", 0);
+ if (!p2) {
+ bpf_path_put(p1);
+ return 0;
+ }
+
+ bpf_path_d_path(p1, buf, sizeof(buf));
+ bpf_path_d_path(p2, buf, sizeof(buf));
+
+ bpf_path_put(p2);
+ bpf_path_put(p1);
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c b/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
new file mode 100644
index 000000000000..520c227af5ca
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/limits.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+static char buf[PATH_MAX];
+
+SEC("lsm.s/file_open")
+__failure __msg("Unreleased reference")
+int BPF_PROG(kern_path_unreleased)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ /* Acquired but never released - should fail verification */
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(path_put_unacquired)
+{
+ struct path p = {};
+
+ /* Can't release an unacquired path - should fail verification */
+ bpf_path_put(&p);
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(path_use_after_put, struct file *file)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_put(p);
+
+ /* Using path after put - should fail verification */
+ bpf_path_d_path(p, buf, sizeof(buf));
+ return 0;
+}
+
+SEC("lsm.s/file_open")
+__failure __msg("pointer type STRUCT path must point to scalar, or struct with scalar")
+int BPF_PROG(double_path_put)
+{
+ struct path *p;
+
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (!p)
+ return 0;
+
+ bpf_path_put(p);
+ /* Double put - should fail verification */
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("fentry/vfs_open")
+__failure __msg("calling kernel function bpf_kern_path is not allowed")
+int BPF_PROG(kern_path_non_lsm)
+{
+ struct path *p;
+
+ /* Calling bpf_kern_path() from a non-LSM BPF program isn't permitted */
+ p = bpf_kern_path("/proc/self/exe", 0);
+ if (p)
+ bpf_path_put(p);
+ return 0;
+}
+
+SEC("lsm.s/sb_eat_lsm_opts")
+__failure __msg("arg#0 doesn't point to a const string")
+int BPF_PROG(kern_path_non_const_str, char *options, void **mnt_opts)
+{
+ struct path *p;
+
+ /* Calling bpf_kern_path() from a with non-const string isn't permitted */
+ p = bpf_kern_path(options, 0);
+ if (p)
+ bpf_path_put(p);
+ return 0;
+}
+
+
+char _license[] SEC("license") = "GPL";
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 2/3] bpf: Add bpf_kern_path and bpf_path_put kfuncs
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Add two new kfuncs to fs/bpf_fs_kfuncs.c that wrap kern_path() for use
by BPF LSM programs:
bpf_kern_path():
- Resolves a pathname string to a struct path
- Allocates memory for the path structure
- Returns NULL on error or if the path doesn't exist
- Marked with KF_ACQUIRE | KF_SLEEPABLE | KF_RET_NULL
bpf_path_put():
- Releases the path reference and frees the allocated memory
- Marked with KF_RELEASE to enforce acquire/release semantics
These kfuncs enable BPF LSM programs to resolve pathnames provided by
hook arguments (e.g., dev_name from sb_mount) and validate or inspect
the resolved paths. The verifier enforces proper resource management
through acquire/release tracking.
Example usage:
struct path *p = bpf_kern_path("/etc/passwd", LOOKUP_FOLLOW);
if (p) {
// Use the path...
bpf_path_put(p); // Must release
}
Signed-off-by: Song Liu <song@kernel.org>
---
fs/bpf_fs_kfuncs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 5ace2511fec5..977f8dcbc208 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -11,6 +11,7 @@
#include <linux/file.h>
#include <linux/kernfs.h>
#include <linux/mm.h>
+#include <linux/namei.h>
#include <linux/xattr.h>
__bpf_kfunc_start_defs();
@@ -96,6 +97,61 @@ __bpf_kfunc int bpf_path_d_path(const struct path *path, char *buf, size_t buf__
return len;
}
+/**
+ * bpf_kern_path - resolve a pathname to a struct path
+ * @pathname__str: pathname to resolve
+ * @flags: lookup flags (e.g., LOOKUP_FOLLOW)
+ *
+ * Resolve the pathname for the supplied *pathname__str* and return a pointer
+ * to a struct path. This is a wrapper around kern_path() that allocates and
+ * returns a struct path pointer on success.
+ *
+ * The returned struct path pointer must be released using bpf_path_put().
+ * Failing to call bpf_path_put() on the returned struct path pointer will
+ * result in the BPF program being rejected by the BPF verifier.
+ *
+ * This BPF kfunc may only be called from BPF LSM programs.
+ *
+ * Return: A pointer to an allocated struct path on success, NULL on error.
+ */
+__bpf_kfunc struct path *bpf_kern_path(const char *pathname__str, unsigned int flags)
+{
+ struct path *path;
+ int ret;
+
+ path = kmalloc(sizeof(*path), GFP_KERNEL);
+ if (!path)
+ return NULL;
+
+ ret = kern_path(pathname__str, flags, path);
+ if (ret) {
+ kfree(path);
+ return NULL;
+ }
+
+ return path;
+}
+
+/**
+ * bpf_path_put - release a struct path reference
+ * @path: struct path pointer to release
+ *
+ * Release the struct path pointer that was acquired by bpf_kern_path().
+ * This BPF kfunc calls path_put() on the supplied *path* and then frees
+ * the allocated memory.
+ *
+ * Only struct path pointers acquired by bpf_kern_path() may be passed to
+ * this BPF kfunc. Attempting to pass any other pointer will result in the
+ * BPF program being rejected by the BPF verifier.
+ *
+ * This BPF kfunc may only be called from BPF LSM programs.
+ */
+__bpf_kfunc void bpf_path_put(struct path *path)
+{
+ path_put(path);
+ kfree(path);
+}
+
static bool match_security_bpf_prefix(const char *name__str)
{
return !strncmp(name__str, XATTR_NAME_BPF_LSM, XATTR_NAME_BPF_LSM_LEN);
@@ -363,6 +419,8 @@ BTF_ID_FLAGS(func, bpf_get_task_exe_file,
KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_path_d_path, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kern_path, KF_TRUSTED_ARGS | KF_ACQUIRE | KF_SLEEPABLE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_path_put, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 1/3] bpf: Allow const char * from LSM hooks as kfunc const string arguments
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
In-Reply-To: <20251127005011.1872209-1-song@kernel.org>
Let the BPF verifier to recognize const char * arguments from LSM hooks
(and other BPF program types) as valid const string pointers that can be
passed to kfuncs expecting KF_ARG_PTR_TO_CONST_STR.
Previously, kfuncs with KF_ARG_PTR_TO_CONST_STR only accepted
PTR_TO_MAP_VALUE from readonly maps. This was limiting for LSM programs
that receive const char * arguments from hooks like sb_mount's dev_name.
Signed-off-by: Song Liu <song@kernel.org>
---
include/linux/btf.h | 1 +
kernel/bpf/btf.c | 33 ++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 51 +++++++++++++++++++++++++++++++++----------
3 files changed, 73 insertions(+), 12 deletions(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index f06976ffb63f..bd5a32d33254 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -224,6 +224,7 @@ struct btf *btf_base_btf(const struct btf *btf);
bool btf_type_is_i32(const struct btf_type *t);
bool btf_type_is_i64(const struct btf_type *t);
bool btf_type_is_primitive(const struct btf_type *t);
+bool btf_type_is_const_char_ptr(const struct btf *btf, const struct btf_type *t);
bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
const struct btf_member *m,
u32 expected_offset, u32 expected_size);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 0de8fc8a0e0b..94a272585b97 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -897,6 +897,25 @@ bool btf_type_is_primitive(const struct btf_type *t)
btf_is_any_enum(t);
}
+bool btf_type_is_const_char_ptr(const struct btf *btf, const struct btf_type *t)
+{
+ const char *tname;
+
+ /* The type chain has to be PTR->CONST->CHAR */
+ if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR)
+ return false;
+
+ t = btf_type_by_id(btf, t->type);
+ if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)
+ return false;
+
+ t = btf_type_by_id(btf, t->type);
+ tname = btf_name_by_offset(btf, t->name_off);
+ if (tname && strcmp(tname, "char") == 0)
+ return true;
+ return false;
+}
+
/*
* Check that given struct member is a regular int with expected
* offset and size.
@@ -6746,6 +6765,20 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
/* Default prog with MAX_BPF_FUNC_REG_ARGS args */
return true;
t = btf_type_by_id(btf, args[arg].type);
+
+ /*
+ * For const string, we need to match "const char *"
+ * exactly. Therefore, do the check before the skipping
+ * modifiers.
+ */
+ if (btf_type_is_const_char_ptr(btf, t)) {
+ info->reg_type = PTR_TO_BTF_ID;
+ if (prog_args_trusted(prog))
+ info->reg_type |= PTR_TRUSTED;
+ info->btf = btf;
+ info->btf_id = args[arg].type;
+ return true;
+ }
}
/* skip modifiers */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 766695491bc5..a9757c056d4b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9598,8 +9598,12 @@ static enum bpf_dynptr_type dynptr_get_type(struct bpf_verifier_env *env,
return state->stack[spi].spilled_ptr.dynptr.type;
}
-static int check_reg_const_str(struct bpf_verifier_env *env,
- struct bpf_reg_state *reg, u32 regno)
+/*
+ * Check for const string saved in a bpf map. The caller is responsible
+ * to check reg->type == PTR_TO_MAP_VALUE.
+ */
+static int check_reg_const_str_in_map(struct bpf_verifier_env *env,
+ struct bpf_reg_state *reg, u32 regno)
{
struct bpf_map *map = reg->map_ptr;
int err;
@@ -9607,9 +9611,6 @@ static int check_reg_const_str(struct bpf_verifier_env *env,
u64 map_addr;
char *str_ptr;
- if (reg->type != PTR_TO_MAP_VALUE)
- return -EINVAL;
-
if (!bpf_map_is_rdonly(map)) {
verbose(env, "R%d does not point to a readonly map'\n", regno);
return -EACCES;
@@ -9646,6 +9647,26 @@ static int check_reg_const_str(struct bpf_verifier_env *env,
return 0;
}
+/* Check for const string passed in as input to the bpf program. */
+static int check_reg_const_str_arg(struct bpf_reg_state *reg)
+{
+ const struct btf *btf;
+ const struct btf_type *t;
+ const char *tname;
+
+ if (base_type(reg->type) != PTR_TO_BTF_ID)
+ return -EINVAL;
+
+ btf = reg->btf;
+ t = btf_type_by_id(btf, reg->btf_id);
+ if (!t)
+ return -EINVAL;
+
+ if (btf_type_is_const_char_ptr(btf, t))
+ return 0;
+ return -EINVAL;
+}
+
/* Returns constant key value in `value` if possible, else negative error */
static int get_constant_map_key(struct bpf_verifier_env *env,
struct bpf_reg_state *key,
@@ -9964,7 +9985,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
break;
case ARG_PTR_TO_CONST_STR:
{
- err = check_reg_const_str(env, reg, regno);
+ if (reg->type != PTR_TO_MAP_VALUE)
+ return -EINVAL;
+ err = check_reg_const_str_in_map(env, reg, regno);
if (err)
return err;
break;
@@ -13626,13 +13649,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
meta->arg_btf_id = reg->btf_id;
break;
case KF_ARG_PTR_TO_CONST_STR:
- if (reg->type != PTR_TO_MAP_VALUE) {
- verbose(env, "arg#%d doesn't point to a const string\n", i);
- return -EINVAL;
+ if (reg->type == PTR_TO_MAP_VALUE) {
+ ret = check_reg_const_str_in_map(env, reg, regno);
+ if (ret)
+ return ret;
+ } else {
+ ret = check_reg_const_str_arg(reg);
+ if (ret) {
+ verbose(env, "arg#%d doesn't point to a const string\n", i);
+ return ret;
+ }
}
- ret = check_reg_const_str(env, reg, regno);
- if (ret)
- return ret;
break;
case KF_ARG_PTR_TO_WORKQUEUE:
if (reg->type != PTR_TO_MAP_VALUE) {
--
2.47.3
^ permalink raw reply related
* [PATCH bpf-next 0/3] Introduce bpf_kern_path and bpf_path_put
From: Song Liu @ 2025-11-27 0:50 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-security-module
Cc: ast, daniel, andrii, kernel-team, viro, brauner, jack, paul,
jmorris, serge, Song Liu
Security solutions use LSM hook security_sb_mount to monitor mount
operations. security_sb_mount takes dev_name as a string. To get a struct
path from dev_name, in-tree LSMs use kern_path. Introduce kfuncs
bpf_kern_path so that bpf LSM can do similar operations. bpf_kern_path
takes a reference on the return value path. Also add kfunc bpf_path_put to
release path returned by bpf_kern_path. Note that, bpf_kern_path only holds
reference on the path during the duration of this bpf program. The verifier
enforces the bpf program release this reference.
Patch 1/3 prepares bpf verifier to handle const char * passed in as hook
argument. Before this change, bpf helpers and kfuncs only consider value
from read only map as const string.
Patch 2/3 adds the two kfuncs.
Patch 3/3 add tests for the new kfuncs.
Song Liu (3):
bpf: Allow const char * from LSM hooks as kfunc const string arguments
bpf: Add bpf_kern_path and bpf_path_put kfuncs
selftests/bpf: Add tests for bpf_kern_path kfunc
fs/bpf_fs_kfuncs.c | 58 +++++++++++
include/linux/btf.h | 1 +
kernel/bpf/btf.c | 33 +++++++
kernel/bpf/verifier.c | 51 +++++++---
.../testing/selftests/bpf/bpf_experimental.h | 4 +
.../selftests/bpf/prog_tests/kern_path.c | 82 ++++++++++++++++
.../selftests/bpf/progs/test_kern_path.c | 56 +++++++++++
.../selftests/bpf/progs/verifier_kern_path.c | 52 ++++++++++
.../bpf/progs/verifier_kern_path_fail.c | 97 +++++++++++++++++++
9 files changed, 422 insertions(+), 12 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path.c
create mode 100644 tools/testing/selftests/bpf/progs/verifier_kern_path_fail.c
--
2.47.3
^ permalink raw reply
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Gregory Lumen @ 2025-11-26 23:40 UTC (permalink / raw)
To: Roberto Sassu
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: <1e5a3b427fe2783e57e88ca14630f5e38e01fac5.camel@huaweicloud.com>
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.
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
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.
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 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 22:18 UTC (permalink / raw)
To: david laight, Helge Deller
Cc: Helge Deller, John Paul Adrian Glaubitz, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <20251126212309.5b21edca@pumpkin>
On 11/26/25 13:23, david laight wrote:
> On Wed, 26 Nov 2025 16:12:23 +0100
> Helge Deller <deller@kernel.org> wrote:
>
>> * david laight <david.laight@runbox.com>:
>>> On Wed, 26 Nov 2025 12:03:03 +0100
>>> Helge Deller <deller@gmx.de> wrote:
>>>
>>>> On 11/26/25 11:44, david laight wrote:
>>> ...
>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>>> index 26e82ba879d44..3dcc342337aca 100644
>>>>>> --- a/security/apparmor/match.c
>>>>>> +++ b/security/apparmor/match.c
>>>>>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>>>>> u8, u8, byte_to_byte);
>>>>>
>>>>> Is that that just memcpy() ?
>>>>
>>>> No, it's memcpy() only on big-endian machines.
>>>
>>> You've misread the quoting...
>>> The 'data8' case that was only half there is a memcpy().
>>>
>>>> On little-endian machines it converts from big-endian
>>>> 16/32-bit ints to little-endian 16/32-bit ints.
>>>>
>>>> But I see some potential for optimization here:
>>>> a) on big-endian machines just use memcpy()
>>>
>>> true
>>>
>>>> b) on little-endian machines use memcpy() to copy from possibly-unaligned
>>>> memory to then known-to-be-aligned destination. Then use a loop with
>>>> be32_to_cpu() instead of get_unaligned_xx() as it's faster.
>>>
>>> There is a function that does a loop byteswap of a buffer - no reason
>>> to re-invent it.
>>
>> I assumed there must be something, but I did not see it. Which one?
>
> I can't find it either - just some functions to do an in-place swap.
> (Which aren't usually a good idea)
>
>>
>>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>>> The loop control and extra memory accesses kill performance.
>>
>> Yes, you are probably right.
>>
>>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>>> generate optimal code - For LE I think it is something like:
>>> low = *(addr & ~3);
>>> high = *((addr + 3) & ~3);
>>> shift = (addr & 3) * 8;
>>> value = low << shift | high >> (32 - shift);
>>> Note that it is only 2 aligned memory reads - even for 64bit.
>>
>> Ok, then maybe we should keep it simple like this patch:
>>
>> [PATCH v2] 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.
>
> Not sure that reads right.
> 'Allow for misaligned data from userspace when byteswapping source blob' ?
>
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b..386da2023d50 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -104,16 +104,20 @@ struct aa_dfa {
>> struct table_header *tables[YYTD_ID_TSIZE];
>> };
>>
>> -#define byte_to_byte(X) (X)
>> +#define byte_to_byte(X) (*(X))
>
> It's a bit of a shame you need something for the above...
>
We got rid of that in the last patch by just replacing the call to
UNPACK_ARRAY for bytes with just a call to memcpy.
> David
>
>
>>
>> #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) || sizeof(BTYPE) == 1) \
>> + 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..13e2f6873329 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"
>> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>> u8, u8, byte_to_byte);
>> 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
>
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: david laight @ 2025-11-26 21:23 UTC (permalink / raw)
To: Helge Deller
Cc: Helge Deller, John Johansen, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <aScY13MEBATreotz@carbonx1>
On Wed, 26 Nov 2025 16:12:23 +0100
Helge Deller <deller@kernel.org> wrote:
> * david laight <david.laight@runbox.com>:
> > On Wed, 26 Nov 2025 12:03:03 +0100
> > Helge Deller <deller@gmx.de> wrote:
> >
> > > On 11/26/25 11:44, david laight wrote:
> > ...
> > > >> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > > >> index 26e82ba879d44..3dcc342337aca 100644
> > > >> --- a/security/apparmor/match.c
> > > >> +++ b/security/apparmor/match.c
> > > >> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > > >> u8, u8, byte_to_byte);
> > > >
> > > > Is that that just memcpy() ?
> > >
> > > No, it's memcpy() only on big-endian machines.
> >
> > You've misread the quoting...
> > The 'data8' case that was only half there is a memcpy().
> >
> > > On little-endian machines it converts from big-endian
> > > 16/32-bit ints to little-endian 16/32-bit ints.
> > >
> > > But I see some potential for optimization here:
> > > a) on big-endian machines just use memcpy()
> >
> > true
> >
> > > b) on little-endian machines use memcpy() to copy from possibly-unaligned
> > > memory to then known-to-be-aligned destination. Then use a loop with
> > > be32_to_cpu() instead of get_unaligned_xx() as it's faster.
> >
> > There is a function that does a loop byteswap of a buffer - no reason
> > to re-invent it.
>
> I assumed there must be something, but I did not see it. Which one?
I can't find it either - just some functions to do an in-place swap.
(Which aren't usually a good idea)
>
> > But I doubt it is always (if ever) faster to do a copy and then byteswap.
> > The loop control and extra memory accesses kill performance.
>
> Yes, you are probably right.
>
> > Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> > generate optimal code - For LE I think it is something like:
> > low = *(addr & ~3);
> > high = *((addr + 3) & ~3);
> > shift = (addr & 3) * 8;
> > value = low << shift | high >> (32 - shift);
> > Note that it is only 2 aligned memory reads - even for 64bit.
>
> Ok, then maybe we should keep it simple like this patch:
>
> [PATCH v2] 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.
Not sure that reads right.
'Allow for misaligned data from userspace when byteswapping source blob' ?
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..386da2023d50 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,20 @@ struct aa_dfa {
> struct table_header *tables[YYTD_ID_TSIZE];
> };
>
> -#define byte_to_byte(X) (X)
> +#define byte_to_byte(X) (*(X))
It's a bit of a shame you need something for the above...
David
>
> #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) || sizeof(BTYPE) == 1) \
> + 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..13e2f6873329 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"
> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> u8, u8, byte_to_byte);
> 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
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 21:10 UTC (permalink / raw)
To: Helge Deller
Cc: david laight, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <aSdfyGv2T88T5FEu@carbonx1>
On 11/26/25 12:15, Helge Deller wrote:
> * John Johansen <john.johansen@canonical.com>:
>> On 11/26/25 07:12, Helge Deller wrote:
>>> * david laight <david.laight@runbox.com>:
>>>> On Wed, 26 Nov 2025 12:03:03 +0100
>>>> Helge Deller <deller@gmx.de> wrote:
>>>>
>>>>> On 11/26/25 11:44, david laight wrote:
>>>> ...
>>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>>>> index 26e82ba879d44..3dcc342337aca 100644
>>>>>>> --- a/security/apparmor/match.c
>>>>>>> +++ b/security/apparmor/match.c
>>>>>>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>>>>>> u8, u8, byte_to_byte);
>>>>>>
>>>>>> Is that that just memcpy() ?
>>>>>
>>>>> No, it's memcpy() only on big-endian machines.
>>>>
>>>> You've misread the quoting...
>>>> The 'data8' case that was only half there is a memcpy().
>>>>
>>>>> On little-endian machines it converts from big-endian
>>>>> 16/32-bit ints to little-endian 16/32-bit ints.
>>>>>
>>>>> But I see some potential for optimization here:
>>>>> a) on big-endian machines just use memcpy()
>>>>
>>>> true
>>>>
>>>>> b) on little-endian machines use memcpy() to copy from possibly-unaligned
>>>>> memory to then known-to-be-aligned destination. Then use a loop with
>>>>> be32_to_cpu() instead of get_unaligned_xx() as it's faster.
>>>>
>>>> There is a function that does a loop byteswap of a buffer - no reason
>>>> to re-invent it.
>>>
>>> I assumed there must be something, but I did not see it. Which one?
>>>
>>>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>>>> The loop control and extra memory accesses kill performance.
>>>
>>> Yes, you are probably right.
>>>
>>>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>>>> generate optimal code - For LE I think it is something like:
>>>> low = *(addr & ~3);
>>>> high = *((addr + 3) & ~3);
>>>> shift = (addr & 3) * 8;
>>>> value = low << shift | high >> (32 - shift);
>>>> Note that it is only 2 aligned memory reads - even for 64bit.
>>>
>>> Ok, then maybe we should keep it simple like this patch:
>>>
>>> [PATCH v2] 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..386da2023d50 100644
>>> --- a/security/apparmor/include/match.h
>>> +++ b/security/apparmor/include/match.h
>>> @@ -104,16 +104,20 @@ struct aa_dfa {
>>> struct table_header *tables[YYTD_ID_TSIZE];
>>> };
>>> -#define byte_to_byte(X) (X)
>>> +#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) || sizeof(BTYPE) == 1) \
>>> + 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..13e2f6873329 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"
>>> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>> u8, u8, byte_to_byte);
>>> 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
>>
>> I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
>> ie.
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index 26e82ba879d44..389202560675c 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -67,8 +67,7 @@ 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);
>> + memcp(table->td_data, blob, th.td_lolen);
>
> True.
> Then byte_to_byte() can go away in match.h as well.
> So, here is a (untested) v3:
>
and lightly tested now
I will pull it into my tree
>
> [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>
>
Acked-by: John Johansen <john.johansen@canonical.com>
> 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
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-26 20:15 UTC (permalink / raw)
To: John Johansen
Cc: david laight, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <f5637038-9661-47fe-ba69-e461760ac975@canonical.com>
* John Johansen <john.johansen@canonical.com>:
> On 11/26/25 07:12, Helge Deller wrote:
> > * david laight <david.laight@runbox.com>:
> > > On Wed, 26 Nov 2025 12:03:03 +0100
> > > Helge Deller <deller@gmx.de> wrote:
> > >
> > > > On 11/26/25 11:44, david laight wrote:
> > > ...
> > > > > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > > > > > index 26e82ba879d44..3dcc342337aca 100644
> > > > > > --- a/security/apparmor/match.c
> > > > > > +++ b/security/apparmor/match.c
> > > > > > @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > > > > > u8, u8, byte_to_byte);
> > > > >
> > > > > Is that that just memcpy() ?
> > > >
> > > > No, it's memcpy() only on big-endian machines.
> > >
> > > You've misread the quoting...
> > > The 'data8' case that was only half there is a memcpy().
> > >
> > > > On little-endian machines it converts from big-endian
> > > > 16/32-bit ints to little-endian 16/32-bit ints.
> > > >
> > > > But I see some potential for optimization here:
> > > > a) on big-endian machines just use memcpy()
> > >
> > > true
> > >
> > > > b) on little-endian machines use memcpy() to copy from possibly-unaligned
> > > > memory to then known-to-be-aligned destination. Then use a loop with
> > > > be32_to_cpu() instead of get_unaligned_xx() as it's faster.
> > >
> > > There is a function that does a loop byteswap of a buffer - no reason
> > > to re-invent it.
> >
> > I assumed there must be something, but I did not see it. Which one?
> >
> > > But I doubt it is always (if ever) faster to do a copy and then byteswap.
> > > The loop control and extra memory accesses kill performance.
> >
> > Yes, you are probably right.
> >
> > > Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> > > generate optimal code - For LE I think it is something like:
> > > low = *(addr & ~3);
> > > high = *((addr + 3) & ~3);
> > > shift = (addr & 3) * 8;
> > > value = low << shift | high >> (32 - shift);
> > > Note that it is only 2 aligned memory reads - even for 64bit.
> >
> > Ok, then maybe we should keep it simple like this patch:
> >
> > [PATCH v2] 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..386da2023d50 100644
> > --- a/security/apparmor/include/match.h
> > +++ b/security/apparmor/include/match.h
> > @@ -104,16 +104,20 @@ struct aa_dfa {
> > struct table_header *tables[YYTD_ID_TSIZE];
> > };
> > -#define byte_to_byte(X) (X)
> > +#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) || sizeof(BTYPE) == 1) \
> > + 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..13e2f6873329 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"
> > @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > u8, u8, byte_to_byte);
> > 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
>
> I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
> ie.
>
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 26e82ba879d44..389202560675c 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -67,8 +67,7 @@ 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);
> + memcp(table->td_data, blob, th.td_lolen);
True.
Then byte_to_byte() can go away in match.h as well.
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
^ permalink raw reply related
* Re: [PATCH v15 3/4] lsm: count the LSMs enabled at compile time
From: Andy Shevchenko @ 2025-11-26 19:44 UTC (permalink / raw)
To: Paul Moore
Cc: KP Singh, linux-security-module, bpf, ast, casey, andrii,
keescook, daniel, renauld, revest, song, linux, Kui-Feng Lee,
John Johansen
In-Reply-To: <19ac18b9e00.2843.85c95baa4474aabc7814e68940a78392@paul-moore.com>
On Wed, Nov 26, 2025 at 02:02:24PM -0500, Paul Moore wrote:
> On November 26, 2025 12:14:21 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Fri, Aug 16, 2024 at 05:43:06PM +0200, KP Singh wrote:
...
> > > -/* This counts to 12. Any more, it will return 13th argument. */
> > > -#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10,
> > > _11, _12, _n, X...) _n
> > > -#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7,
> > > 6, 5, 4, 3, 2, 1, 0)
> > > +/* This counts to 15. Any more, it will return 16th argument. */
> > > +#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10,
> > > _11, _12, _13, _14, _15, _n, X...) _n
> > > +#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 15, 14, 13, 12, 11,
> > > 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> >
> > You forgot to update _12 in the upper comment.
>
> Do you plan to send a patch to fix this?
Not really. Too lazy to write a commit message for it.
Appreciate if you or the initial author or somebody else
can do that.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 19:33 UTC (permalink / raw)
To: Helge Deller, david laight
Cc: Helge Deller, John Paul Adrian Glaubitz, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <aScY13MEBATreotz@carbonx1>
On 11/26/25 07:12, Helge Deller wrote:
> * david laight <david.laight@runbox.com>:
>> On Wed, 26 Nov 2025 12:03:03 +0100
>> Helge Deller <deller@gmx.de> wrote:
>>
>>> On 11/26/25 11:44, david laight wrote:
>> ...
>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>> index 26e82ba879d44..3dcc342337aca 100644
>>>>> --- a/security/apparmor/match.c
>>>>> +++ b/security/apparmor/match.c
>>>>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>>>> u8, u8, byte_to_byte);
>>>>
>>>> Is that that just memcpy() ?
>>>
>>> No, it's memcpy() only on big-endian machines.
>>
>> You've misread the quoting...
>> The 'data8' case that was only half there is a memcpy().
>>
>>> On little-endian machines it converts from big-endian
>>> 16/32-bit ints to little-endian 16/32-bit ints.
>>>
>>> But I see some potential for optimization here:
>>> a) on big-endian machines just use memcpy()
>>
>> true
>>
>>> b) on little-endian machines use memcpy() to copy from possibly-unaligned
>>> memory to then known-to-be-aligned destination. Then use a loop with
>>> be32_to_cpu() instead of get_unaligned_xx() as it's faster.
>>
>> There is a function that does a loop byteswap of a buffer - no reason
>> to re-invent it.
>
> I assumed there must be something, but I did not see it. Which one?
>
>> But I doubt it is always (if ever) faster to do a copy and then byteswap.
>> The loop control and extra memory accesses kill performance.
>
> Yes, you are probably right.
>
>> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
>> generate optimal code - For LE I think it is something like:
>> low = *(addr & ~3);
>> high = *((addr + 3) & ~3);
>> shift = (addr & 3) * 8;
>> value = low << shift | high >> (32 - shift);
>> Note that it is only 2 aligned memory reads - even for 64bit.
>
> Ok, then maybe we should keep it simple like this patch:
>
> [PATCH v2] 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..386da2023d50 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,16 +104,20 @@ struct aa_dfa {
> struct table_header *tables[YYTD_ID_TSIZE];
> };
>
> -#define byte_to_byte(X) (X)
> +#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) || sizeof(BTYPE) == 1) \
> + 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..13e2f6873329 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"
> @@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> u8, u8, byte_to_byte);
> 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
I think we can make one more tweak, in just not using UNPACK_ARRAY at all for the byte case
ie.
diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index 26e82ba879d44..389202560675c 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -67,8 +67,7 @@ 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);
+ memcp(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);
^ permalink raw reply related
* [PATCH v4 0/4] Landlock: Disconnected directory handling
From: Mickaël Salaün @ 2025-11-26 19:11 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Mikhail Ivanov, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module
Hi,
This patch series fixes and test Landlock's handling of disconnected
directories.
This fourth version implements an hybrid version of disconnected
directory handling, simpler and safer, suggested by Tingmao. One patch
was merged, and a new one improve variable scope.
Previous versions:
v3: https://lore.kernel.org/r/20250719104204.545188-1-mic@digikod.net
v2: https://lore.kernel.org/r/20250711191938.2007175-1-mic@digikod.net
v1: https://lore.kernel.org/r/20250701183812.3201231-1-mic@digikod.net
Regards,
Mickaël Salaün (3):
landlock: Fix handling of disconnected directories
landlock: Improve variable scope
selftests/landlock: Add disconnected leafs and branch test suites
Tingmao Wang (1):
selftests/landlock: Add tests for access through disconnected paths
security/landlock/errata/abi-1.h | 16 +
security/landlock/fs.c | 43 +-
tools/testing/selftests/landlock/fs_test.c | 1335 +++++++++++++++++++-
3 files changed, 1373 insertions(+), 21 deletions(-)
create mode 100644 security/landlock/errata/abi-1.h
--
2.51.0
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 19:22 UTC (permalink / raw)
To: david laight
Cc: Helge Deller, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <20251126104444.29002552@pumpkin>
On 11/26/25 02:44, david laight wrote:
> On Wed, 26 Nov 2025 01:11:45 -0800
> John Johansen <john.johansen@canonical.com> wrote:
>
>> On 11/25/25 13:13, Helge Deller wrote:
>>> On 11/25/25 20:20, John Johansen wrote:
>>>> On 11/25/25 07:11, Helge Deller wrote:
>>>>> * John Johansen <john.johansen@canonical.com>:
>>>>>> On 11/18/25 04:49, Helge Deller wrote:
>>>>>>> Hi Adrian,
>>>>>>>
>>>>>>> On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
>>>>>>>> On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
>>>>>>>>> My patch fixed two call sites, but I suspect you see another call site which
>>>>>>>>> hasn't been fixed yet.
>>>>>>>>>
>>>>>>>>> Can you try attached patch? It might indicate the caller of the function and
>>>>>>>>> maybe prints the struct name/address which isn't aligned.
>>>>>>>>>
>>>>>>>>> Helge
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>>>>>> index c5a91600842a..b477430c07eb 100644
>>>>>>>>> --- a/security/apparmor/match.c
>>>>>>>>> +++ b/security/apparmor/match.c
>>>>>>>>> @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>>>>>>>>> if (size < sizeof(struct table_set_header))
>>>>>>>>> goto fail;
>>>>>>>>> + if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
>>>>>>>>> + pr_warn("dfa blob stream %pS not aligned.\n", data);
>>>>>>>>> +
>>>>>>>>> if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
>>>>>>>>> goto fail;
>>>>>>>>
>>>>>>>> Here is the relevant output with the patch applied:
>>>>>>>>
>>>>>>>> [ 73.840639] ------------[ cut here ]------------
>>>>>>>> [ 73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
>>>>>>>> sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
>>>>>>>> [ 74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
>>>>>>>> [ 74.536543] Call Trace:
>>>>>>>> [ 74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
>>>>>>>> [ 74.633757] [<0000000000476438>] __warn+0xd8/0x100
>>>>>>>> [ 74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
>>>>>>>> [ 74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
>>>>>>>> [ 74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
>>>>>>>> [ 74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
>>>>>>>> [ 75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
>>>>>>>> [ 75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
>>>>>>>> [ 75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
>>>>>>>> [ 75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
>>>>>>>> [ 75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
>>>>>>>> [ 75.406932] [<0000000000767174>] sys_write+0x14/0x40
>>>>>>>> [ 75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
>>>>>>>> [ 75.548802] ---[ end trace 0000000000000000 ]---
>>>>>>>> [ 75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
>>>>>>>> [ 75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
>>>>>>>
>>>>>>> The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
>>>>>>> (via the write syscall).
>>>>>>> Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
>>>>>>> a source address which is not correctly aligned.
>>>>>>
>>>>>> the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
>>>>>> is but the userspace tooling hasn't been treating it as aligned. With that said,
>>>>>> the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
>>>>>> and there really should be some validation to catch it.
>>>>>>
>>>>>>> You should be able to debug/find the problematic code with strace from userspace.
>>>>>>> Maybe someone with apparmor knowledge here on the list has an idea?
>>>>>>>
>>>>>> This is likely an unaligned 2nd profile, being split out and loaded separately
>>>>>> from the rest of the container. Basically the loader for some reason (there
>>>>>> are a few different possible reasons) is poking into the container format and
>>>>>> pulling out the profile at some offset, this gets loaded to the kernel but
>>>>>> it would seem that its causing an issue with the dfa alignment within the container,
>>>>>> which should be aligned to the original container.
>>>>>
>>>>>
>>>>> Regarding this:
>>>>>
>>>>>> Kernel side, we are going to need to add some extra verification checks, it should
>>>>>> be catching this, as unaligned as part of the unpack. Userspace side, we will have
>>>>>> to verify my guess and fix the loader.
>>>>>
>>>>> I wonder if loading those tables are really time critical?
>>>>
>>>> no, most policy is loaded once on boot and then at package upgrades. There are some
>>>> bits that may be loaded at application startup like, snap, libvirt, lxd, basically
>>>> container managers might do some thing custom per container.
>>>>
>>>> Its the run time we want to minimize, the cost of.
>>>>
>>>> Policy already can be unaligned (the container format rework to fix this is low
>>>> priority), and is treated as untrusted. It goes through an unpack, and translation to
>>>> machine native, with as many bounds checks, necessary transforms etc done at unpack
>>>> time as possible, so that the run time costs can be minimized.
>>>>
>>>>> If not, maybe just making the kernel aware that the tables might be unaligned
>>>>> can help, e.g. with the following (untested) patch.
>>>>> Adrian, maybe you want to test?
>>>>>
>>>>
>>>>> ------------------------
>>>>>
>>>>> [PATCH] Allow apparmor to handle unaligned dfa tables
>>>>>
>>>>> The dfa tables can originate from kernel or userspace and 8-byte alignment
>>>>> isn't always guaranteed and as such may trigger unaligned memory accesses
>>>>> on various architectures.
>>>>> Work around it by using the get_unaligned_xx() helpers.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>> lgtm,
>>>>
>>>> Acked-by: John Johansen <john.johansen@canonical.com>
>>>>
>>>> I'll pull this into my tree regardless of whether it fixes the issue
>>>> for Adrian, as it definitely fixes an issue.
>>>>
>>>> We can added additional patches on top s needed.
>>>
>>> My patch does not modify the UNPACK_ARRAY() macro, which we
>>> possibly should adjust as well.
>>
>> Indeed. See the patch below. I am not surprised testing hasn't triggered this
>> case, but a malicious userspace could certainly construct a policy that would
>> trigger it. Yes it would have to be root, but I still would like to prevent
>> root from being able to trigger this.
>>
>>> Adrian's testing seems to trigger only a few unaligned accesses,
>>> so maybe it's not a issue currently.
>>>
>> I don't think the userspace compiler is generating one that is bad, but it
>> possible to construct one and get it to the point where it can trip in
>> UNPACK_ARRAY
>>
>> commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
>> Author: John Johansen <john.johansen@canonical.com>
>> Date: Tue Nov 25 13:59:32 2025 -0800
>>
>> apparmor: fix unaligned memory access of UNPACK_ARRAY
>>
>> The UNPACK_ARRAY macro has the potential to have unaligned memory
>> access when the unpacking an unaligned profile, which is caused by
>> userspace splitting up a profile container before sending it to the
>> kernel.
>>
>> While this is corner case, policy loaded from userspace should be
>> treated as untrusted so ensure that userspace can not trigger an
>> unaligned access.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b1..203f7c07529f5 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -104,7 +104,7 @@ struct aa_dfa {
>> struct table_header *tables[YYTD_ID_TSIZE];
>> };
>>
>> -#define byte_to_byte(X) (X)
>> +#define byte_to_byte(X) *(X)
>
> Even though is is only used once that ought to be (*(X))
>
>>
>> #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
>> do { \
>> @@ -112,7 +112,7 @@ struct aa_dfa {
>> TTYPE *__t = (TTYPE *) TABLE; \
>> BTYPE *__b = (BTYPE *) BLOB; \
>> for (__i = 0; __i < LEN; __i++) { \
>> - __t[__i] = NTOHX(__b[__i]); \
>> + __t[__i] = NTOHX(&__b[__i]); \
>> } \
>> } while (0)
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index 26e82ba879d44..3dcc342337aca 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>> u8, u8, byte_to_byte);
>
> Is that that just memcpy() ?
>
yeah for the byte case it is and we should just replace that case of UNPACK_ARRAY
> David
>
>> 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
>>
>>
>>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox