Re: [PATCH] big_keys: Use struct for internal payload

[Eric Biggers]

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@xxxxxxxxxx>
> Cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> Cc: James Morris <jmorris@xxxxxxxxx>
> Cc: "Serge E. Hallyn" <serge@xxxxxxxxxx>
> Cc: linux-hardening@xxxxxxxxxxxxxxx
> Cc: keyrings@xxxxxxxxxxxxxxx
> Cc: linux-security-module@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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