linux-hardening - 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 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]

Date:   Mon, 9 May 2022 16:13:48 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     David Howells <dhowells@...hat.com>,
        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 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.

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);

>  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;
	}

- Eric