mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] landlock: Move the domain layer counter out of the anonymous union
@ 2026-10-07 16:28 Abel Vesa
  2026-10-07 21:49 ` Nick Desaulniers
  2026-10-09 11:51 ` Mike Bommarito
  0 siblings, 2 replies; 6+ messages in thread
From: Abel Vesa @ 2026-10-07 16:28 UTC (permalink / raw)
  To: Mickaël Salaün, Günther Noack, Paul Moore,
	James Morris, Serge E. Hallyn, Kees Cook, Gustavo A. R. Silva,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Tingmao Wang
  Cc: linux-security-module, linux-kernel, linux-hardening, llvm,
	Konrad Dybcio, Abel Vesa

With Clang 20 and CONFIG_FORTIFY_SOURCE, stacking Landlock domains can
trigger a fortify panic in the handled_masks copy in inherit_ruleset():

  __fortify_panic
  landlock_merge_ruleset
  __arm64_sys_landlock_restrict_self

Clang miscalculates the location of the __counted_by counter in the
anonymous structure nested inside the domain's union.  In an arm64 build,
__builtin_dynamic_object_size(domain->handled_masks, 1) reads offset 40
(the first handled_masks entry) instead of offset 36 (num_layers).
Because the destination domain is zero-initialized and its masks have not
yet been populated, FORTIFY sees a zero-sized destination and rejects the
copy of the parent's layers.

Move num_layers and handled_masks to the top level of landlock_domain so
that Clang uses the correct counter.

Assisted-by: LLM
Fixes: bd3a19800dd1 ("landlock: Add counted_by in landlock_domain")
Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
Move the domain layer counter and flexible array out of the anonymous
union to work around Clang 20 reading the wrong counter for __counted_by.
This fixes the FORTIFY panic when copying the parent domain's layer stack.

Based on next-20260930. Validated arm64 Landlock object builds with
Clang 20 and GCC 14, with CONFIG_FORTIFY_SOURCE enabled, and checked the
counter load in Clang-generated code. Not boot-tested.
---
 security/landlock/domain.h | 48 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/security/landlock/domain.h b/security/landlock/domain.h
index caa3d19d2c43..5e65ab004f62 100644
--- a/security/landlock/domain.h
+++ b/security/landlock/domain.h
@@ -216,37 +216,29 @@ struct landlock_domain {
 		/**
 		 * @work_free: Enables to free a domain within a lockless
 		 * section.  This is only used by landlock_put_domain_deferred()
-		 * when @usage reaches zero.  The fields @usage, @num_layers and
-		 * @handled_masks are then unused.
+		 * when @usage reaches zero.  The field @usage is then unused.
 		 */
 		struct work_struct work_free;
-		struct {
-			/**
-			 * @usage: Number of credentials referencing this
-			 * domain.
-			 */
-			refcount_t usage;
-			/**
-			 * @num_layers: Number of layers that are used in this
-			 * domain.  This enables to check that all the layers
-			 * allow an access request.
-			 */
-			u32 num_layers;
-			/**
-			 * @handled_masks: Contains the subset of filesystem and
-			 * network actions that are restricted by a domain.  A
-			 * domain saves all layers of merged rulesets in a stack
-			 * (FAM), starting from the first layer to the last one.
-			 * These layers are used when merging rulesets, for user
-			 * space backward compatibility (i.e. future-proof), and
-			 * to properly handle merged rulesets without
-			 * overlapping access rights.  These layers are set once
-			 * and never changed for the lifetime of the domain.
-			 */
-			struct access_masks
-				handled_masks[] __counted_by(num_layers);
-		};
+		/**
+		 * @usage: Number of credentials referencing this domain.
+		 */
+		refcount_t usage;
 	};
+	/**
+	 * @num_layers: Number of layers that are used in this domain.  This
+	 * enables to check that all the layers allow an access request.
+	 */
+	u32 num_layers;
+	/**
+	 * @handled_masks: Contains the subset of filesystem and network actions
+	 * that are restricted by a domain.  A domain saves all layers of merged
+	 * rulesets in a stack (FAM), starting from the first layer to the last
+	 * one.  These layers are used when merging rulesets, for user space
+	 * backward compatibility (i.e. future-proof), and to properly handle
+	 * merged rulesets without overlapping access rights.  These layers are
+	 * set once and never changed for the lifetime of the domain.
+	 */
+	struct access_masks handled_masks[] __counted_by(num_layers);
 };
 
 static inline access_mask_t

---
base-commit: 6c2cb8b8b843d216ab549b678a0d8831c43153e0
change-id: 20261007-b4-landlock-fix-domain-fortify-panic-510367f8cfe3

Best regards,
--  
Abel Vesa <abel.vesa@oss.qualcomm.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] landlock: Move the domain layer counter out of the anonymous union
  2026-10-07 16:28 [PATCH] landlock: Move the domain layer counter out of the anonymous union Abel Vesa
@ 2026-10-07 21:49 ` Nick Desaulniers
  2026-10-08 11:49   ` Abel Vesa
  2026-10-09 11:51 ` Mike Bommarito
  1 sibling, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2026-10-07 21:49 UTC (permalink / raw)
  To: Abel Vesa, Bill Wendling, Tingmao Wang, Nathan Chancellor, Kees Cook
  Cc: Gustavo A. R. Silva, Justin Stitt, linux-security-module,
	linux-kernel, linux-hardening, llvm, Konrad Dybcio

On Wed, Oct 7, 2026 at 9:28 AM Abel Vesa <abel.vesa@oss.qualcomm.com> wrote:
>
> With Clang 20 and CONFIG_FORTIFY_SOURCE, stacking Landlock domains can
> trigger a fortify panic in the handled_masks copy in inherit_ruleset():
>
>   __fortify_panic
>   landlock_merge_ruleset
>   __arm64_sys_landlock_restrict_self
>
> Clang miscalculates the location of the __counted_by counter in the

Thanks for the patch.

Would you consider this a bug in clang, or an error in how the
__counted_by attribute is applied?

If the former, then a bug report against upstream llvm-project/ and a
link to that bug report would be appreciated.  Then Bill can take a
look.

Or if this was a known issue specific to clang 20, then we should have
a link to the fix (or consider bumping counted_by support to the
compiler version that is bug free).

> anonymous structure nested inside the domain's union.  In an arm64 build,
> __builtin_dynamic_object_size(domain->handled_masks, 1) reads offset 40
> (the first handled_masks entry) instead of offset 36 (num_layers).
> Because the destination domain is zero-initialized and its masks have not
> yet been populated, FORTIFY sees a zero-sized destination and rejects the
> copy of the parent's layers.
>
> Move num_layers and handled_masks to the top level of landlock_domain so
> that Clang uses the correct counter.
>
> Assisted-by: LLM
> Fixes: bd3a19800dd1 ("landlock: Add counted_by in landlock_domain")
> Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---
> Move the domain layer counter and flexible array out of the anonymous
> union to work around Clang 20 reading the wrong counter for __counted_by.
> This fixes the FORTIFY panic when copying the parent domain's layer stack.
>
> Based on next-20260930. Validated arm64 Landlock object builds with
> Clang 20 and GCC 14, with CONFIG_FORTIFY_SOURCE enabled, and checked the
> counter load in Clang-generated code. Not boot-tested.
> ---
>  security/landlock/domain.h | 48 +++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/security/landlock/domain.h b/security/landlock/domain.h
> index caa3d19d2c43..5e65ab004f62 100644
> --- a/security/landlock/domain.h
> +++ b/security/landlock/domain.h
> @@ -216,37 +216,29 @@ struct landlock_domain {
>                 /**
>                  * @work_free: Enables to free a domain within a lockless
>                  * section.  This is only used by landlock_put_domain_deferred()
> -                * when @usage reaches zero.  The fields @usage, @num_layers and
> -                * @handled_masks are then unused.
> +                * when @usage reaches zero.  The field @usage is then unused.
>                  */
>                 struct work_struct work_free;
> -               struct {
> -                       /**
> -                        * @usage: Number of credentials referencing this
> -                        * domain.
> -                        */
> -                       refcount_t usage;
> -                       /**
> -                        * @num_layers: Number of layers that are used in this
> -                        * domain.  This enables to check that all the layers
> -                        * allow an access request.
> -                        */
> -                       u32 num_layers;
> -                       /**
> -                        * @handled_masks: Contains the subset of filesystem and
> -                        * network actions that are restricted by a domain.  A
> -                        * domain saves all layers of merged rulesets in a stack
> -                        * (FAM), starting from the first layer to the last one.
> -                        * These layers are used when merging rulesets, for user
> -                        * space backward compatibility (i.e. future-proof), and
> -                        * to properly handle merged rulesets without
> -                        * overlapping access rights.  These layers are set once
> -                        * and never changed for the lifetime of the domain.
> -                        */
> -                       struct access_masks
> -                               handled_masks[] __counted_by(num_layers);
> -               };
> +               /**
> +                * @usage: Number of credentials referencing this domain.
> +                */
> +               refcount_t usage;
>         };
> +       /**
> +        * @num_layers: Number of layers that are used in this domain.  This
> +        * enables to check that all the layers allow an access request.
> +        */
> +       u32 num_layers;
> +       /**
> +        * @handled_masks: Contains the subset of filesystem and network actions
> +        * that are restricted by a domain.  A domain saves all layers of merged
> +        * rulesets in a stack (FAM), starting from the first layer to the last
> +        * one.  These layers are used when merging rulesets, for user space
> +        * backward compatibility (i.e. future-proof), and to properly handle
> +        * merged rulesets without overlapping access rights.  These layers are
> +        * set once and never changed for the lifetime of the domain.
> +        */
> +       struct access_masks handled_masks[] __counted_by(num_layers);
>  };
>
>  static inline access_mask_t
>
> ---
> base-commit: 6c2cb8b8b843d216ab549b678a0d8831c43153e0
> change-id: 20261007-b4-landlock-fix-domain-fortify-panic-510367f8cfe3
>
> Best regards,
> --
> Abel Vesa <abel.vesa@oss.qualcomm.com>
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] landlock: Move the domain layer counter out of the anonymous union
  2026-10-07 21:49 ` Nick Desaulniers
@ 2026-10-08 11:49   ` Abel Vesa
  2026-10-09 17:23     ` Nick Desaulniers
  0 siblings, 1 reply; 6+ messages in thread
From: Abel Vesa @ 2026-10-08 11:49 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Bill Wendling, Tingmao Wang, Nathan Chancellor, Kees Cook,
	Gustavo A. R. Silva, Justin Stitt, linux-security-module,
	linux-kernel, linux-hardening, llvm, Konrad Dybcio

On 26-10-07 14:49:28, Nick Desaulniers wrote:
> On Wed, Oct 7, 2026 at 9:28 AM Abel Vesa <abel.vesa@oss.qualcomm.com> wrote:
> >
> > With Clang 20 and CONFIG_FORTIFY_SOURCE, stacking Landlock domains can
> > trigger a fortify panic in the handled_masks copy in inherit_ruleset():
> >
> >   __fortify_panic
> >   landlock_merge_ruleset
> >   __arm64_sys_landlock_restrict_self
> >
> > Clang miscalculates the location of the __counted_by counter in the
> 
> Thanks for the patch.
> 
> Would you consider this a bug in clang, or an error in how the
> __counted_by attribute is applied?
> 
> If the former, then a bug report against upstream llvm-project/ and a
> link to that bug report would be appreciated.  Then Bill can take a
> look.
> 
> Or if this was a known issue specific to clang 20, then we should have
> a link to the fix (or consider bumping counted_by support to the
> compiler version that is bug free).

This is a Clang code generation bug, and it is not limited to Clang 20.
I checked a reduced reproducer with Clang 20.1.2, 21.1.8, 22.1.8 and
23.1.2. All four reproduce it at both -O0 and -O2.

The native x86_64 runtime test returns zero for the dynamic object size
of the zero-initialized array, despite num_layers being set. The AArch64
assembly also shows the count being loaded from offset 40 rather than
num_layers at offset 36. Moving the counter and array out of the union
makes the reproducer pass with all four versions.

It turns out Amaan Qureshi has already reported exactly this Landlock
issue and proposed an LLVM fix:

  https://github.com/llvm/llvm-project/pull/228309

Thanks,
Abel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] landlock: Move the domain layer counter out of the anonymous union
  2026-10-07 16:28 [PATCH] landlock: Move the domain layer counter out of the anonymous union Abel Vesa
  2026-10-07 21:49 ` Nick Desaulniers
@ 2026-10-09 11:51 ` Mike Bommarito
  1 sibling, 0 replies; 6+ messages in thread
From: Mike Bommarito @ 2026-10-09 11:51 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mickaël Salaün, Günther Noack, Paul Moore,
	James Morris, Serge E . Hallyn, Kees Cook, Gustavo A . R . Silva,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Tingmao Wang, Konrad Dybcio, linux-security-module, linux-kernel,
	linux-hardening, llvm

On Wed, Oct 07, 2026 at 07:28:30PM +0300, Abel Vesa wrote:
> With Clang 20 and CONFIG_FORTIFY_SOURCE, stacking Landlock domains can
> trigger a fortify panic in the handled_masks copy in inherit_ruleset():
[...]
> Based on next-20260930. Validated arm64 Landlock object builds with
> Clang 20 and GCC 14, with CONFIG_FORTIFY_SOURCE enabled, and checked the
> counter load in Clang-generated code. Not boot-tested.

Hi all, hit this issue yesterday on x86_64 (Framework 13, Meteor Lake),
v7.3-rc5 built with clang 21.1.8 and CONFIG_FORTIFY_SOURCE=y.  I triggered
it simply by opening the printer settings on 26.04 (through xz, see below).

My build tree was recent v7.3-rc5, so I applied the patch, which was clean,
and then went through the repro steps after rebuilding.  Everything works
as expected after 10-15 minutes of use - no crash, no other dmesg/journal
issues.

I hope you don't mind, but given how irritating it was to track down
the issue, I am including some of the Claude output below to help others
find the root cause more quickly if they hit it.  I checked the
disassembly and ran the tests below myself on this machine.

<CLAUDE>
This is easy to hit on a stock desktop.  xz 5.8 sandboxes itself with
nested landlock_restrict_self() calls, so on an unpatched clang build every
`xz -d` oopses:

  memcpy: detected buffer overflow: 4 byte write of buffer size 0
  WARNING: lib/string_helpers.c:1037 at __fortify_report+0x1f/0x60, CPU#6: xz/9888
  kernel BUG at lib/string_helpers.c:1044!
  Oops: invalid opcode: 0000 [#1] SMP NOPTI
  RIP: 0010:__fortify_panic+0x9/0x10
  Call Trace:
   landlock_merge_ruleset+0x353/0x360
   __se_sys_landlock_restrict_self+0x1bf/0x470
   __x64_sys_landlock_restrict_self+0x16/0x30

On Ubuntu, simply opening GNOME Settings -> Printers runs the CUPS driver
helpers, which run xz.  With kdump-tools installed (panic_on_oops=1), that
panics the machine.  Nothing more than an unprivileged process is needed;
the reproducer below triggers it as a normal user:

  prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
  for (i = 0; i < 2; i++) {
          struct landlock_ruleset_attr a = {
                  .handled_access_fs = LANDLOCK_ACCESS_FS_MAKE_REG };
          int fd = syscall(SYS_landlock_create_ruleset, &a, sizeof(a), 0);
          syscall(SYS_landlock_restrict_self, fd, 0);  /* i == 1 oopses */
          close(fd);
  }

The x86_64 codegen matches your arm64 analysis.  Unpatched, with
num_layers at 0x24 and handled_masks at 0x28:

  mov    0x28(%rbx),%esi      # "count" read from child->handled_masks[0]
  shl    $0x2,%rsi
  cmp    %rdx,%rsi
  jb     <__fortify_panic>

With the patch, num_layers is at 0x40 and handled_masks at 0x44.  The
count is loaded from the right field, and clang folds away the fortify
checks after the WARN_ON_ONCE(child->num_layers <= parent->num_layers):

  mov    0x40(%r12),%edx
  cmp    %edx,0x40(%rbx)
  ...
  call   memcpy

Results after booting the patched kernel, with panic_on_oops=1 and
kdump loaded (the configuration that used to panic):

  nested restrict reproducer:  both layers succeed
  echo hello | xz | xz -d:     ok
  lpinfo -m (CUPS -> xz):      ok
  fortify reports in dmesg:    none; kernel not tainted

The layout change grows struct landlock_domain from 64 to 72 bytes.  A
one-layer domain is still a kmalloc-96 allocation (68 bytes before, 76
after); only stacks of 7 or 8 layers move up to kmalloc-128.  That seems
fine.
</CLAUDE>

Since this is reachable from unprivileged userspace on any clang +
FORTIFY_SOURCE build of 7.3-rc, it would be good to get this in before
v7.3 final.

Tested-by: Mike Bommarito <michael.bommarito@gmail.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] landlock: Move the domain layer counter out of the anonymous union
  2026-10-08 11:49   ` Abel Vesa
@ 2026-10-09 17:23     ` Nick Desaulniers
  2026-10-09 20:45       ` Nathan Chancellor
  0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2026-10-09 17:23 UTC (permalink / raw)
  To: Abel Vesa, Bill Wendling, Nathan Chancellor, Kees Cook
  Cc: Tingmao Wang, Gustavo A. R. Silva, Justin Stitt,
	linux-security-module, linux-kernel, linux-hardening, llvm,
	Konrad Dybcio, michael.bommarito

On Thu, Oct 8, 2026 at 4:49 AM Abel Vesa <abel.vesa@oss.qualcomm.com> wrote:
>
> On 26-10-07 14:49:28, Nick Desaulniers wrote:
> > Would you consider this a bug in clang, or an error in how the
> > __counted_by attribute is applied?
> >
> It turns out Amaan Qureshi has already reported exactly this Landlock
> issue and proposed an LLVM fix:
>
>   https://github.com/llvm/llvm-project/pull/228309

In that case, I would _not_ work around the compiler bug like this and
instead wait for that fix to land in clang, then bump the required
version of clang for counted-by.

Kees and Nathan are traveling for LPC, so I don't expect quick
feedback from them, but they should confirm whether they agree with my
proposal.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] landlock: Move the domain layer counter out of the anonymous union
  2026-10-09 17:23     ` Nick Desaulniers
@ 2026-10-09 20:45       ` Nathan Chancellor
  0 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2026-10-09 20:45 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Abel Vesa, Bill Wendling, Kees Cook, Tingmao Wang,
	Gustavo A. R. Silva, Justin Stitt, linux-security-module,
	linux-kernel, linux-hardening, llvm, Konrad Dybcio,
	michael.bommarito

On Fri, Oct 09, 2026 at 10:23:14AM -0700, Nick Desaulniers wrote:
> On Thu, Oct 8, 2026 at 4:49 AM Abel Vesa <abel.vesa@oss.qualcomm.com> wrote:
> >
> > On 26-10-07 14:49:28, Nick Desaulniers wrote:
> > > Would you consider this a bug in clang, or an error in how the
> > > __counted_by attribute is applied?
> > >
> > It turns out Amaan Qureshi has already reported exactly this Landlock
> > issue and proposed an LLVM fix:
> >
> >   https://github.com/llvm/llvm-project/pull/228309
> 
> In that case, I would _not_ work around the compiler bug like this and
> instead wait for that fix to land in clang, then bump the required
> version of clang for counted-by.
> 
> Kees and Nathan are traveling for LPC, so I don't expect quick
> feedback from them, but they should confirm whether they agree with my
> proposal.

Bill/Kees/Justin should review that patch upstream but bumping the
required version for __counted_by to clang-24 (or maybe 23.1.x if it can
land in release/23.x) kind of sucks :/ but if this is a big enough
footgun that we don't want to continuously workaround, I guess we have
no choice.

-- 
Cheers,
Nathan

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-10-09 20:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-07 16:28 [PATCH] landlock: Move the domain layer counter out of the anonymous union Abel Vesa
2026-10-07 21:49 ` Nick Desaulniers
2026-10-08 11:49   ` Abel Vesa
2026-10-09 17:23     ` Nick Desaulniers
2026-10-09 20:45       ` Nathan Chancellor
2026-10-09 11:51 ` Mike Bommarito

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®