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
| ||
|
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
Powered by blists - more mailing lists