diff mbox series

[v14,04/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h and add flags check function

Message ID 20220826091228.1701185-1-roberto.sassu@huaweicloud.com (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

Roberto Sassu Aug. 26, 2022, 9:12 a.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller set in the
argument only defined flags. Introduce it directly in include/linux/key.h,
to reduce the risk that the check is not in sync with currently defined
flags.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/key.h      | 16 ++++++++++++++++
 security/keys/internal.h |  2 --
 2 files changed, 16 insertions(+), 2 deletions(-)

Comments

Roberto Sassu Aug. 26, 2022, 9:22 a.m. UTC | #1
On Fri, 2022-08-26 at 11:12 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller set
> in the
> argument only defined flags. Introduce it directly in
> include/linux/key.h,
> to reduce the risk that the check is not in sync with currently
> defined
> flags.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: KP Singh <kpsingh@kernel.org>

Jarkko, could you please ack it if it is fine?

Thanks

Roberto

> ---
>  include/linux/key.h      | 16 ++++++++++++++++
>  security/keys/internal.h |  2 --
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 7febc4881363..e679dbf0c940 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -88,6 +88,22 @@ enum key_need_perm {
>  	KEY_DEFER_PERM_CHECK,	/* Special: permission check is
> deferred */
>  };
>  
> +#define KEY_LOOKUP_CREATE	0x01
> +#define KEY_LOOKUP_PARTIAL	0x02
> +
> +/**
> + * key_lookup_flags_valid - detect if provided key lookup flags are
> valid
> + * @flags: key lookup flags.
> + *
> + * Verify whether or not the caller set in the argument only defined
> flags.
> + *
> + * Return: true if flags are valid, false if not.
> + */
> +static inline bool key_lookup_flags_valid(u64 flags)
> +{
> +	return !(flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL));
> +}
> +
>  struct seq_file;
>  struct user_struct;
>  struct signal_struct;
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 9b9cf3b6fcbb..3c1e7122076b 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -165,8 +165,6 @@ extern struct key *request_key_and_link(struct
> key_type *type,
>  
>  extern bool lookup_user_key_possessed(const struct key *key,
>  				      const struct key_match_data
> *match_data);
> -#define KEY_LOOKUP_CREATE	0x01
> -#define KEY_LOOKUP_PARTIAL	0x02
>  
>  extern long join_session_keyring(const char *name);
>  extern void key_change_session_keyring(struct callback_head *twork);
Jarkko Sakkinen Aug. 28, 2022, 3:59 a.m. UTC | #2
On Fri, Aug 26, 2022 at 11:22:54AM +0200, Roberto Sassu wrote:
> On Fri, 2022-08-26 at 11:12 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller set
> > in the
> > argument only defined flags. Introduce it directly in
> > include/linux/key.h,
> > to reduce the risk that the check is not in sync with currently
> > defined
> > flags.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: KP Singh <kpsingh@kernel.org>
> 
> Jarkko, could you please ack it if it is fine?

So, as said I'm not really confident that a function is
even needed in the first place. It's fine if there are
enough call sites to make it legit.

BR, Jarkko
Jarkko Sakkinen Aug. 28, 2022, 4:03 a.m. UTC | #3
On Sun, Aug 28, 2022 at 06:59:41AM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 26, 2022 at 11:22:54AM +0200, Roberto Sassu wrote:
> > On Fri, 2022-08-26 at 11:12 +0200, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller set
> > > in the
> > > argument only defined flags. Introduce it directly in
> > > include/linux/key.h,
> > > to reduce the risk that the check is not in sync with currently
> > > defined
> > > flags.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > 
> > Jarkko, could you please ack it if it is fine?
> 
> So, as said I'm not really confident that a function is
> even needed in the first place. It's fine if there are
> enough call sites to make it legit.

And *if* a named constant is enough, you could probably
then just squash to the same patch that uses it, right?

If there overwhelming amount of call sites I do fully
get having a helper.

BR, Jarkko
Roberto Sassu Aug. 29, 2022, 7:25 a.m. UTC | #4
On Sun, 2022-08-28 at 07:03 +0300, Jarkko Sakkinen wrote:
> On Sun, Aug 28, 2022 at 06:59:41AM +0300, Jarkko Sakkinen wrote:
> > On Fri, Aug 26, 2022 at 11:22:54AM +0200, Roberto Sassu wrote:
> > > On Fri, 2022-08-26 at 11:12 +0200, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller
> > > > set
> > > > in the
> > > > argument only defined flags. Introduce it directly in
> > > > include/linux/key.h,
> > > > to reduce the risk that the check is not in sync with currently
> > > > defined
> > > > flags.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > 
> > > Jarkko, could you please ack it if it is fine?
> > 
> > So, as said I'm not really confident that a function is
> > even needed in the first place. It's fine if there are
> > enough call sites to make it legit.
> 
> And *if* a named constant is enough, you could probably
> then just squash to the same patch that uses it, right?

Yes, the constant seems better. Maybe, I would add in the same patch
that exports the lookup flags, since we have that.

Thanks

Roberto
Jarkko Sakkinen Aug. 29, 2022, 12:33 p.m. UTC | #5
On Mon, Aug 29, 2022 at 09:25:05AM +0200, Roberto Sassu wrote:
> On Sun, 2022-08-28 at 07:03 +0300, Jarkko Sakkinen wrote:
> > On Sun, Aug 28, 2022 at 06:59:41AM +0300, Jarkko Sakkinen wrote:
> > > On Fri, Aug 26, 2022 at 11:22:54AM +0200, Roberto Sassu wrote:
> > > > On Fri, 2022-08-26 at 11:12 +0200, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.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_valid() to check if the caller
> > > > > set
> > > > > in the
> > > > > argument only defined flags. Introduce it directly in
> > > > > include/linux/key.h,
> > > > > to reduce the risk that the check is not in sync with currently
> > > > > defined
> > > > > flags.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > > 
> > > > Jarkko, could you please ack it if it is fine?
> > > 
> > > So, as said I'm not really confident that a function is
> > > even needed in the first place. It's fine if there are
> > > enough call sites to make it legit.
> > 
> > And *if* a named constant is enough, you could probably
> > then just squash to the same patch that uses it, right?
> 
> Yes, the constant seems better. Maybe, I would add in the same patch
> that exports the lookup flags, since we have that.

Yeah, then it would be probably easier to review too
since it is "in the context".

BR, Jarkko
diff mbox series

Patch

diff --git a/include/linux/key.h b/include/linux/key.h
index 7febc4881363..e679dbf0c940 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -88,6 +88,22 @@  enum key_need_perm {
 	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
 };
 
+#define KEY_LOOKUP_CREATE	0x01
+#define KEY_LOOKUP_PARTIAL	0x02
+
+/**
+ * key_lookup_flags_valid - detect if provided key lookup flags are valid
+ * @flags: key lookup flags.
+ *
+ * Verify whether or not the caller set in the argument only defined flags.
+ *
+ * Return: true if flags are valid, false if not.
+ */
+static inline bool key_lookup_flags_valid(u64 flags)
+{
+	return !(flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL));
+}
+
 struct seq_file;
 struct user_struct;
 struct signal_struct;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 9b9cf3b6fcbb..3c1e7122076b 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -165,8 +165,6 @@  extern struct key *request_key_and_link(struct key_type *type,
 
 extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
-#define KEY_LOOKUP_CREATE	0x01
-#define KEY_LOOKUP_PARTIAL	0x02
 
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);