linux-kernel - Re: [PATCH] big_keys: Use struct for internal payload

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: <202205101509.B5E3096@keescook>
Date:   Tue, 10 May 2022 15:18:22 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     David Howells <dhowells@...hat.com>,
        Christoph Hellwig <hch@...radead.org>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-hardening@...r.kernel.org, keyrings@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] big_keys: Use struct for internal payload

On Mon, May 09, 2022 at 04:13:48PM -0700, Eric Biggers wrote:
> On Sun, May 08, 2022 at 10:57:31AM -0700, Kees Cook wrote:
> > The randstruct GCC plugin gets upset when it sees struct path (which is
> > randomized) being assigned from a "void *" (which it cannot type-check).
> > 
> > There's no need for these casts, as the entire internal payload use is
> > following a normal struct layout. Convert the enum-based void * offset
> > dereferencing to the new big_key_payload struct. No meaningful machine
> > code changes result after this change, and source readability is improved.
> > 
> > Drop the randstruct exception now that there is no "confusing" cross-type
> > assignment.
> > 
> > Cc: David Howells <dhowells@...hat.com>
> > Cc: Jarkko Sakkinen <jarkko@...nel.org>
> > Cc: James Morris <jmorris@...ei.org>
> > Cc: "Serge E. Hallyn" <serge@...lyn.com>
> > Cc: linux-hardening@...r.kernel.org
> > Cc: keyrings@...r.kernel.org
> > Cc: linux-security-module@...r.kernel.org
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> >  scripts/gcc-plugins/randomize_layout_plugin.c |  2 -
> >  security/keys/big_key.c                       | 64 ++++++++++---------
> >  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> This looks fine to me, although the way that an array of void pointers is cast
> to/from another struct is still weird.  I'd prefer if the payload was just
> changed into a separate allocation.

Yeah, though I realized after sending this patch that I'd done it
before[1] back with the rest of the randstruct GCC plugin enabling,
and it seems David was against the separate allocation, which, given the
space available, isn't unreasonable right up until struct path doesn't fit
anymore, but that's why I've added the BUILD_BUG_ON() to check sizes. :)
And this version ended up quite close to what hwh suggested[2] in 2017.

> A couple nits below if you stay with your proposed solution:
> 
> >  void big_key_free_preparse(struct key_preparsed_payload *prep)
> >  {
> > +	struct big_key_payload *payload = to_big_key_payload(prep->payload);
> > +
> >  	if (prep->datalen > BIG_KEY_FILE_THRESHOLD) {
> > -		struct path *path = (struct path *)&prep->payload.data[big_key_path];
> > +		struct path *path = &payload->path;
> >  
> >  		path_put(path);
> >  	}
> 
> This could just do:
> 
> 	if (prep->datalen > BIG_KEY_FILE_THRESHOLD)
> 		path_put(&payload->path);

Sure, I can avoid the extra variable.

> 
> >  void big_key_destroy(struct key *key)
> >  {
> > -	size_t datalen = (size_t)key->payload.data[big_key_len];
> > +	struct big_key_payload *payload = to_big_key_payload(key->payload);
> >  
> > -	if (datalen > BIG_KEY_FILE_THRESHOLD) {
> > -		struct path *path = (struct path *)&key->payload.data[big_key_path];
> > +	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
> > +		struct path *path = &payload->path;
> >  
> >  		path_put(path);
> >  		path->mnt = NULL;
> >  		path->dentry = NULL;
> >  	}
> 
> And similarly:
> 
> 	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
> 		path_put(&payload->path);
> 		payload->path.mnt = NULL;
> 		payload->path.dentry = NULL;
> 	}

I will respin.

Thanks!

-Kees

[1] https://lore.kernel.org/lkml/20170508214324.GA124468@beast/
[2] https://lore.kernel.org/lkml/20170528081249.GD22193@infradead.org/

-- 
Kees Cook