linux-kernel - Re: [PATCH v12 04/10] KEYS: Move KEY_LOOKUP

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Aug 2022 08:42:20 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     roberto.sassu@...weicloud.com
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, mykolal@...com,
        corbet@....net, dhowells@...hat.com, rostedt@...dmis.org,
        mingo@...hat.com, paul@...l-moore.com, jmorris@...ei.org,
        serge@...lyn.com, shuah@...nel.org, bpf@...r.kernel.org,
        linux-doc@...r.kernel.org, keyrings@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        deso@...teo.net, Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v12 04/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h

On Thu, Aug 18, 2022 at 05:29:23PM +0200, roberto.sassu@...weicloud.com wrote:
> From: Roberto Sassu <roberto.sassu@...wei.com>
> 
> In preparation for the patch that introduces the bpf_lookup_user_key() eBPF
> kfunc, move KEY_LOOKUP_ definitions to include/linux/key.h, to be able to
> validate the kfunc parameters.
> 
> Also, introduce key_lookup_flags_check() directly in include/linux/key.h,
> to reduce the risk that the check is not in sync with currently defined
> flags.

Missing the description what the heck this function even is.

Please, explain that.

Also, the short subject is misleading because this *just*
does not move flags.

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> Reviewed-by: KP Singh <kpsingh@...nel.org>
> ---
>  include/linux/key.h      | 11 +++++++++++
>  security/keys/internal.h |  2 --
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 7febc4881363..b5bbae77a9e7 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -88,6 +88,17 @@ enum key_need_perm {
>  	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
>  };
>  
> +#define KEY_LOOKUP_CREATE	0x01
> +#define KEY_LOOKUP_PARTIAL	0x02
> +

/*
 * Explain what the heck this function is.
 */
> +static inline int key_lookup_flags_check(u64 flags)
> +{
> +	if (flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL))
> +		return -EINVAL;
> +
> +	return 0;
> +}

This is essentially a boolean function, right?

I.e. the implementation can be just:

!!(flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL))

Not even sure if this is needed in the first place, or
would it be better just to open code it. How many call
sites does it have anyway?

BR, Jarkko