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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250307.db38587e80e7@gnoack.org>
Date: Fri, 7 Mar 2025 15:15:44 +0100
From: Günther Noack <gnoack3000@...il.com>
To: Mickaël Salaün <mic@...ikod.net>
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, 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?


> +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()");

–Günther