* Re: [PATCH v2 3/3] assoc_array: trim the final shortcut word using the current chunk end
From: Jarkko Sakkinen @ 2026-07-18 18:46 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Andrew Morton, Paul Moore, James Morris,
Serge E . Hallyn, keyrings, linux-security-module, linux-kernel
In-Reply-To: <alvIBm3Po_1gX6g_@kernel.org>
On Sat, Jul 18, 2026 at 09:38:01PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 14, 2026 at 07:54:51AM -0400, Michael Bommarito wrote:
> > assoc_array_walk() masks off the bits past shortcut->skip_to_level in the
> > word that contains skip_to_level, gated on
> > round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > skip_to_level.
> >
> > That guard is wrong in two opposite ways:
> >
> > - When sc_level is word-aligned (every word after the first) round_up()
> > is a no-op, so the guard is sc_level > skip_to_level and never fires for
> > the word that holds skip_to_level. A shortcut that spans more than one
> > word and ends in the middle of its last word leaves that word untrimmed,
> > and its stale high bits leak into the dissimilarity word and can steer
> > the walk down the wrong descendant.
> >
> > - When sc_level is unaligned (the first word) and skip_to_level sits on
> > the next chunk boundary, sc_level + CHUNK would exceed skip_to_level and
> > fire the trim with shift = skip_to_level & CHUNK_MASK == 0, which clears
> > the whole dissimilarity word and makes a differing shortcut compare
> > equal.
> >
> > Use the end of the chunk that contains sc_level instead:
> >
> > skip_to_level < round_down(sc_level, CHUNK) + CHUNK
> >
> > For an aligned sc_level whose word holds skip_to_level this now fires (the
> > first bug); for an unaligned sc_level with skip_to_level on the following
> > boundary it does not, so shift is never 0 when the branch runs and the trim
> > never clears the whole word.
> >
> > Fixes: 3cb989501c26 ("Add a generic associative array implementation.")
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> > ---
> > lib/assoc_array.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> > index bcc6e0a013eb8..b6c9723e12ced 100644
> > --- a/lib/assoc_array.c
> > +++ b/lib/assoc_array.c
> > @@ -255,7 +255,8 @@ assoc_array_walk(const struct assoc_array *array,
> > sc_segments = shortcut->index_key[sc_level >> ASSOC_ARRAY_KEY_CHUNK_SHIFT];
> > dissimilarity = segments ^ sc_segments;
> >
> > - if (round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > shortcut->skip_to_level) {
> > + if (shortcut->skip_to_level < round_down(sc_level,
> > + ASSOC_ARRAY_KEY_CHUNK_SIZE) + ASSOC_ARRAY_KEY_CHUNK_SIZE) {
> > /* Trim segments that are beyond the shortcut */
> > int shift = shortcut->skip_to_level & ASSOC_ARRAY_KEY_CHUNK_MASK;
> > dissimilarity &= ~(ULONG_MAX << shift);
> > --
> > 2.53.0
> >
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> BR, Jarkko
Were you able to reproduce this with basic command-line tools? The
patches are verifiable by reading the code but asking this just in
case if you had a snippet at hand (not interested on complex
reproducers).
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v2 3/3] assoc_array: trim the final shortcut word using the current chunk end
From: Jarkko Sakkinen @ 2026-07-18 18:37 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Andrew Morton, Paul Moore, James Morris,
Serge E . Hallyn, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260714115451.3773164-4-michael.bommarito@gmail.com>
On Tue, Jul 14, 2026 at 07:54:51AM -0400, Michael Bommarito wrote:
> assoc_array_walk() masks off the bits past shortcut->skip_to_level in the
> word that contains skip_to_level, gated on
> round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > skip_to_level.
>
> That guard is wrong in two opposite ways:
>
> - When sc_level is word-aligned (every word after the first) round_up()
> is a no-op, so the guard is sc_level > skip_to_level and never fires for
> the word that holds skip_to_level. A shortcut that spans more than one
> word and ends in the middle of its last word leaves that word untrimmed,
> and its stale high bits leak into the dissimilarity word and can steer
> the walk down the wrong descendant.
>
> - When sc_level is unaligned (the first word) and skip_to_level sits on
> the next chunk boundary, sc_level + CHUNK would exceed skip_to_level and
> fire the trim with shift = skip_to_level & CHUNK_MASK == 0, which clears
> the whole dissimilarity word and makes a differing shortcut compare
> equal.
>
> Use the end of the chunk that contains sc_level instead:
>
> skip_to_level < round_down(sc_level, CHUNK) + CHUNK
>
> For an aligned sc_level whose word holds skip_to_level this now fires (the
> first bug); for an unaligned sc_level with skip_to_level on the following
> boundary it does not, so shift is never 0 when the branch runs and the trim
> never clears the whole word.
>
> Fixes: 3cb989501c26 ("Add a generic associative array implementation.")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> lib/assoc_array.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> index bcc6e0a013eb8..b6c9723e12ced 100644
> --- a/lib/assoc_array.c
> +++ b/lib/assoc_array.c
> @@ -255,7 +255,8 @@ assoc_array_walk(const struct assoc_array *array,
> sc_segments = shortcut->index_key[sc_level >> ASSOC_ARRAY_KEY_CHUNK_SHIFT];
> dissimilarity = segments ^ sc_segments;
>
> - if (round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > shortcut->skip_to_level) {
> + if (shortcut->skip_to_level < round_down(sc_level,
> + ASSOC_ARRAY_KEY_CHUNK_SIZE) + ASSOC_ARRAY_KEY_CHUNK_SIZE) {
> /* Trim segments that are beyond the shortcut */
> int shift = shortcut->skip_to_level & ASSOC_ARRAY_KEY_CHUNK_MASK;
> dissimilarity &= ~(ULONG_MAX << shift);
> --
> 2.53.0
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v2 2/3] keys: make keyring key-chunk byte order agree with keyring_diff_objects()
From: Jarkko Sakkinen @ 2026-07-18 18:36 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Andrew Morton, Paul Moore, James Morris,
Serge E . Hallyn, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260714115451.3773164-3-michael.bommarito@gmail.com>
On Tue, Jul 14, 2026 at 07:54:50AM -0400, Michael Bommarito wrote:
> keyring_get_key_chunk() loads description bytes into the index chunk low
> address first, while keyring_diff_objects() numbers the first differing
> bit from the low end and folds the absolute byte index into the level
> without removing the inline-prefix offset the level already carries.
> The two disagree on byte order and bit position, so the array can be
> told two keys first differ at a bit that does not differ in the chunk
> the walker uses, letting crafted descriptions collide into one node.
>
> Load the chunk in the order keyring_diff_objects() assumes and drop the
> inline-prefix length when folding the byte index into the level. This
> only changes the in-memory ordering used to place keys within a keyring;
> add, search and read of non-colliding keys are unaffected.
>
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> security/keys/keyring.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 1739373172ad5..e7066893e6ffc 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -292,9 +292,10 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> desc_len -= offset;
> if (desc_len > n)
> desc_len = n;
> + d += desc_len;
> do {
> chunk <<= 8;
> - chunk |= *d++;
> + chunk |= *--d;
> } while (--desc_len > 0);
> return chunk;
> }
> @@ -375,7 +376,7 @@ static int keyring_diff_objects(const void *object, const void *data)
> return -1;
>
> differ_plus_i:
> - level += i;
> + level += i - (int)sizeof(a->desc);
> differ:
> i = level * 8 + __ffs(seg_a ^ seg_b);
> return i;
> --
> 2.53.0
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v2 1/3] keys: fix out-of-bounds read in keyring_get_key_chunk()
From: Jarkko Sakkinen @ 2026-07-18 18:36 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Andrew Morton, Paul Moore, James Morris,
Serge E . Hallyn, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260714115451.3773164-2-michael.bommarito@gmail.com>
On Tue, Jul 14, 2026 at 07:54:49AM -0400, Michael Bommarito wrote:
> For description-level chunks keyring_get_key_chunk() advances the read
> pointer by level * sizeof(long) past the inline prefix but only
> bounds-checks the prefix, so a long enough key description is read past
> its kmemdup(desc, desc_len + 1) allocation. Compute the full byte
> offset and bounds-check the description against it before reading.
>
> The walk only reaches a description-level chunk when two keys collide
> through the hash, x, type and domain_tag chunks, so this is reached from
> an unprivileged add_key(2) with a crafted pair of same-type keys whose
> index hashes collide; KASAN reports a slab-out-of-bounds read.
>
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> security/keys/keyring.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> KASAN, x86_64: add_key(2) of a crafted hash-colliding "user"-key pair
> (~63-byte descriptions) reports
>
> BUG: KASAN: slab-out-of-bounds in keyring_get_key_chunk
> keyring_get_key_chunk <- assoc_array_insert <- __key_link_begin
> <- __do_sys_add_key
>
> reading one byte past the description allocation; the same trigger is
> KASAN-clean with this patch. On a kernel built without init-on-alloc,
> reading the colliding keyring back with KEYCTL_READ returns
> uninitialized slab until patches 2 and 3 are applied too. Trigger
> available off-list.
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 7a2ee0ded7c93..1739373172ad5 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -270,7 +270,7 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> const struct keyring_index_key *index_key = data;
> unsigned long chunk = 0;
> const u8 *d;
> - int desc_len = index_key->desc_len, n = sizeof(chunk);
> + int desc_len = index_key->desc_len, n = sizeof(chunk), offset;
>
> level /= ASSOC_ARRAY_KEY_CHUNK_SIZE;
> switch (level) {
> @@ -284,12 +284,12 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> return (unsigned long)index_key->domain_tag;
> default:
> level -= 4;
> - if (desc_len <= sizeof(index_key->desc))
> + offset = sizeof(index_key->desc) + level * sizeof(long);
> + if (desc_len <= offset)
> return 0;
>
> - d = index_key->description + sizeof(index_key->desc);
> - d += level * sizeof(long);
> - desc_len -= sizeof(index_key->desc);
> + d = index_key->description + offset;
> + desc_len -= offset;
> if (desc_len > n)
> desc_len = n;
> do {
> --
> 2.53.0
>
Same feedback as before.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 3/3] assoc_array: trim the final shortcut word when skip_to_level is chunk-aligned
From: Jarkko Sakkinen @ 2026-07-18 18:34 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
Andrew Morton, keyrings, linux-security-module, linux-kernel
In-Reply-To: <alvGyuUIvjJiRjPk@kernel.org>
On Sat, Jul 18, 2026 at 09:32:46PM +0300, Jarkko Sakkinen wrote:
> On Sat, Jul 11, 2026 at 09:45:00PM -0400, Michael Bommarito wrote:
> > assoc_array_walk() masks off the bits past shortcut->skip_to_level in
> > the final word of a shortcut before testing it, gated on
> > round_up(sc_level, chunk_size) > skip_to_level. Once sc_level is
> > word-aligned (every word after the first) round_up() is a no-op and the
> > guard never fires for the word that contains skip_to_level, so its stale
> > high bits leak into the dissimilarity word and can steer the walk down
> > the wrong descendant.
> >
> > Test sc_level + ASSOC_ARRAY_KEY_CHUNK_SIZE > skip_to_level directly; an
> > exact-multiple skip_to_level ends on the boundary and stays untrimmed.
> >
> > Fixes: 3cb989501c26 ("Add a generic associative array implementation.")
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> > ---
> > lib/assoc_array.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> > index bcc6e0a013eb8..1de2c337f8fcf 100644
> > --- a/lib/assoc_array.c
> > +++ b/lib/assoc_array.c
> > @@ -255,7 +255,7 @@ assoc_array_walk(const struct assoc_array *array,
> > sc_segments = shortcut->index_key[sc_level >> ASSOC_ARRAY_KEY_CHUNK_SHIFT];
> > dissimilarity = segments ^ sc_segments;
> >
> > - if (round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > shortcut->skip_to_level) {
> > + if (sc_level + ASSOC_ARRAY_KEY_CHUNK_SIZE > shortcut->skip_to_level) {
> > /* Trim segments that are beyond the shortcut */
> > int shift = shortcut->skip_to_level & ASSOC_ARRAY_KEY_CHUNK_MASK;
> > dissimilarity &= ~(ULONG_MAX << shift);
> > --
> > 2.53.0
> >
>
> Ditto.
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> BR, Jarkko
On holiday up until end of this month so expect some delay in my
responses.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 3/3] assoc_array: trim the final shortcut word when skip_to_level is chunk-aligned
From: Jarkko Sakkinen @ 2026-07-18 18:32 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
Andrew Morton, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260712014500.480410-4-michael.bommarito@gmail.com>
On Sat, Jul 11, 2026 at 09:45:00PM -0400, Michael Bommarito wrote:
> assoc_array_walk() masks off the bits past shortcut->skip_to_level in
> the final word of a shortcut before testing it, gated on
> round_up(sc_level, chunk_size) > skip_to_level. Once sc_level is
> word-aligned (every word after the first) round_up() is a no-op and the
> guard never fires for the word that contains skip_to_level, so its stale
> high bits leak into the dissimilarity word and can steer the walk down
> the wrong descendant.
>
> Test sc_level + ASSOC_ARRAY_KEY_CHUNK_SIZE > skip_to_level directly; an
> exact-multiple skip_to_level ends on the boundary and stays untrimmed.
>
> Fixes: 3cb989501c26 ("Add a generic associative array implementation.")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> lib/assoc_array.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> index bcc6e0a013eb8..1de2c337f8fcf 100644
> --- a/lib/assoc_array.c
> +++ b/lib/assoc_array.c
> @@ -255,7 +255,7 @@ assoc_array_walk(const struct assoc_array *array,
> sc_segments = shortcut->index_key[sc_level >> ASSOC_ARRAY_KEY_CHUNK_SHIFT];
> dissimilarity = segments ^ sc_segments;
>
> - if (round_up(sc_level, ASSOC_ARRAY_KEY_CHUNK_SIZE) > shortcut->skip_to_level) {
> + if (sc_level + ASSOC_ARRAY_KEY_CHUNK_SIZE > shortcut->skip_to_level) {
> /* Trim segments that are beyond the shortcut */
> int shift = shortcut->skip_to_level & ASSOC_ARRAY_KEY_CHUNK_MASK;
> dissimilarity &= ~(ULONG_MAX << shift);
> --
> 2.53.0
>
Ditto.
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 2/3] keys: make keyring key-chunk byte order agree with keyring_diff_objects()
From: Jarkko Sakkinen @ 2026-07-18 18:31 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
Andrew Morton, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260712014500.480410-3-michael.bommarito@gmail.com>
On Sat, Jul 11, 2026 at 09:44:59PM -0400, Michael Bommarito wrote:
> keyring_get_key_chunk() loads description bytes into the index chunk low
> address first, while keyring_diff_objects() numbers the first differing
> bit from the low end and folds the absolute byte index into the level
> without removing the inline-prefix offset the level already carries.
> The two disagree on byte order and bit position, so the array can be
> told two keys first differ at a bit that does not differ in the chunk
> the walker uses, letting crafted descriptions collide into one node.
>
> Load the chunk in the order keyring_diff_objects() assumes and drop the
> inline-prefix length when folding the byte index into the level. This
> only changes the in-memory ordering used to place keys within a keyring;
> add, search and read of non-colliding keys are unaffected.
>
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> security/keys/keyring.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 1739373172ad5..e7066893e6ffc 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -292,9 +292,10 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> desc_len -= offset;
> if (desc_len > n)
> desc_len = n;
> + d += desc_len;
> do {
> chunk <<= 8;
> - chunk |= *d++;
> + chunk |= *--d;
> } while (--desc_len > 0);
> return chunk;
> }
> @@ -375,7 +376,7 @@ static int keyring_diff_objects(const void *object, const void *data)
> return -1;
>
> differ_plus_i:
> - level += i;
> + level += i - (int)sizeof(a->desc);
> differ:
> i = level * 8 + __ffs(seg_a ^ seg_b);
> return i;
> --
> 2.53.0
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Add to +1 version.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 1/3] keys: fix out-of-bounds read in keyring_get_key_chunk()
From: Jarkko Sakkinen @ 2026-07-18 18:29 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
Andrew Morton, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260712014500.480410-2-michael.bommarito@gmail.com>
On Sat, Jul 11, 2026 at 09:44:58PM -0400, Michael Bommarito wrote:
> For description-level chunks keyring_get_key_chunk() advances the read
> pointer by level * sizeof(long) past the inline prefix but only
> bounds-checks the prefix, so a long enough key description is read past
> its kmemdup(desc, desc_len + 1) allocation. Compute the full byte
> offset and bounds-check the description against it before reading.
>
> The walk only reaches a description-level chunk when two keys collide
> through the hash, x, type and domain_tag chunks, so this is reached from
> an unprivileged add_key(2) with a crafted pair of same-type keys whose
> index hashes collide; KASAN reports a slab-out-of-bounds read.
>
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> security/keys/keyring.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> KASAN, x86_64: add_key(2) of a crafted hash-colliding "user"-key pair
> (~63-byte descriptions) reports
>
> BUG: KASAN: slab-out-of-bounds in keyring_get_key_chunk
> keyring_get_key_chunk <- assoc_array_insert <- __key_link_begin
> <- __do_sys_add_key
>
> reading one byte past the description allocation; the same trigger is
> KASAN-clean with this patch. On a kernel built without init-on-alloc,
> reading the colliding keyring back with KEYCTL_READ returns
> uninitialized slab until patches 2 and 3 are applied too. Trigger
> available off-list.
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 7a2ee0ded7c93..1739373172ad5 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -270,7 +270,7 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> const struct keyring_index_key *index_key = data;
> unsigned long chunk = 0;
> const u8 *d;
> - int desc_len = index_key->desc_len, n = sizeof(chunk);
> + int desc_len = index_key->desc_len, n = sizeof(chunk), offset;
Just a nut but this is quite nasty looking statement to begin with, so
at least I would not extend it.
I'd add "int offset" to its own line. Maybe for the sake of clarity it
should be also unsigned int unless negative values hold some merit.
>
> level /= ASSOC_ARRAY_KEY_CHUNK_SIZE;
> switch (level) {
> @@ -284,12 +284,12 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
> return (unsigned long)index_key->domain_tag;
> default:
> level -= 4;
> - if (desc_len <= sizeof(index_key->desc))
> + offset = sizeof(index_key->desc) + level * sizeof(long);
> + if (desc_len <= offset)
> return 0;
>
> - d = index_key->description + sizeof(index_key->desc);
> - d += level * sizeof(long);
> - desc_len -= sizeof(index_key->desc);
> + d = index_key->description + offset;
> + desc_len -= offset;
> if (desc_len > n)
> desc_len = n;
> do {
> --
> 2.53.0
>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH 0/3] keys: fix keyring assoc-array out-of-bounds read and index inconsistency
From: Jarkko Sakkinen @ 2026-07-18 18:23 UTC (permalink / raw)
To: Michael Bommarito
Cc: David Howells, Paul Moore, James Morris, Serge E . Hallyn,
Andrew Morton, keyrings, linux-security-module, linux-kernel
In-Reply-To: <20260712014500.480410-1-michael.bommarito@gmail.com>
On Sat, Jul 11, 2026 at 09:44:57PM -0400, Michael Bommarito wrote:
> keyring_get_key_chunk() advances the description read pointer by
> level * sizeof(long) past the inline prefix but only bounds-checks the
> prefix, so once the associative-array walk reaches a description-level
> chunk it reads past the kmemdup(desc, desc_len + 1) description
> allocation. Reaching that depth needs two keys that collide through the
> hash, x, type and domain_tag chunks, which an unprivileged add_key(2)
> can arrange with a crafted pair of same-type keys.
Thanks for the description. I fully get the scenario from this.
>
> An unprivileged user can thus read up to sizeof(long) bytes past a
> keyring key's description; on kernels built without init-on-alloc the
> same collision, read back with KEYCTL_READ, returns uninitialized kernel
> slab.
>
> Patch 1 is the memory-safety fix and stands alone. Patches 2 and 3 fix
> two index-key consistency bugs that let the crafted keys collide into a
> single malformed node in the first place, which is what enables the
> KEYCTL_READ disclosure.
>
> The KASAN reproduction is on patch 1. Trigger is available off-list.
>
> Michael Bommarito (3):
> keys: fix out-of-bounds read in keyring_get_key_chunk()
> keys: make keyring key-chunk byte order agree with
> keyring_diff_objects()
> assoc_array: trim the final shortcut word when skip_to_level is
> chunk-aligned
>
> lib/assoc_array.c | 2 +-
> security/keys/keyring.c | 15 ++++++++-------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
>
> base-commit: 2c7c88a412aa6d09cd04b414211b4ef8553b5309
> --
> 2.53.0
>
BR, Jarkko
^ permalink raw reply
* Re: Landlock: LANDLOCK_ACCESS_FS_IOCTL_DEV is bypassable via io_uring IORING_OP_URING_CMD (confirmed on real NVMe hardware)
From: Mickaël Salaün @ 2026-07-18 15:01 UTC (permalink / raw)
To: Vivek Parikh
Cc: Günther Noack, Paul Moore, Jens Axboe, linux-security-module,
io-uring
In-Reply-To: <20260718135650.380643-1-viv0411.parikh@gmail.com>
Hi Vivek,
Similar reports were already sent:
https://lore.kernel.org/all/20260616201633.275067-1-hexlabsecurity@proton.me/
Please take a look at the Landlock threat model:
https://lore.kernel.org/all/20260707210336.2060040-1-mic@digikod.net/
This is not a bypass.
Regards,
Mickaël
On Sat, Jul 18, 2026 at 07:26:48PM +0530, Vivek Parikh wrote:
> Hi Mickaël,
>
> Note: this was found with AI assistance, so I am treating it as public per
> Documentation/process/security-bugs.
>
> While continuing the LSM-mediation audit I found that Landlock's
> LANDLOCK_ACCESS_FS_IOCTL_DEV right can be bypassed with io_uring's
> IORING_OP_URING_CMD. Unlike the mount_setattr(2) gap I reported earlier,
> this one is fully unprivileged and squarely inside Landlock's documented
> model. I have verified it on real NVMe hardware (output below).
>
> The mechanism
> -------------
> Landlock enforces IOCTL_DEV through the file_ioctl / file_ioctl_compat LSM
> hooks (security/landlock/fs.c: LSM_HOOK_INIT(file_ioctl, ...) /
> file_ioctl_compat -> hook_file_ioctl_common ->
> LANDLOCK_ACCESS_FS_IOCTL_DEV, security/landlock/fs.c:1854/1865).
>
> io_uring's IORING_OP_URING_CMD dispatches driver passthrough commands
> through a *different* hook, security_uring_cmd(ioucmd)
> (io_uring/uring_cmd.c:249), before calling file->f_op->uring_cmd. Landlock
> implements no uring_cmd hook -- it has no io_uring hooks at all (only
> SELinux and Smack implement security_uring_cmd). So for the same device
> fd:
>
> ioctl(devfd, CMD, arg) -> security_file_ioctl -> Landlock: DENIED
> (IOCTL_DEV not granted)
> IORING_OP_URING_CMD(devfd) -> security_uring_cmd -> Landlock: NO HOOK
> -> f_op->uring_cmd runs
>
> uring_cmd is the async twin of the device ioctl. NVMe makes the
> equivalence explicit (drivers/nvme/host/ioctl.c): the ioctl path handles
> NVME_IOCTL_ADMIN_CMD and the uring_cmd path handles NVME_URING_CMD_ADMIN
> -- the same admin/IO passthrough commands. Both the controller char dev
> (nvme_dev_uring_cmd, core.c:3841) and the namespace char dev /dev/ngX
> (nvme_ns_chr_uring_cmd, core.c:3946) implement ->uring_cmd, as do ublk
> and drivers/char/mem.c.
>
> Impact
> ------
> A Landlock-sandboxed task that is denied IOCTL_DEV on a device but holds
> an fd to it (opened under a granted fs right, or inherited) can issue the
> equivalent device commands via IORING_OP_URING_CMD, defeating exactly what
> IOCTL_DEV exists to gate. On NVMe -- the most common storage device on
> Linux systems -- this means arbitrary admin passthrough: the same code
> path carries FORMAT NVM, SANITIZE, and firmware-download commands, not
> just reads/writes. The realistic scenario: a sandboxed storage workload
> is given an NVMe namespace fd for fast IO with the expectation "it can do
> IO but cannot send device-control commands" -- the expectation
> LANDLOCK_ACCESS_FS_IOCTL_DEV was added (ABI 5) to express. It is void.
>
> For ublk devices the gap is total: ublk's control plane is uring_cmd-only
> (no ioctl equivalent), so on ublk char devices IOCTL_DEV currently
> mediates nothing at all.
>
> Documentation/userspace-api/landlock.rst presents IOCTL_DEV as the control
> over device ioctls; a sandbox author reasonably expects withholding it to
> stop device control commands. io_uring is unprivileged, Landlock is
> unprivileged, and no capability is required anywhere.
>
> Affected versions
> -----------------
> This is not a regression -- it is a coverage gap that shipped with the
> IOCTL_DEV right. LANDLOCK_ACCESS_FS_IOCTL_DEV was added in v6.10
> (b25f7415eb41, May 2024); NVMe/ublk ->uring_cmd predate it (v6.0), so the
> first kernel to support IOCTL_DEV was already bypassable. It is present in
> every kernel from v6.10 through v7.2-rc3, including 6.12 LTS. (SELinux and
> Smack implement security_uring_cmd and are unaffected; Landlock and AppArmor
> do not, but only Landlock exposes IOCTL_DEV as a user-facing right.)
>
> For completeness on scope: exploitation requires a Landlock policy that
> handles IOCTL_DEV, a sandboxee that holds/opens a ->uring_cmd-capable device
> fd, and io_uring not otherwise blocked. Sandboxers that also seccomp-filter
> io_uring (e.g. Chromium) are not affected via this path; the exposure is for
> the growing set of tools that adopt IOCTL_DEV without blocking io_uring.
>
> Reproducer (confirmed on real hardware)
> ---------------------------------------
> Three self-contained PoCs (raw io_uring, no liburing) are available on
> request; I am not inlining them.
>
> PoC 1 targets /dev/null (null_fops has no ->unlocked_ioctl but has
> .uring_cmd = uring_cmd_null) and needs no special hardware:
>
> [*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
> [1] ioctl(/dev/null, dev-cmd) = -1 (Permission denied) <- Landlock DENIED
> [2] io_uring URING_CMD(/dev/null) res = 0 (OK) <- Landlock did NOT mediate
>
> PoC 2 targets a real NVMe controller (/dev/nvme0, WD PC SN740) with
> IDENTIFY CONTROLLER (admin opcode 0x06, read-only), on
> 7.0.0-27-generic:
>
> [*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
> [1] ioctl(NVME_IOCTL_ADMIN_CMD identify) = -1 (Permission denied) <- Landlock DENIED
> [2] URING_CMD(NVME_URING_CMD_ADMIN identify) res = 0 (OK)
> IDENTIFY data: vid=0xb715 model="WD PC SN740 SDDPMQD-512G-1101"
>
> The same PoC was also confirmed on AWS EC2 (Ubuntu 7.0.0-1008-aws, stock
> cloud image, default settings, real NVMe EBS volume):
>
> [1] ioctl(NVME_IOCTL_ADMIN_CMD identify) = -1 (Permission denied) <- Landlock DENIED
> [2] URING_CMD(NVME_URING_CMD_ADMIN identify) res = 0 (OK)
> IDENTIFY data: vid=0x0f1d model="Amazon Elastic Block Store"
>
> PoC 3 targets /dev/fuse (world-accessible, 0666) as a fully unprivileged
> user (fresh uid, no group memberships, Landlock ABI 8; also reproduced in
> a Docker container with seccomp relaxed). No fuse module parameter is
> needed for this signal: fuse_uring_cmd() (fs/fuse/dev_uring.c:1217) calls
> fuse_get_dev() *before* its enable_uring check, so a never-mounted fd
> returns -EPERM even with enable_uring=N (the default). The full
> queue-registration scenario does need enable_uring=1; NVMe and ublk have
> no such gate at all.
>
> [*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
> [1] ioctl(/dev/fuse, FUSE_DEV_IOC_CLONE) = -1 (Permission denied) <- Landlock DENIED
> [2] URING_CMD(/dev/fuse, FUSE_IO_URING_CMD_REGISTER) res = -1 (EPERM)
> <- reached fuse_uring_cmd; Landlock did NOT mediate
>
> (-EPERM can only originate inside fuse_uring_cmd on a never-mounted
> fd, proving the call passed security_uring_cmd into the driver.)
>
> The ioctl admin passthrough is denied while the identical admin command
> executes via io_uring. (Note: NVMe uring passthrough requires
> SQE128+CQE32, per nvme_uring_cmd_checks.)
>
> Mitigations / not affected
> --------------------------
> The bypass is neutralised anywhere io_uring cannot be reached:
>
> - kernel.io_uring_disabled = 2 makes io_uring_setup() return -EPERM for all
> callers (io_uring_allowed(), io_uring/io_uring.c); value 1 restricts it to
> io_uring_group / CAP_SYS_ADMIN. Where an admin has set either, this path is
> blocked. This is a hardening knob, not a universal default: RHEL 9.3 / Rocky
> 9.3 ship io_uring *enabled* on the host (Red Hat documents the syscalls as
> succeeding or returning EPERM per configuration).
> - Many container runtimes block the io_uring syscalls in their default seccomp
> profile (e.g. Docker/Podman -> io_uring_setup fails with EPERM/ENOSYS inside
> the container, verified with Docker 29 on Fedora 42), so a sandboxee confined
> by such a runtime is protected. Likewise application sandboxers that
> seccomp-filter io_uring (e.g. Chromium) are not affected via this path.
> Additionally, on SELinux-enforcing hosts, containers without a relaxed label
> are denied io_uring_setup by selinux_uring_allowed -- a second independent
> gate (verified: the same container run fails with EACCES until
> --security-opt label=disable is given).
>
> So the exposed population is: io_uring-enabled kernels (the desktop default,
> and RHEL/Rocky on a bare host) running a Landlock sandbox that handles
> IOCTL_DEV without an io_uring seccomp block.
>
> Fix direction
> -------------
> security_uring_cmd(ioucmd) gives the LSM the io_uring_cmd (and thus the
> struct file). Landlock should implement a uring_cmd hook that, for a
> device file, requires LANDLOCK_ACCESS_FS_IOCTL_DEV -- mirroring
> hook_file_ioctl. The uring_cmd command is driver-specific rather than a
> standard ioctl cmd number, so the is_masked_device_ioctl() allow-list
> (FIONREAD etc.) does not apply; the safe behavior is to require IOCTL_DEV
> for any uring_cmd on a device file. I am happy to prepare that patch
> (LSM_HOOK_INIT(uring_cmd, ...) + a tools/testing/selftests/landlock test)
> if you agree with the direction.
>
> CCing Jens and io-uring, as the io_uring side is involved (a new LSM hook
> consumer, no io_uring behavior change expected).
>
> Thanks,
> Vivek
>
^ permalink raw reply
* Landlock: LANDLOCK_ACCESS_FS_IOCTL_DEV is bypassable via io_uring IORING_OP_URING_CMD (confirmed on real NVMe hardware)
From: Vivek Parikh @ 2026-07-18 13:56 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Günther Noack, Paul Moore, Jens Axboe, linux-security-module,
io-uring, viv0411.parikh
Hi Mickaël,
Note: this was found with AI assistance, so I am treating it as public per
Documentation/process/security-bugs.
While continuing the LSM-mediation audit I found that Landlock's
LANDLOCK_ACCESS_FS_IOCTL_DEV right can be bypassed with io_uring's
IORING_OP_URING_CMD. Unlike the mount_setattr(2) gap I reported earlier,
this one is fully unprivileged and squarely inside Landlock's documented
model. I have verified it on real NVMe hardware (output below).
The mechanism
-------------
Landlock enforces IOCTL_DEV through the file_ioctl / file_ioctl_compat LSM
hooks (security/landlock/fs.c: LSM_HOOK_INIT(file_ioctl, ...) /
file_ioctl_compat -> hook_file_ioctl_common ->
LANDLOCK_ACCESS_FS_IOCTL_DEV, security/landlock/fs.c:1854/1865).
io_uring's IORING_OP_URING_CMD dispatches driver passthrough commands
through a *different* hook, security_uring_cmd(ioucmd)
(io_uring/uring_cmd.c:249), before calling file->f_op->uring_cmd. Landlock
implements no uring_cmd hook -- it has no io_uring hooks at all (only
SELinux and Smack implement security_uring_cmd). So for the same device
fd:
ioctl(devfd, CMD, arg) -> security_file_ioctl -> Landlock: DENIED
(IOCTL_DEV not granted)
IORING_OP_URING_CMD(devfd) -> security_uring_cmd -> Landlock: NO HOOK
-> f_op->uring_cmd runs
uring_cmd is the async twin of the device ioctl. NVMe makes the
equivalence explicit (drivers/nvme/host/ioctl.c): the ioctl path handles
NVME_IOCTL_ADMIN_CMD and the uring_cmd path handles NVME_URING_CMD_ADMIN
-- the same admin/IO passthrough commands. Both the controller char dev
(nvme_dev_uring_cmd, core.c:3841) and the namespace char dev /dev/ngX
(nvme_ns_chr_uring_cmd, core.c:3946) implement ->uring_cmd, as do ublk
and drivers/char/mem.c.
Impact
------
A Landlock-sandboxed task that is denied IOCTL_DEV on a device but holds
an fd to it (opened under a granted fs right, or inherited) can issue the
equivalent device commands via IORING_OP_URING_CMD, defeating exactly what
IOCTL_DEV exists to gate. On NVMe -- the most common storage device on
Linux systems -- this means arbitrary admin passthrough: the same code
path carries FORMAT NVM, SANITIZE, and firmware-download commands, not
just reads/writes. The realistic scenario: a sandboxed storage workload
is given an NVMe namespace fd for fast IO with the expectation "it can do
IO but cannot send device-control commands" -- the expectation
LANDLOCK_ACCESS_FS_IOCTL_DEV was added (ABI 5) to express. It is void.
For ublk devices the gap is total: ublk's control plane is uring_cmd-only
(no ioctl equivalent), so on ublk char devices IOCTL_DEV currently
mediates nothing at all.
Documentation/userspace-api/landlock.rst presents IOCTL_DEV as the control
over device ioctls; a sandbox author reasonably expects withholding it to
stop device control commands. io_uring is unprivileged, Landlock is
unprivileged, and no capability is required anywhere.
Affected versions
-----------------
This is not a regression -- it is a coverage gap that shipped with the
IOCTL_DEV right. LANDLOCK_ACCESS_FS_IOCTL_DEV was added in v6.10
(b25f7415eb41, May 2024); NVMe/ublk ->uring_cmd predate it (v6.0), so the
first kernel to support IOCTL_DEV was already bypassable. It is present in
every kernel from v6.10 through v7.2-rc3, including 6.12 LTS. (SELinux and
Smack implement security_uring_cmd and are unaffected; Landlock and AppArmor
do not, but only Landlock exposes IOCTL_DEV as a user-facing right.)
For completeness on scope: exploitation requires a Landlock policy that
handles IOCTL_DEV, a sandboxee that holds/opens a ->uring_cmd-capable device
fd, and io_uring not otherwise blocked. Sandboxers that also seccomp-filter
io_uring (e.g. Chromium) are not affected via this path; the exposure is for
the growing set of tools that adopt IOCTL_DEV without blocking io_uring.
Reproducer (confirmed on real hardware)
---------------------------------------
Three self-contained PoCs (raw io_uring, no liburing) are available on
request; I am not inlining them.
PoC 1 targets /dev/null (null_fops has no ->unlocked_ioctl but has
.uring_cmd = uring_cmd_null) and needs no special hardware:
[*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
[1] ioctl(/dev/null, dev-cmd) = -1 (Permission denied) <- Landlock DENIED
[2] io_uring URING_CMD(/dev/null) res = 0 (OK) <- Landlock did NOT mediate
PoC 2 targets a real NVMe controller (/dev/nvme0, WD PC SN740) with
IDENTIFY CONTROLLER (admin opcode 0x06, read-only), on
7.0.0-27-generic:
[*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
[1] ioctl(NVME_IOCTL_ADMIN_CMD identify) = -1 (Permission denied) <- Landlock DENIED
[2] URING_CMD(NVME_URING_CMD_ADMIN identify) res = 0 (OK)
IDENTIFY data: vid=0xb715 model="WD PC SN740 SDDPMQD-512G-1101"
The same PoC was also confirmed on AWS EC2 (Ubuntu 7.0.0-1008-aws, stock
cloud image, default settings, real NVMe EBS volume):
[1] ioctl(NVME_IOCTL_ADMIN_CMD identify) = -1 (Permission denied) <- Landlock DENIED
[2] URING_CMD(NVME_URING_CMD_ADMIN identify) res = 0 (OK)
IDENTIFY data: vid=0x0f1d model="Amazon Elastic Block Store"
PoC 3 targets /dev/fuse (world-accessible, 0666) as a fully unprivileged
user (fresh uid, no group memberships, Landlock ABI 8; also reproduced in
a Docker container with seccomp relaxed). No fuse module parameter is
needed for this signal: fuse_uring_cmd() (fs/fuse/dev_uring.c:1217) calls
fuse_get_dev() *before* its enable_uring check, so a never-mounted fd
returns -EPERM even with enable_uring=N (the default). The full
queue-registration scenario does need enable_uring=1; NVMe and ublk have
no such gate at all.
[*] Landlock enforced: READ/WRITE granted on /, IOCTL_DEV denied
[1] ioctl(/dev/fuse, FUSE_DEV_IOC_CLONE) = -1 (Permission denied) <- Landlock DENIED
[2] URING_CMD(/dev/fuse, FUSE_IO_URING_CMD_REGISTER) res = -1 (EPERM)
<- reached fuse_uring_cmd; Landlock did NOT mediate
(-EPERM can only originate inside fuse_uring_cmd on a never-mounted
fd, proving the call passed security_uring_cmd into the driver.)
The ioctl admin passthrough is denied while the identical admin command
executes via io_uring. (Note: NVMe uring passthrough requires
SQE128+CQE32, per nvme_uring_cmd_checks.)
Mitigations / not affected
--------------------------
The bypass is neutralised anywhere io_uring cannot be reached:
- kernel.io_uring_disabled = 2 makes io_uring_setup() return -EPERM for all
callers (io_uring_allowed(), io_uring/io_uring.c); value 1 restricts it to
io_uring_group / CAP_SYS_ADMIN. Where an admin has set either, this path is
blocked. This is a hardening knob, not a universal default: RHEL 9.3 / Rocky
9.3 ship io_uring *enabled* on the host (Red Hat documents the syscalls as
succeeding or returning EPERM per configuration).
- Many container runtimes block the io_uring syscalls in their default seccomp
profile (e.g. Docker/Podman -> io_uring_setup fails with EPERM/ENOSYS inside
the container, verified with Docker 29 on Fedora 42), so a sandboxee confined
by such a runtime is protected. Likewise application sandboxers that
seccomp-filter io_uring (e.g. Chromium) are not affected via this path.
Additionally, on SELinux-enforcing hosts, containers without a relaxed label
are denied io_uring_setup by selinux_uring_allowed -- a second independent
gate (verified: the same container run fails with EACCES until
--security-opt label=disable is given).
So the exposed population is: io_uring-enabled kernels (the desktop default,
and RHEL/Rocky on a bare host) running a Landlock sandbox that handles
IOCTL_DEV without an io_uring seccomp block.
Fix direction
-------------
security_uring_cmd(ioucmd) gives the LSM the io_uring_cmd (and thus the
struct file). Landlock should implement a uring_cmd hook that, for a
device file, requires LANDLOCK_ACCESS_FS_IOCTL_DEV -- mirroring
hook_file_ioctl. The uring_cmd command is driver-specific rather than a
standard ioctl cmd number, so the is_masked_device_ioctl() allow-list
(FIONREAD etc.) does not apply; the safe behavior is to require IOCTL_DEV
for any uring_cmd on a device file. I am happy to prepare that patch
(LSM_HOOK_INIT(uring_cmd, ...) + a tools/testing/selftests/landlock test)
if you agree with the direction.
CCing Jens and io-uring, as the io_uring side is involved (a new LSM hook
consumer, no io_uring behavior change expected).
Thanks,
Vivek
^ permalink raw reply
* Landlock: mount_setattr(2) is unmediated by LSMs (ro-mount confinement bypass)
From: Vivek Parikh @ 2026-07-18 10:14 UTC (permalink / raw)
To: Mickaël Salaün, Christian Brauner, Alexander Viro
Cc: Günther Noack, Paul Moore, linux-security-module,
linux-fsdevel, Vivek Parikh
Hi Mickaël, Christian,
Note: this was found with AI assistance, so I am treating it as public per
Documentation/process/security-bugs.
While auditing Landlock's filesystem-topology restrictions I found that
mount_setattr(2) is not mediated by any LSM. It only matters for a
sandboxed task that holds CAP_SYS_ADMIN in its own user namespace -- the
rootful-container / userns-root profile that landlock_restrict_self()
explicitly supports (security/landlock/syscalls.c). Fully unprivileged
callers are not affected.
do_mount_setattr() (fs/namespace.c:4928) -> mount_setattr_prepare() has no
security_* hook anywhere on its path, whereas mount(2), move_mount(2),
umount(2), remount and pivot_root(2) all do (security_sb_mount,
security_move_mount, security_sb_umount, security_sb_remount,
security_sb_pivotroot). Landlock hooks exactly those five in
security/landlock/fs.c (hook_sb_mount, hook_move_mount, hook_sb_umount,
hook_sb_remount, hook_sb_pivotroot) but cannot see mount_setattr(2).
Documentation/userspace-api/landlock.rst ("Filesystem topology
modification") states that sandboxed threads cannot modify filesystem
topology, but such a task can still, via mount_setattr(2):
1. Clear MOUNT_ATTR_RDONLY on a read-only (bind) mount and write through
it -- subverting the common ro-bind-mount confinement pattern.
2. Change mount propagation (shared/private/slave/unbindable).
3. Request MOUNT_ATTR_IDMAP changes (narrower in practice: gated by
can_idmap_mount()).
This is a mediation/coverage gap, not a rule-evaluation bug: Landlock's
filesystem access-rights checks still apply on top. The issue is that the
ro-mount / propagation / idmap layer of a confinement -- which sandbox
setups rely on -- is changeable despite the documented topology
restriction. It affects every LSM, not just Landlock (SELinux, AppArmor
and Smack cannot mediate mount_setattr(2) either).
A self-contained unprivileged reproducer (userns+mountns, no external
privilege) is available on request; I am not inlining it here. Its output,
on 7.0.0-27-generic (host) and reproduced on 7.2.0-rc3 (QEMU guest,
CONFIG_SECURITY_LANDLOCK=y):
[1] write via ro mount before Landlock: Read-only file system (expected)
[2] Landlock enforced (all fs accesses allowed via rule on /)
[3] mount(2) under Landlock: Operation not permitted (expected EPERM)
[4] mount_setattr(rw-flip) under Landlock: SUCCESS <-- gap
[5] write via formerly-ro mount: SUCCEEDED <-- confinement subverted
mount(2) is correctly denied (step 3) while the equivalent attribute
change via mount_setattr(2) succeeds (step 4) and makes the previously
read-only tree writable (step 5).
I could not find an existing LSM hook for this in v7.2-rc3. If this is a
known and accepted limitation (the v30 Landlock series was synchronized
with mount_setattr(2)), then documenting it in landlock.rst -- the
"Filesystem topology modification" section currently names only mount(2)
and pivot_root(2) -- would already help. Otherwise, the consistent fix
would be to add a security_sb_mount_setattr() LSM hook in
do_mount_setattr() and wire Landlock's topology denial to it, mirroring
hook_sb_remount(). I am happy to prepare that patch (plus a
tools/testing/selftests/landlock/ test) if you agree with the direction.
Thanks,
Vivek
^ permalink raw reply
* Re: [PATCH] apparmor: leverage audit_log_n_untrustedstring() when possible
From: Ryan Lee @ 2026-07-17 23:52 UTC (permalink / raw)
To: Paul Moore; +Cc: Georgia Garcia, John Johansen, linux-security-module, apparmor
In-Reply-To: <CAHC9VhQNthghRo8S-w8BQhe46ur8vhUyzW+xkOngAJQUb6O-Ug@mail.gmail.com>
On Fri, Jul 17, 2026 at 2:58 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jul 17, 2026 at 5:54 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jul 17, 2026 at 5:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > Make use of the audit_log_n_untrustedstring() function to simplify the
> > > code in aa_label_xaudit().
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > > security/apparmor/label.c | 5 +----
> > > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > Untested beyond a basic compile, but I noticed this while looking at
> > something else (unrelated) and wanted to send it to the list before I
> > forgot about it ...
>
> My apologies Georgia, I thought I had copied your email into the
> original posting but it appears I copied John's email twice into my
> posting script (which helpfully de-duped it).
>
> Sorry about that.
>
> > > diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> > > index 3fd384d8c41a..a165cadf8249 100644
> > > --- a/security/apparmor/label.c
> > > +++ b/security/apparmor/label.c
> > > @@ -1743,10 +1743,7 @@ void aa_label_xaudit(struct audit_buffer *ab, struct aa_ns *ns,
> > > str = (char *) label->hname;
> > > len = strlen(str);
> > > }
> > > - if (audit_string_contains_control(str, len))
> > > - audit_log_n_hex(ab, str, len);
> > > - else
> > > - audit_log_n_string(ab, str, len);
> > > + audit_log_n_untrustedstring(ab, str, len);
> > >
> > > kfree(name);
> > > }
> > > --
> > > 2.55.0
>
> --
> paul-moore.com
>
Reviewed-By: Ryan Lee <ryan.lee@canonical.com>
^ permalink raw reply
* [PATCH v2 3/3] landlock: Document LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
From: Justin Suess @ 2026-07-17 22:03 UTC (permalink / raw)
To: gnoack3000, mic; +Cc: linux-kernel, linux-security-module, Justin Suess
In-Reply-To: <20260717220320.1030123-1-utilityemal77@gmail.com>
Document atomically setting no_new_privs with ruleset enforcement,
following the same compatibility section style as previous ABI
additions.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
Documentation/userspace-api/landlock.rst | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 5a63d4476c1c..ec87d35f4715 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -8,7 +8,7 @@ Landlock: unprivileged access control
=====================================
:Author: Mickaël Salaün
-:Date: June 2026
+:Date: July 2026
The goal of Landlock is to enable restriction of ambient rights (e.g. global
filesystem or network access) for a set of processes. Because Landlock
@@ -789,6 +789,18 @@ when at least one sys_landlock_add_rule() call is made for it with the
``LANDLOCK_ADD_RULE_QUIET`` flag, additional add-rule calls for the same
object without this flag do not clear it.
+Atomic no_new_privs (ABI < 11)
+------------------------------
+
+Starting with the Landlock ABI version 11, sys_landlock_restrict_self()
+accepts the ``LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS`` flag, which sets the
+no_new_privs attribute of the calling thread atomically with the enforcement
+of the ruleset: no_new_privs is set if and only if the call succeeds. This
+removes the need for a prior :manpage:`prctl(2)` ``PR_SET_NO_NEW_PRIVS``
+call, and with it the ``CAP_SYS_ADMIN`` requirement. When combined with
+``LANDLOCK_RESTRICT_SELF_TSYNC``, no_new_privs is set on all threads of the
+process.
+
.. _kernel_support:
Kernel support
--
2.54.0
^ permalink raw reply related
* [PATCH v2 2/3] selftests/landlock: Test LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
From: Justin Suess @ 2026-07-17 22:03 UTC (permalink / raw)
To: gnoack3000, mic; +Cc: linux-kernel, linux-security-module, Justin Suess
In-Reply-To: <20260717220320.1030123-1-utilityemal77@gmail.com>
Check that a successful landlock_restrict_self(2) call with
LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS sets no_new_privs without a prior
prctl(2) call nor CAP_SYS_ADMIN, that a failed call leaves the attribute
unchanged, and that LANDLOCK_RESTRICT_SELF_TSYNC extends it to sibling
threads. Also check that this flag requires a ruleset, and update the
restrict_self_checks_ordering EPERM checks since this flag is now
checked before the flags validity.
Update the ABI version and last-flag checks accordingly.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
tools/testing/selftests/landlock/base_test.c | 65 +++++++++++++++++--
tools/testing/selftests/landlock/tsync_test.c | 33 ++++++++++
2 files changed, 93 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index cbd3c1669951..2d4903588903 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -76,7 +76,7 @@ TEST(abi_version)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
};
- ASSERT_EQ(10, landlock_create_ruleset(NULL, 0,
+ ASSERT_EQ(11, landlock_create_ruleset(NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION));
ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
@@ -255,8 +255,15 @@ TEST(restrict_self_checks_ordering)
/* Checks unprivileged enforcement without no_new_privs. */
drop_caps(_metadata);
- ASSERT_EQ(-1, landlock_restrict_self(-1, -1));
+ ASSERT_EQ(-1, landlock_restrict_self(
+ -1, ~LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
ASSERT_EQ(EPERM, errno);
+ /*
+ * LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS fulfills the no_new_privs /
+ * CAP_SYS_ADMIN requirement, so the invalid flags are checked first.
+ */
+ ASSERT_EQ(-1, landlock_restrict_self(-1, -1));
+ ASSERT_EQ(EINVAL, errno);
ASSERT_EQ(-1, landlock_restrict_self(-1, 0));
ASSERT_EQ(EPERM, errno);
ASSERT_EQ(-1, landlock_restrict_self(ruleset_fd, 0));
@@ -288,7 +295,7 @@ TEST(restrict_self_fd)
EXPECT_EQ(EBADFD, errno);
}
-TEST(restrict_self_fd_logging_flags)
+TEST(restrict_self_fd_flags)
{
int fd;
@@ -302,11 +309,16 @@ TEST(restrict_self_fd_logging_flags)
EXPECT_EQ(-1, landlock_restrict_self(
fd, LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF));
EXPECT_EQ(EBADFD, errno);
+
+ /* LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS requires a ruleset FD. */
+ EXPECT_EQ(-1, landlock_restrict_self(
+ fd, LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+ EXPECT_EQ(EBADFD, errno);
}
-TEST(restrict_self_logging_flags)
+TEST(restrict_self_flags)
{
- const __u32 last_flag = LANDLOCK_RESTRICT_SELF_TSYNC;
+ const __u32 last_flag = LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS;
/* Tests invalid flag combinations. */
@@ -349,6 +361,18 @@ TEST(restrict_self_logging_flags)
LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON));
EXPECT_EQ(EBADF, errno);
+ /* LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS requires a ruleset FD. */
+
+ EXPECT_EQ(-1, landlock_restrict_self(
+ -1, LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+ EXPECT_EQ(EBADF, errno);
+
+ EXPECT_EQ(-1,
+ landlock_restrict_self(
+ -1, LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF |
+ LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+ EXPECT_EQ(EBADF, errno);
+
/* Tests with an invalid ruleset_fd. */
EXPECT_EQ(-1, landlock_restrict_self(
@@ -359,6 +383,37 @@ TEST(restrict_self_logging_flags)
-1, LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF));
}
+TEST(restrict_self_no_new_privs)
+{
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
+ };
+ const int ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+
+ ASSERT_LE(0, ruleset_fd);
+
+ /*
+ * The calling thread does not need CAP_SYS_ADMIN nor an explicit
+ * prctl(2) PR_SET_NO_NEW_PRIVS call.
+ */
+ drop_caps(_metadata);
+ ASSERT_EQ(0, prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
+
+ /* Checks that a failed call does not set no_new_privs. */
+ EXPECT_EQ(-1, landlock_restrict_self(
+ -1, LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+ EXPECT_EQ(EBADF, errno);
+ EXPECT_EQ(0, prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
+
+ /* Checks that a successful call sets no_new_privs. */
+ ASSERT_EQ(0, landlock_restrict_self(
+ ruleset_fd, LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+ EXPECT_EQ(1, prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
+
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
TEST(ruleset_fd_io)
{
struct landlock_ruleset_attr ruleset_attr = {
diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
index 9cf1491bbaaf..d5336186b2c7 100644
--- a/tools/testing/selftests/landlock/tsync_test.c
+++ b/tools/testing/selftests/landlock/tsync_test.c
@@ -90,6 +90,39 @@ TEST(multi_threaded_success)
EXPECT_EQ(0, close(ruleset_fd));
}
+TEST(multi_threaded_no_new_privs)
+{
+ pthread_t t1, t2;
+ bool no_new_privs1, no_new_privs2;
+ const int ruleset_fd = create_ruleset(_metadata);
+
+ disable_caps(_metadata);
+
+ ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
+ ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
+
+ /* No prior prctl(2) PR_SET_NO_NEW_PRIVS call. */
+ ASSERT_EQ(0, prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
+
+ EXPECT_EQ(0, landlock_restrict_self(
+ ruleset_fd,
+ LANDLOCK_RESTRICT_SELF_TSYNC |
+ LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS));
+
+ EXPECT_EQ(1, prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0));
+
+ ASSERT_EQ(0, pthread_cancel(t1));
+ ASSERT_EQ(0, pthread_cancel(t2));
+ ASSERT_EQ(0, pthread_join(t1, NULL));
+ ASSERT_EQ(0, pthread_join(t2, NULL));
+
+ /* The no_new_privs flag was enabled on all threads. */
+ EXPECT_TRUE(no_new_privs1);
+ EXPECT_TRUE(no_new_privs2);
+
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
TEST(multi_threaded_success_despite_diverging_domains)
{
pthread_t t1, t2;
--
2.54.0
^ permalink raw reply related
* [PATCH v2 1/3] landlock: Add LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
From: Justin Suess @ 2026-07-17 22:03 UTC (permalink / raw)
To: gnoack3000, mic; +Cc: linux-kernel, linux-security-module, Justin Suess
In-Reply-To: <20260717220320.1030123-1-utilityemal77@gmail.com>
Add a landlock_restrict_self(2) flag to set the no_new_privs attribute
of the calling thread atomically with the enforcement of the ruleset:
no_new_privs is set if and only if the call succeeds. This removes the
need for a prior prctl(2) PR_SET_NO_NEW_PRIVS call and guarantees that a
failed enforcement leaves the attribute unchanged.
Because no_new_privs is set by the call itself, the no_new_privs /
CAP_SYS_ADMIN requirement of landlock_restrict_self(2) is fulfilled by
construction, and the related EPERM check is skipped. As a consequence,
an unprivileged caller passing unknown flags along with this flag gets
EINVAL instead of EPERM.
The attribute is only set past the last point of failure, just before
committing the new credentials. When combined with
LANDLOCK_RESTRICT_SELF_TSYNC, no_new_privs is set on the sibling threads
as well, in their commit phase, with the same atomicity.
Bump the Landlock ABI version to 11.
Cc: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
include/uapi/linux/landlock.h | 13 +++++++++++++
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 28 +++++++++++++++++++++-------
security/landlock/tsync.c | 8 ++++++--
security/landlock/tsync.h | 4 +++-
5 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 272f047df438..77820e430ab8 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -191,12 +191,25 @@ struct landlock_ruleset_attr {
*
* If the calling thread is running with no_new_privs, this operation
* enables no_new_privs on the sibling threads as well.
+ *
+ * The following flag ties the no_new_privs attribute to the ruleset
+ * enforcement:
+ *
+ * %LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
+ * Sets the no_new_privs attribute of the calling thread atomically with
+ * the enforcement of the ruleset: no_new_privs is set if and only if
+ * sys_landlock_restrict_self() succeeds. This removes the need for a
+ * prior :manpage:`prctl(2)` ``PR_SET_NO_NEW_PRIVS`` call, and with it the
+ * %CAP_SYS_ADMIN requirement. This flag requires a ruleset. When
+ * combined with %LANDLOCK_RESTRICT_SELF_TSYNC, no_new_privs is set on the
+ * sibling threads as well.
*/
/* clang-format off */
#define LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF (1U << 0)
#define LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON (1U << 1)
#define LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF (1U << 2)
#define LANDLOCK_RESTRICT_SELF_TSYNC (1U << 3)
+#define LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS (1U << 4)
/* clang-format on */
/**
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 08d5f2f6d321..1a7c5fb8f6fd 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -34,7 +34,7 @@
#define LANDLOCK_NUM_ACCESS_MAX \
MAX(MAX(LANDLOCK_NUM_ACCESS_FS, LANDLOCK_NUM_ACCESS_NET), LANDLOCK_NUM_SCOPE)
-#define LANDLOCK_LAST_RESTRICT_SELF LANDLOCK_RESTRICT_SELF_TSYNC
+#define LANDLOCK_LAST_RESTRICT_SELF LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
#define LANDLOCK_MASK_RESTRICT_SELF ((LANDLOCK_LAST_RESTRICT_SELF << 1) - 1)
/* clang-format on */
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 36b02892c62f..36b8a3fb506f 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -169,7 +169,7 @@ static const struct file_operations ruleset_fops = {
* If the change involves a fix that requires userspace awareness, also update
* the errata documentation in Documentation/userspace-api/landlock.rst .
*/
-const int landlock_abi_version = 10;
+const int landlock_abi_version = 11;
/**
* sys_landlock_create_ruleset - Create a new ruleset
@@ -502,21 +502,28 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
* - %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON
* - %LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF
* - %LANDLOCK_RESTRICT_SELF_TSYNC
+ * - %LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
*
* This system call enforces a Landlock ruleset on the current thread.
* Enforcing a ruleset requires that the task has %CAP_SYS_ADMIN in its
* namespace or is running with no_new_privs. This avoids scenarios where
* unprivileged tasks can affect the behavior of privileged children.
*
+ * With %LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS, the no_new_privs attribute of the
+ * calling thread is set atomically with the enforcement of the ruleset, which
+ * fulfills the above requirement: no_new_privs is set if and only if the call
+ * succeeds.
+ *
* Return: 0 on success, or -errno on failure. Possible returned errors are:
*
* - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
* - %EINVAL: @flags contains an unknown bit.
* - %EBADF: @ruleset_fd is not a file descriptor for the current thread;
* - %EBADFD: @ruleset_fd is not a ruleset file descriptor;
- * - %EPERM: @ruleset_fd has no read access to the underlying ruleset, or the
- * current thread is not running with no_new_privs, or it doesn't have
- * %CAP_SYS_ADMIN in its namespace.
+ * - %EPERM: @ruleset_fd has no read access to the underlying ruleset, or
+ * %LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS is not set while the current thread
+ * is not running with no_new_privs and doesn't have %CAP_SYS_ADMIN in its
+ * namespace.
* - %E2BIG: The maximum number of stacked rulesets is reached for the current
* thread.
*
@@ -529,6 +536,8 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
struct landlock_ruleset *ruleset __free(landlock_put_ruleset) = NULL;
struct cred *new_cred;
struct landlock_cred_security *new_llcred;
+ const bool set_no_new_privs =
+ !!(flags & LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS);
bool __maybe_unused log_same_exec, log_new_exec, log_subdomains,
prev_log_subdomains;
@@ -537,9 +546,10 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
/*
* Similar checks as for seccomp(2), except that an -EPERM may be
- * returned.
+ * returned. LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS fulfills this
+ * requirement.
*/
- if (!task_no_new_privs(current) &&
+ if (!set_no_new_privs && !task_no_new_privs(current) &&
!ns_capable_noaudit(current_user_ns(), CAP_SYS_ADMIN))
return -EPERM;
@@ -620,12 +630,16 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
if (flags & LANDLOCK_RESTRICT_SELF_TSYNC) {
const int err = landlock_restrict_sibling_threads(
- current_cred(), new_cred);
+ current_cred(), new_cred, flags);
if (err) {
abort_creds(new_cred);
return err;
}
}
+ /* Sets no_new_privs past the last point of failure. */
+ if (set_no_new_privs)
+ task_set_no_new_privs(current);
+
return commit_creds(new_cred);
}
diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
index c5730bbd9ed3..0b71e158c3f5 100644
--- a/security/landlock/tsync.c
+++ b/security/landlock/tsync.c
@@ -17,6 +17,7 @@
#include <linux/sched/task.h>
#include <linux/slab.h>
#include <linux/task_work.h>
+#include <uapi/linux/landlock.h>
#include "cred.h"
#include "tsync.h"
@@ -466,7 +467,8 @@ static void cancel_tsync_works(const struct tsync_works *works,
* restrict_sibling_threads - enables a Landlock policy for all sibling threads
*/
int landlock_restrict_sibling_threads(const struct cred *old_cred,
- const struct cred *new_cred)
+ const struct cred *new_cred,
+ const u32 restrict_flags)
{
int err;
struct tsync_shared_context shared_ctx;
@@ -481,7 +483,9 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
init_completion(&shared_ctx.all_finished);
shared_ctx.old_cred = old_cred;
shared_ctx.new_cred = new_cred;
- shared_ctx.set_no_new_privs = task_no_new_privs(current);
+ shared_ctx.set_no_new_privs =
+ (restrict_flags & LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS) ||
+ task_no_new_privs(current);
/*
* Serialize concurrent TSYNC operations to prevent deadlocks when
diff --git a/security/landlock/tsync.h b/security/landlock/tsync.h
index ef86bb61c2f6..2ae4f938ca00 100644
--- a/security/landlock/tsync.h
+++ b/security/landlock/tsync.h
@@ -9,8 +9,10 @@
#define _SECURITY_LANDLOCK_TSYNC_H
#include <linux/cred.h>
+#include <linux/types.h>
int landlock_restrict_sibling_threads(const struct cred *old_cred,
- const struct cred *new_cred);
+ const struct cred *new_cred,
+ u32 restrict_flags);
#endif /* _SECURITY_LANDLOCK_TSYNC_H */
--
2.54.0
^ permalink raw reply related
* [PATCH v2 0/3] Implement LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
From: Justin Suess @ 2026-07-17 22:03 UTC (permalink / raw)
To: gnoack3000, mic; +Cc: linux-kernel, linux-security-module, Justin Suess
Howdy
This series adds a new landlock_restrict_self(2) flag:
LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS.
This is a redesign of v1 [1], which added
LANDLOCK_RESTRICT_SELF_NNP_ON_EXEC, a flag staging no_new_privs so that
it was only set at the next execve(2). Following Mickaël's feedback,
[2] the staging mechanism (credential bit and bprm_committing_creds hook)
is dropped entirely. The new flag instead sets the no_new_privs
attribute of the calling thread atomically with the enforcement of the
ruleset: no_new_privs is set if and only if the
landlock_restrict_self(2) call succeeds.
Semantics:
A single call replaces the usual prctl(PR_SET_NO_NEW_PRIVS) +
landlock_restrict_self(2) pair. Because no_new_privs is set by the
call itself, the no_new_privs/CAP_SYS_ADMIN precondition is fulfilled
by construction, so the flag is usable by unprivileged processes. This
is safe for the same reason the prctl(2) pair is: the executed programs
can either gain privileges or be restricted, never both.
The two states cannot diverge. A failed call (invalid ruleset FD,
E2BIG, ENOMEM, interrupted TSYNC, ...) leaves no_new_privs unchanged,
and a successful call never returns without no_new_privs set: the
attribute is set past the last point of failure, right before
commit_creds(), which cannot fail.
Combined with LANDLOCK_RESTRICT_SELF_TSYNC, no_new_privs is set on all
threads with the same guarantee: each sibling thread sets it in the
commit phase of the TSYNC protocol, after its all-or-nothing barrier,
so either every thread gets both the domain and no_new_privs, or none
does. This also makes it possible to atomically set no_new_privs
process-wide, which prctl(2) cannot do.
Unlike the v1 flag, this flag requires a ruleset: calls with a
ruleset_fd of -1 are rejected. As a consequence of the fulfilled
precondition, an unprivileged caller passing unknown flag bits together
with this flag receives EINVAL instead of EPERM; the selftests pin this
error ordering as well.
The reason why -1 ruleset_fd is rejected is basically then we are
making a Landlock-flaved prctl(nnp) call that doesn't do anything
special. It seems better to be able to have the option to define
behavior later rather than have a useless feature stuck in the
syscall abi. So we reject the -1 ruleset_fd for now.
The Landlock ABI version is bumped to 11.
Test coverage:
base_test checks that a successful call sets no_new_privs without a
prior prctl(2) nor CAP_SYS_ADMIN, that a failed call leaves it
unchanged, that the flag requires a ruleset FD, and the updated
EPERM/EINVAL ordering. tsync_test checks that TSYNC sets no_new_privs
on sibling threads along with the domain.
Changes since v1:
- Renamed LANDLOCK_RESTRICT_SELF_NNP_ON_EXEC to
LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS, per Mickaël's feedback.
- Dropped the staged "on exec" design: removed the credential bit and
the bprm_committing_creds hook; no_new_privs is now set atomically
with the ruleset enforcement.
- The flag now fulfills the no_new_privs/CAP_SYS_ADMIN precondition,
a significant departure from the v1.
- The flag now requires a ruleset (no ruleset_fd = -1). Otherwise such
a call would just == a vanilla prctl no-new-privs call. This is left
in case we want to repurpose it later rather than defining useless
redundant behavior.
[1] https://lore.kernel.org/linux-security-module/20260708133928.852999-1-utilityemal77@gmail.com/
[2] https://lore.kernel.org/linux-security-module/20260709.Eaphooyoh6sh@digikod.net/
Justin Suess (3):
landlock: Add LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
selftests/landlock: Test LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
landlock: Document LANDLOCK_RESTRICT_SELF_NO_NEW_PRIVS
Documentation/userspace-api/landlock.rst | 14 +++-
include/uapi/linux/landlock.h | 13 ++++
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 28 ++++++--
security/landlock/tsync.c | 8 ++-
security/landlock/tsync.h | 4 +-
tools/testing/selftests/landlock/base_test.c | 65 +++++++++++++++++--
tools/testing/selftests/landlock/tsync_test.c | 33 ++++++++++
8 files changed, 150 insertions(+), 17 deletions(-)
base-commit: 55f82176ef8dde632ea3eb94a6224950ed809d7c
--
2.54.0
^ permalink raw reply
* Re: [PATCH] apparmor: leverage audit_log_n_untrustedstring() when possible
From: Paul Moore @ 2026-07-17 21:57 UTC (permalink / raw)
To: Georgia Garcia; +Cc: John Johansen, linux-security-module
In-Reply-To: <CAHC9VhRcJGj3Pnrpmpi0uRi3ndcMVGGQorsU4dJ-vA_uQmRuQA@mail.gmail.com>
On Fri, Jul 17, 2026 at 5:54 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jul 17, 2026 at 5:52 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Make use of the audit_log_n_untrustedstring() function to simplify the
> > code in aa_label_xaudit().
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> > security/apparmor/label.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
>
> Untested beyond a basic compile, but I noticed this while looking at
> something else (unrelated) and wanted to send it to the list before I
> forgot about it ...
My apologies Georgia, I thought I had copied your email into the
original posting but it appears I copied John's email twice into my
posting script (which helpfully de-duped it).
Sorry about that.
> > diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> > index 3fd384d8c41a..a165cadf8249 100644
> > --- a/security/apparmor/label.c
> > +++ b/security/apparmor/label.c
> > @@ -1743,10 +1743,7 @@ void aa_label_xaudit(struct audit_buffer *ab, struct aa_ns *ns,
> > str = (char *) label->hname;
> > len = strlen(str);
> > }
> > - if (audit_string_contains_control(str, len))
> > - audit_log_n_hex(ab, str, len);
> > - else
> > - audit_log_n_string(ab, str, len);
> > + audit_log_n_untrustedstring(ab, str, len);
> >
> > kfree(name);
> > }
> > --
> > 2.55.0
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] apparmor: leverage audit_log_n_untrustedstring() when possible
From: Paul Moore @ 2026-07-17 21:54 UTC (permalink / raw)
To: linux-security-module; +Cc: John Johansen
In-Reply-To: <20260717215254.383183-2-paul@paul-moore.com>
On Fri, Jul 17, 2026 at 5:52 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Make use of the audit_log_n_untrustedstring() function to simplify the
> code in aa_label_xaudit().
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> security/apparmor/label.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
Untested beyond a basic compile, but I noticed this while looking at
something else (unrelated) and wanted to send it to the list before I
forgot about it ...
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index 3fd384d8c41a..a165cadf8249 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -1743,10 +1743,7 @@ void aa_label_xaudit(struct audit_buffer *ab, struct aa_ns *ns,
> str = (char *) label->hname;
> len = strlen(str);
> }
> - if (audit_string_contains_control(str, len))
> - audit_log_n_hex(ab, str, len);
> - else
> - audit_log_n_string(ab, str, len);
> + audit_log_n_untrustedstring(ab, str, len);
>
> kfree(name);
> }
> --
> 2.55.0
--
paul-moore.com
^ permalink raw reply
* [PATCH] apparmor: leverage audit_log_n_untrustedstring() when possible
From: Paul Moore @ 2026-07-17 21:52 UTC (permalink / raw)
To: linux-security-module; +Cc: John Johansen
Make use of the audit_log_n_untrustedstring() function to simplify the
code in aa_label_xaudit().
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
security/apparmor/label.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index 3fd384d8c41a..a165cadf8249 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1743,10 +1743,7 @@ void aa_label_xaudit(struct audit_buffer *ab, struct aa_ns *ns,
str = (char *) label->hname;
len = strlen(str);
}
- if (audit_string_contains_control(str, len))
- audit_log_n_hex(ab, str, len);
- else
- audit_log_n_string(ab, str, len);
+ audit_log_n_untrustedstring(ab, str, len);
kfree(name);
}
--
2.55.0
^ permalink raw reply related
* Re: [GIT PULL] selinux/selinux-pr-20260717
From: pr-tracker-bot @ 2026-07-17 20:17 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <fa44478822ef189fb7a5ce16135dadd8@paul-moore.com>
The pull request you sent on Fri, 17 Jul 2026 13:03:30 -0400:
> https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20260717
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8fc574321e59a2484063b2e75016772815038608
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* [GIT PULL] selinux/selinux-pr-20260717
From: Paul Moore @ 2026-07-17 17:03 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Linus,
A single SELinux patch to correct a problem with the overlayfs mmap()
and mprotect() fixes from earlier this year where we inadvertenly
included an additional SELinux execmem permission check on some
operations. Please merge for an upcoming v7.2-rcX release.
Paul
--
The following changes since commit a13c140cc289c0b7b3770bce5b3ad42ab35074aa:
Linux 7.2-rc3 (2026-07-12 14:16:39 -0700)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20260717
for you to fetch changes up to 9fe595fad54d4ac6a402edb3f60bec859d52cea6:
selinux: fix incorrect execmem checks on overlayfs
(2026-07-14 18:10:20 -0400)
----------------------------------------------------------------
selinux/stable-7.2 PR 20260717
----------------------------------------------------------------
Ondrej Mosnacek (1):
selinux: fix incorrect execmem checks on overlayfs
security/selinux/hooks.c | 42 ++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 18 deletions(-)
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v3 1/3] landlock: Require LANDLOCK_ACCESS_FS_MAKE_WHITEOUT for RENAME_WHITEOUT
From: Günther Noack @ 2026-07-17 9:23 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Christian Brauner, linux-security-module, Paul Moore,
Amir Goldstein, Miklos Szeredi, Serge Hallyn, Stephen Smalley
In-Reply-To: <aivEo6bV_phFFJg5@google.com>
On Fri, Jun 12, 2026 at 10:34:43AM +0200, Günther Noack wrote:
> On Wed, Jun 10, 2026 at 03:38:56PM +0200, Mickaël Salaün wrote:
> > Making MAKE_CHAR not covering MAKE_WHITEOUT is not addressed (see
> > previous discussion). MAKE_CHAR should not restrict whiteout creation
> > *if* MAKE_WHITEOUT is handled.
>
> (This is option (3) from your reply to V1 [1].)
>
> I am skeptical of this approach, because it complicates how userspace
> needs to deal with this access right. Consider the following
> scenario: A program wants to install the policy:
>
> * DENY MAKE_WHITEOUT, MAKE_CHAR
> * ALLOW MAKE_WHITEOUT in /foo (path_beneath rule)
>
> Then, if the kernel ABI predates make-whiteout, with the usual
> best-effort fallback (clearing out the unsupported bits), this ruleset
> becomes:
>
> * DENY MAKE_CHAR
> * (no ALLOW rule)
>
> But this ruleset is incorrect, because it denies mknod("/foo/x",
> S_IFCHR | mode, makedev(0, 0)) in /foo, which was explicitly allowed
> in the earlier ruleset.
>
> So in order to implement the best-effort fallback, I guess userspace
> libraries would now have to take into account whether there are any
> rules where MAKE_WHITEOUT is specifically allowed, and if so, they
> can't restrict MAKE_CHAR either? I find this a bit complicated and I
> think it's foreseeable that library implementers will predominantly
> get this wrong.
>
>
> Let me circle back to the other options you mentioned in [1], quoting
> them here for reference:
> > I see four options:
> >
> > 1. Consider whiteouts as regular files and make them handled by
> > LANDLOCK_ACCESS_FS_MAKE_REG. This would require an erratum and would
> > make sense for direct mknod calls, but it would be weird for
> > renameat2 calls than move a file and should only require
> > LANDLOCK_ACCESS_FS_REMOVE_FILE from the user point of view.
>
> It would be weird for renameat2 calls to require MAKE_REG in the
> source directory, but the weirdness would only affect
> fuse-overlayfs-style programs and could be documented explicitly for
> them for the case that they start using Landlock.
>
> Normal programs that just call rename() on an existing FUSE-Overlayfs
> filesystem would *not* require the MAKE_REG right, because the FUSE
> process would do that on their behalf with the FUSE processes'
> credentials.
>
> >
> > 2. Add a new LANDLOCK_ACCESS_FS_MAKE_WHITEOUT right to handle whitout
> > creation (direct and indirect?) and keep LANDLOCK_ACCESS_FS_MAKE_CHAR
> > handle direct whiteout creation (and don't backport anything). It
> > looks inconsistent from an access control point of view.
>
> MAKE_WHITEOUT to handle rename(RENAME_WHITEOUT) and MAKE_CHAR to
> handle mknod(chardev (0, 0)) -- This is a bit inconsistent, but it
> does not make a difference for any programs other than the ones
> calling rename(RENAME_WHITEOUT) (i.e., overlayfs-fuse), and it could
> be documented for that one use case.
>
> I find this a pragmatic balance, and it does not require special logic
> for the best-effort fallback either. Could you be persuaded to go
> this route instead?
>
> > 3. Add a new LANDLOCK_ACCESS_FS_MAKE_WHITEOUT right and, when handled,
> > make LANDLOCK_ACCESS_FS_MAKE_CHAR not handle whiteout. This would be
> > a bit weird from a kernel point of view but it should work well for
> > users while still forbidding direct whiteout creation.
>
> Except for the best-effort fallback, which is IMHO prone to
> implementation bugs. (see above)
>
> On the side, the implementation of this is also non-trivial: In order
> to check for mknod(..., makedev(0, 0)), we need to check
> layer-by-layer whether the layer handles MAKE_WHITEOUT and then either
> check for MAKE_CHAR or MAKE_WHITEOUT.
>
>
> > 4. Add a new LANDLOCK_ACCESS_FS_MAKE_WHITEOUT right and make
> > LANDLOCK_ACCESS_FS_MAKE_CHAR never handle whiteout (and backport
> > MAKE_CHAR fix with an errata). This would be consistent but backport
> > a way to directly create whiteouts (e.g. with mknod).
>
> It's mostly theoretical, but lifting the mknod(chardev (0,0))
> restriction for normal mknod() calls and calling it an erratum seems
> surprising as well, because it would relax security guarantees for
> existing programs.
>
> I also pondered the alternative of creating an erratum but
> intentionally *not* backporting it, but even in that case, that
> surprising erratum still affects older programs which are deployed on
> newer kernels.
>
>
> Revisiting this discussion, I'd lean towards option 1 or 2 -- could
> you be persuaded towards one of these?
Friendly ping, Mickaël; I would like to have some feedback on this approach
before sending v4. Could you please have a look?
Thanks,
—Günther
> I have a slight preference for option 1 (using MAKE_REG) because it
> would be a narrow fix that could be backported to older kernels as
> well and would not require a new access right. Given that the use
> case for RENAME_WHITEOUT is really only for FUSE-OverlayFS and given
> that FUSE-OverlayFS anyway needs MAKE_REG permissions there, I have
> trouble imagining a scenario where a separate access right for
> MAKE_WHITEOUT is needed in a policy. It seems like a pragmatic
> choice.
>
>
> > Specific tests should check that all
> > these cases are proprely handled.
> >
> > There is no documentation update related to the new feature. A note
> > should also explain what exactly is a whiteout and why it is not
> > considered a character device (see previous discussions).
> >
> > The sandboxer is not updated.
> >
> > There is no audit tests.
>
> Acknowledged, these were missing.
>
> (I was initially hoping that this bug report wouldn't expand into a
> full-fledged feature with its own access right constant, but it is
> correct that this is all required in that case... :-/)
>
> Will add this for the next patch set revision if it is still needed.
>
> —Günther
>
> [1] https://lore.kernel.org/all/20260414.Lae5ida1eeGh@digikod.net/
^ permalink raw reply
* Re: [PATCH v5 2/3] bpf: add bpf_init_inode_xattr kfunc for atomic inode labeling
From: Paul Moore @ 2026-07-16 21:55 UTC (permalink / raw)
To: David Windsor, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Jiri Olsa,
Kumar Kartikeya Dwivedi, Emil Tsalapatis, Matt Bobrowski,
James Morris, Serge E . Hallyn, Casey Schaufler, Stephen Smalley,
Ondrej Mosnacek, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Alexander Viro, Christian Brauner, Jan Kara,
Shuah Khan
Cc: bpf, linux-security-module, linux-fsdevel, linux-integrity,
selinux, linux-kselftest, linux-kernel, David Windsor
In-Reply-To: <20260708000956.46138-3-dwindsor@gmail.com>
On Jul 7, 2026 David Windsor <dwindsor@gmail.com> wrote:
>
> Add bpf_init_inode_xattr() kfunc for BPF LSM programs to atomically set
> xattrs via the inode_init_security hook using security_lsmxattr_add().
> The hook now passes its xattr state as a single struct lsm_xattrs
> object, which the kfunc takes directly.
>
> This kfunc is only callable from inode_init_security; the verifier
> rejects attempts to call it elsewhere.
>
> A previous attempt [1] required a kmalloc string output protocol for
> the xattr name. Since commit 6bcdfd2cac55 ("security: Allow all LSMs to
> provide xattrs for inode_init_security hook") [2], the xattr name is no
> longer allocated; it is a static constant.
>
> Link: https://kernsec.org/pipermail/linux-security-module-archive/2022-October/034878.html [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6bcdfd2cac55 [2]
> Suggested-by: Song Liu <song@kernel.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
> fs/bpf_fs_kfuncs.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 768aca2dc0f0..ad0025f2264a 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -11,7 +11,9 @@
> #include <linux/file.h>
> #include <linux/kernfs.h>
> #include <linux/mm.h>
> +#include <linux/security.h>
> #include <linux/xattr.h>
> +#include <uapi/linux/lsm.h>
>
> __bpf_kfunc_start_defs();
>
> @@ -374,6 +376,39 @@ __bpf_kfunc struct inode *bpf_real_inode(struct dentry *dentry)
> return d_real_inode(dentry);
> }
>
> +/**
> + * bpf_init_inode_xattr - set an xattr on a new inode from inode_init_security
> + * @xattrs: inode_init_security xattr state from the hook context
> + * @name__str: xattr name (e.g., "bpf.file_label")
> + * @value_p: dynptr containing the xattr value
> + *
> + * Only callable from lsm/inode_init_security programs.
> + *
> + * Return: 0 on success, negative error on failure.
> + */
> +__bpf_kfunc int bpf_init_inode_xattr(struct lsm_xattrs *xattrs,
> + const char *name__str,
> + const struct bpf_dynptr *value_p)
> +{
> + struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
> + const void *value;
> + u32 value_len;
> +
> + if (!name__str)
> + return -EINVAL;
> + if (strncmp(name__str, XATTR_BPF_LSM_SUFFIX,
> + sizeof(XATTR_BPF_LSM_SUFFIX) - 1))
> + return -EPERM;
> +
> + value_len = __bpf_dynptr_size(value_ptr);
> + value = __bpf_dynptr_data(value_ptr, value_len);
> + if (!value)
> + return -EINVAL;
> +
> + return security_lsmxattr_add(xattrs, LSM_ID_BPF, name__str, value,
> + value_len);
> +}
I'm sorry David, now that I'm seeing this function again, especially
with the LSM specific bits extracted into a LSM function, this absolutely
belongs somewhere under security/. It's only callable from within a
BPF LSM callback and all it does outside of some BPF pointer boilerplate
is call right back into a LSM helper function.
If the BPF maintainers aren't willing to accept that, then we will all
need to find another way.
> __bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
> @@ -385,6 +420,7 @@ BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
> BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE)
> BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE)
> BTF_ID_FLAGS(func, bpf_real_inode, KF_SLEEPABLE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_init_inode_xattr)
> BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
>
> static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
> --
> 2.53.0
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v5 1/3] security: rework inode_init_security xattr handling
From: Paul Moore @ 2026-07-16 21:55 UTC (permalink / raw)
To: David Windsor, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Jiri Olsa,
Kumar Kartikeya Dwivedi, Emil Tsalapatis, Matt Bobrowski,
James Morris, Serge E . Hallyn, Casey Schaufler, Stephen Smalley,
Ondrej Mosnacek, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Alexander Viro, Christian Brauner, Jan Kara,
Shuah Khan
Cc: bpf, linux-security-module, linux-fsdevel, linux-integrity,
selinux, linux-kselftest, linux-kernel, David Windsor
In-Reply-To: <20260708000956.46138-2-dwindsor@gmail.com>
On Jul 7, 2026 David Windsor <dwindsor@gmail.com> wrote:
>
> In preparation for bpf_init_inode_xattr(), a kfunc that lets bpf LSM
> programs atomically label new inodes, rework how inode_init_security
> xattrs are managed.
>
> inode_init_security receives the LSM xattr array and its count as
> separate parameters. For better compatibility with the bpf verifier,
> update inode_init_security and its callers to consolidate these
> parameters into a single context object: struct lsm_xattrs.
>
> Also, add security_lsmxattr_add(), which claims a slot in the
> inode_init_security xattr array on behalf of the calling LSM and
> fills it with a copy of the given name and value.
>
> Suggested-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
> include/linux/bpf_lsm.h | 3 +
> include/linux/evm.h | 9 +--
> include/linux/lsm_hook_defs.h | 4 +-
> include/linux/lsm_hooks.h | 16 ++---
> include/linux/security.h | 15 +++++
> security/bpf/hooks.c | 1 +
> security/integrity/evm/evm_main.c | 8 ++-
> security/security.c | 108 ++++++++++++++++++++++++++----
> security/selinux/hooks.c | 4 +-
> security/smack/smack_lsm.c | 27 ++++----
> 10 files changed, 148 insertions(+), 47 deletions(-)
...
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 153e9043058f..647f7b88358b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -68,6 +68,11 @@ struct watch;
> struct watch_notification;
> struct lsm_ctx;
>
> +struct lsm_xattrs {
> + struct xattr *xattrs;
> + unsigned int xattr_count;
> +};
Please separate out the 'struct lsm_xattrs' related changes into a
separate patch from the security_lsmxattr_add() changes. I know they
are related, but they are different things.
> diff --git a/security/security.c b/security/security.c
> index 71aea8fdf014..261f68e17cfd 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -12,6 +12,7 @@
> #define pr_fmt(fmt) "LSM: " fmt
>
> #include <linux/bpf.h>
> +#include <linux/bpf_lsm.h>
> #include <linux/capability.h>
> #include <linux/dcache.h>
> #include <linux/export.h>
> @@ -1333,8 +1334,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const initxattrs initxattrs, void *fs_data)
> {
> struct lsm_static_call *scall;
> - struct xattr *new_xattrs = NULL;
> - int ret = -EOPNOTSUPP, xattr_count = 0;
> + struct lsm_xattrs xattrs = {};
> + int ret = -EOPNOTSUPP;
>
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
> @@ -1344,15 +1345,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>
> if (initxattrs) {
> /* Allocate +1 as terminator. */
> - new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
> - sizeof(*new_xattrs), GFP_NOFS);
> - if (!new_xattrs)
> + xattrs.xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
> + sizeof(*xattrs.xattrs), GFP_NOFS);
> + if (!xattrs.xattrs)
> return -ENOMEM;
> }
>
> lsm_for_each_hook(scall, inode_init_security) {
> - ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs,
> - &xattr_count);
> + ret = scall->hl->hook.inode_init_security(inode, dir, qstr,
> + &xattrs);
> if (ret && ret != -EOPNOTSUPP)
> goto out;
> /*
> @@ -1364,18 +1365,101 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> }
>
> /* If initxattrs() is NULL, xattr_count is zero, skip the call. */
> - if (!xattr_count)
> + if (!xattrs.xattr_count)
> goto out;
>
> - ret = initxattrs(inode, new_xattrs, fs_data);
> + ret = initxattrs(inode, xattrs.xattrs, fs_data);
> out:
> - for (; xattr_count > 0; xattr_count--)
> - kfree(new_xattrs[xattr_count - 1].value);
> - kfree(new_xattrs);
> + for (; xattrs.xattr_count > 0; xattrs.xattr_count--)
> + kfree(xattrs.xattrs[xattrs.xattr_count - 1].value);
> + kfree(xattrs.xattrs);
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> +#ifdef CONFIG_BPF_LSM
> +static unsigned int lsm_xattrs_used(const struct lsm_xattrs *xattrs,
> + const char *prefix)
> +{
> + size_t prefix_len = strlen(prefix);
> + unsigned int i, n = 0;
> +
> + for (i = 0; i < xattrs->xattr_count; i++) {
> + const char *name = xattrs->xattrs[i].name;
> +
> + if (name && !strncmp(name, prefix, prefix_len))
> + n++;
> + }
> + return n;
> +}
> +#endif /* CONFIG_BPF_LSM */
More on this below, but this function isn't strictly BPF LSM related so
let's drop the CONFIG_BPF_LSM macro bracketing.
> +/**
> + * security_lsmxattr_add() - Add an xattr during inode_init_security
> + * @xattrs: xattr state shared by inode_init_security hooks
> + * @lsm_id: LSM_ID_* value identifying the calling LSM
> + * @name: xattr name suffix
For BPF you currently pass "bpf.foo" for the name and other LSMs would
pass their own xattr name, likely without a LSM specific sub-namespace
(for example, SELinux would be just "selinux"). In both cases the
'name' parameter always starts with a well known suffix as defined by
the 'lsm_id" parameter.
Since we are already passing the lsm_id parameter, let's do away with
the standard LSM suffixes, e.g. XATTR_BPF_LSM_SUFFIX, and just pass in
any additional name components. For example, instead of passing
"bpf.foo" in the BPF LSM case, you would just pass "foo"; LSMs without
their own sub-namespace, e.g. SELinux, would pass NULL for the name
parameter as XATTR_SELINUX_SUFFIX is all that is needed. While doing
this I would also suggest changing the name of the 'name' parameter to
'name_extra', 'namespace_extra', or something similar to indicate that
it isn't the full name, but rather an additional suffix beyond the
standard suffix associated with the given LSM.
> + * @value: xattr value
> + * @value_len: length of @value
> + *
> + * Claim an xattr slot in @xattrs on behalf of the LSM identified by
> + * @lsm_id and fill it with a copy of @name and @value. Callers can invoke
> + * this function from non-sleepable context.
> + *
> + * Return: Returns 0 on success, -ENOSPC if the calling LSM's slot budget
> + * is exhausted, negative values on other errors.
> + */
> +int security_lsmxattr_add(struct lsm_xattrs *xattrs, u64 lsm_id,
> + const char *name, const void *value,
> + size_t value_len)
> +{
> + struct xattr *xattr;
> + void *xattr_value;
> + size_t name_len;
> +
> + if (!xattrs || !xattrs->xattrs || !name || !value)
> + return -EINVAL;
Sashiko raised a good point about xattrs->xattrs being NULL not
necessarily being a good reason for -EINVAL. If xattrs is NULL, yes,
something has gone wrong and -EINVAL seems reasonable, but the
xattr->xattrs NULL case does seem like it should simply return early
with a value of 0 (see SELinux's handling of this case as an example).
> + name_len = strlen(name);
> + if (name_len == 0 || name_len > XATTR_NAME_MAX)
> + return -EINVAL;
> + if (value_len == 0 || value_len > XATTR_SIZE_MAX)
> + return -EINVAL;
> +
> + switch (lsm_id) {
> +#ifdef CONFIG_BPF_LSM
> + case LSM_ID_BPF:
> + if (lsm_xattrs_used(xattrs, XATTR_BPF_LSM_SUFFIX) >=
> + BPF_LSM_INODE_INIT_XATTRS)
> + return -ENOSPC;
> + break;
> +#endif /* CONFIG_BPF_LSM */
I like to avoid macro conditional code inside functions whenever
possible, and I think this is a case where we could avoid the conditional
block with a little work.
The LSM_ID_BPF macro is already defined as part of the UAPI so that will
always be available. While BPF_LSM_INODE_INIT_XATTRS is dependent on
CONFIG_BPF_LSM in this revision, that should be easy enough to move
outside the CONFIG_BPF_LSM conditional in bpf_lsm.h. Eventually we
should probably expand the lsm_id struct to carry this info, likely just
a permanent/local copy of the LSM's lsm_blob_sizes passed during
registration, but I can take care of that later; just make the
BPF_LSM_INODE_INIT_XATTRS macro always accessible now.
We should also probably record the xattr suffix/length when the LSM is
registered, but that can also be done later with the other lsm_id
additions.
> + default:
> + return -EINVAL;
> + }
> +
> + /* Combine xattr value + name into one allocation. */
> + xattr_value = kmalloc(value_len + name_len + 1, GFP_NOWAIT);
> + if (!xattr_value)
> + return -ENOMEM;
> +
> + memcpy(xattr_value, value, value_len);
> + memcpy(xattr_value + value_len, name, name_len);
> + ((char *)xattr_value)[value_len + name_len] = '\0';
You'll need to add an additional memcpy() here, likely in a switch
statement to handle the different LSMs, to copy over the LSM specific
prefix. It's a little ugly, but when we have things captured in the
lsm_id struct it will get a lot cleaner.
> + xattr = lsm_get_xattr_slot(xattrs);
> + if (!xattr) {
> + kfree(xattr_value);
> + return -ENOSPC;
> + }
> +
> + xattr->value = xattr_value;
> + xattr->name = (const char *)xattr_value + value_len;
> + xattr->value_len = value_len;
> +
> + return 0;
> +}
--
paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox