Re: [PATCH v7 09/28] landlock: Add AUDIT_LANDLOCK_ACCESS and log ptrace denials

Re: [PATCH v7 09/28] landlock: Add AUDIT_LANDLOCK_ACCESS and log ptrace denials

[Mickaël Salaün]

On Thu, Mar 27, 2025 at 09:38:05PM +0000, Tingmao Wang wrote:
> Hi Mickaël,

Hi, thanks for the report.

> 
> On 3/20/25 19:06, Mickaël Salaün wrote:
> [...]
> > +static struct landlock_hierarchy *
> > +get_hierarchy(const struct landlock_ruleset *const domain, const size_t layer)
> > +{
> > +	struct landlock_hierarchy *hierarchy = domain->hierarchy;
> > +	ssize_t i;
> > +
> > +	if (WARN_ON_ONCE(layer >= domain->num_layers))
> > +		return hierarchy;
> > +
> > +	for (i = domain->num_layers - 1; i > layer; i--) {
> > +		if (WARN_ON_ONCE(!hierarchy->parent))
> > +			break;
> > +
> > +		hierarchy = hierarchy->parent;
> > +	}
> > +
> > +	return hierarchy;
> > +}
> > +
> > +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> > +
> > +static void test_get_hierarchy(struct kunit *const test)
> > +{
> > +	struct landlock_hierarchy dom0_hierarchy = {
> > +		.id = 10,
> > +	};
> > +	struct landlock_hierarchy dom1_hierarchy = {
> > +		.parent = &dom0_hierarchy,
> > +		.id = 20,
> > +	};
> > +	struct landlock_hierarchy dom2_hierarchy = {
> > +		.parent = &dom1_hierarchy,
> > +		.id = 30,
> > +	};
> > +	struct landlock_ruleset dom2 = {
> > +		.hierarchy = &dom2_hierarchy,
> > +		.num_layers = 3,
> > +	};
> > +
> > +	KUNIT_EXPECT_EQ(test, 10, get_hierarchy(&dom2, 0)->id);
> > +	KUNIT_EXPECT_EQ(test, 20, get_hierarchy(&dom2, 1)->id);
> > +	KUNIT_EXPECT_EQ(test, 30, get_hierarchy(&dom2, 2)->id);
> > +	KUNIT_EXPECT_EQ(test, 30, get_hierarchy(&dom2, -1)->id);
> 
> This causes a warning from WARN_ON_ONCE(layer >= domain->num_layers)
> when running this test, I guess because layer is unsigned.

Interestingly this doesn't make the test to fail (because the result is
still correct), nor to show up when using tools/testing/kunit/kunit.py,
which is why I didn't see that.

> Should it
> be ssize_t, if this is an expected usage?

The get_hierarchy() code is correct, and the KUnit test is correct too.
Using a ssize_t would introduce a bug.

The issue is that I wanted to test a case that should never happen,
hence the WARN_ON_ONCE().

I guess the best "fix" for now would be to remove the KUnit test with
-1, but there is a new KUnit feature to hide this kind of warning:
https://lore.kernel.org/linux-kselftest/20250313114329.284104-1-acarmina@redhat.com/
It is currently in linux-next, but I'm not sure it will be merged in
Linux 6.15 .

For now I'll keep this commit but I'll send a fix/update to either
remove the test or use the new DEFINE_SUPPRESSED_WARNING macros
depending on its merge status.

> 
> ------------[ cut here ]------------
> WARNING: CPU: 7 PID: 145 at security/landlock/audit.c:142 get_hierarchy (security/landlock/audit.c:142)
> Modules linked in:
> CPU: 7 UID: 0 PID: 145 Comm: kunit_try_catch Tainted: G                 N  6.14.0-next-20250326-dev-00004-g4e57edc3e062-dirty #5 PREEMPT(undef)
> Tainted: [N]=TEST
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> RIP: 0010:get_hierarchy (security/landlock/audit.c:142)
> Code: 83 e8 02 e8 18 00 84 c0 75 02 0f 0b 48 83 c4 08 48 89 d8 5b 41 5c 41 5e 5d c3 48 c7 c7 00 f3 21 83 e8 e2 e7 18 00 84 c0 75 e2 <0f> 0b eb de 48 89 75 e0 e8 a1 a9 a7 ff 48 8b 75 e0 e9 76 ff ff ff
> // snip //
> Call Trace:
>  <TASK>
> test_get_hierarchy (security/landlock/audit.c:178 (discriminator 5))
> ? test_get_denied_layer (security/landlock/audit.c:158)
> ? lock_repin_lock (kernel/locking/lockdep.c:5649 kernel/locking/lockdep.c:5978)
> ? __lock_acquire (kernel/locking/lockdep.c:4675 kernel/locking/lockdep.c:5189)
> ? _raw_spin_unlock_irqrestore (./include/linux/spinlock_api_smp.h:151 kernel/locking/spinlock.c:194)
> ? find_held_lock (kernel/locking/lockdep.c:5348)
> ? trace_irq_enable (./include/trace/events/preemptirq.h:40 (discriminator 17))
> ? trace_hardirqs_on (kernel/trace/trace_preemptirq.c:80)
> ? kvm_clock_get_cycles (./arch/x86/include/asm/preempt.h:95 arch/x86/kernel/kvmclock.c:80 arch/x86/kernel/kvmclock.c:86)
> ? ktime_get_ts64 (kernel/time/timekeeping.c:318 (discriminator 4) kernel/time/timekeeping.c:335 (discriminator 4) kernel/time/timekeeping.c:907 (discriminator 4))
> kunit_try_run_case (lib/kunit/test.c:400 lib/kunit/test.c:443)
> ? kunit_try_run_case_cleanup (lib/kunit/test.c:430)
> 
>