Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag

From: Christian Brauner <brauner@kernel.org>
To: Stas Sergeev <stsp2@yandex.ru>
Cc: linux-kernel@vger.kernel.org,
	"Stefan Metzmacher" <metze@samba.org>,
	"Eric Biederman" <ebiederm@xmission.com>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Andy Lutomirski" <luto@kernel.org>, "Jan Kara" <jack@suse.cz>,
	"Jeff Layton" <jlayton@kernel.org>,
	"Chuck Lever" <chuck.lever@oracle.com>,
	"Alexander Aring" <alex.aring@gmail.com>,
	"David Laight" <David.Laight@aculab.com>,
	linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Christian Göttsche" <cgzones@googlemail.com>
Subject: Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
Date: Thu, 25 Apr 2024 16:02:46 +0200	[thread overview]
Message-ID: <20240425-akrobatisch-palmen-4c1e7a0f69d2@brauner> (raw)
In-Reply-To: <20240424105248.189032-3-stsp2@yandex.ru>

>  struct open_flags {
> -	int open_flag;
> +	u64 open_flag;

Btw, this change taken together with

> +#define VALID_OPENAT2_FLAGS (VALID_OPEN_FLAGS | OA2_INHERIT_CRED)

is also ripe to causes subtle bugs and security issues. This new
VALID_OPENAT2_FLAGS define bypasses

	BUILD_BUG_ON_MSG(upper_32_bits(VALID_OPEN_FLAGS),
			 "struct open_flags doesn't yet handle flags > 32 bits");

in build_open_flags(). And right now lookup_open(), open_last_lookups(),
and do_open() just do:

	int open_flag = op->open_flag;

Because op->open_flag was 32bit that was fine. But now ->open_flag is
64bit which means we truncate the upper 32bit including OA2_INHERIT_CRED
or any other new flag in the upper 32bits in those functions.

So as soon as there's an additional check in e.g., do_open() for
OA2_INHERIT_CRED or in any of the other helpers that's security relevant
we're fscked because that flag is never seen and no bot will help us
here. And it's super easy to miss during review...