linux-kernel - Re: [PATCH v5 02/24] landlock: Add unique ID generator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250308.moo2siethohX@digikod.net>
Date: Sat, 8 Mar 2025 19:40:18 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Günther Noack <gnoack3000@...il.com>
Cc: Eric Paris <eparis@...hat.com>, Paul Moore <paul@...l-moore.com>, 
	Günther Noack <gnoack@...gle.com>, "Serge E . Hallyn" <serge@...lyn.com>, 
	Ben Scarlato <akhna@...gle.com>, Casey Schaufler <casey@...aufler-ca.com>, 
	Charles Zaffery <czaffery@...lox.com>, Daniel Burgener <dburgener@...ux.microsoft.com>, 
	Francis Laniel <flaniel@...ux.microsoft.com>, James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, 
	Jeff Xu <jeffxu@...gle.com>, Jorge Lucangeli Obes <jorgelo@...gle.com>, 
	Kees Cook <kees@...nel.org>, Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, 
	Matt Bobrowski <mattbobrowski@...gle.com>, Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, 
	Phil Sutter <phil@....cc>, Praveen K Paladugu <prapal@...ux.microsoft.com>, 
	Robert Salvet <robert.salvet@...lox.com>, Shervin Oloumi <enlightened@...gle.com>, 
	Song Liu <song@...nel.org>, Tahera Fahimi <fahimitahera@...il.com>, 
	Tyler Hicks <code@...icks.com>, audit@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH v5 02/24] landlock: Add unique ID generator

On Fri, Mar 07, 2025 at 03:15:44PM +0100, Günther Noack wrote:
> On Fri, Jan 31, 2025 at 05:30:37PM +0100, Mickaël Salaün wrote:
> > --- /dev/null
> > +++ b/security/landlock/id.c
> > +static atomic64_t next_id = ATOMIC64_INIT(COUNTER_PRE_INIT);
> > +
> > +static void __init init_id(atomic64_t *const counter, const u32 random_32bits)
> > +{
> > +	u64 init;
> > +
> > +	/*
> > +	 * Ensures sure 64-bit values are always used by user space (or may
> > +	 * fail with -EOVERFLOW), and makes this testable.
> > +	 */
> > +	init = 1ULL << 32;
> > +
> > +	/*
> > +	 * Makes a large (2^32) boot-time value to limit ID collision in logs
> > +	 * from different boots, and to limit info leak about the number of
> > +	 * initially (relative to the reader) created elements (e.g. domains).
> > +	 */
> > +	init += random_32bits;
> > +
> > +	/* Sets first or ignores.  This will be the first ID. */
> > +	atomic64_cmpxchg(counter, COUNTER_PRE_INIT, init);
> 
> It feels like this should always need to succeed.  Or to say it the
> other way around: If this cmpxchg were to fail, the guarantees from
> your commit message would be broken.  Maybe it would be worth handling
> that error case in a more direct way?

This should always succeed and with the current code it always succeed
because there is only one call to this function.  This
atomic64_cmpxchg() is a safeguard to be sure that, even if there are
several calls to this function, the counter will only be initialized
once (i.e. cmpxchg only sets the counter if its value was 0)

We could add a WARN_ON(atomic64_cmpxchg()) but I don't see the point.

> 
> 
> > +static void __init test_init_once(struct kunit *const test)
> > +{
> > +	const u64 first_init = 1ULL + U32_MAX;
> > +	atomic64_t counter = ATOMIC64_INIT(COUNTER_PRE_INIT);
> > +
> > +	init_id(&counter, 0);
> > +	KUNIT_EXPECT_EQ(test, atomic64_read(&counter), first_init);
> > +
> > +	init_id(&counter, ~0);
> > +	KUNIT_EXPECT_EQ(test, atomic64_read(&counter), first_init);
> 
> Maybe we can annotate this with an explanatory message,
> to make it slightly clearer that this is the point of the test:
> 
> KUNIT_EXPECT_EQ_MSG(test, atomic64_read(&counter), first_init,
>     "should still have the same value after the subsequent init_id()");

Yep, good idea.

> 
> –Günther
>