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_check() 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 | 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
+
+static inline int key_lookup_flags_check(u64 flags)
+{
+ if (flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL))
+ return -EINVAL;
+
+ return 0;
+}
+
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);
--
2.25.1On Thu, Aug 18, 2022 at 05:29:23PM +0200, roberto.sassu@huaweicloud.com 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_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@huawei.com>
> Reviewed-by: KP Singh <kpsingh@kernel.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
On Fri, 2022-08-26 at 08:42 +0300, Jarkko Sakkinen wrote:
> On Thu, Aug 18, 2022 at 05:29:23PM +0200,
> roberto.sassu@huaweicloud.com 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_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.
Hi Jarkko
sorry, forgot to update the commit description. Will do it.
> Also, the short subject is misleading because this *just*
> does not move flags.
>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: KP Singh <kpsingh@kernel.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))
Absolutely fine with that, if you prefer.
> 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?
>
Daniel preferred to have this check here.
Thanks
Roberto
On Fri, Aug 26, 2022 at 09:14:09AM +0200, Roberto Sassu wrote:
> On Fri, 2022-08-26 at 08:42 +0300, Jarkko Sakkinen wrote:
> > On Thu, Aug 18, 2022 at 05:29:23PM +0200,
> > roberto.sassu@huaweicloud.com 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_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.
>
> Hi Jarkko
>
> sorry, forgot to update the commit description. Will do it.
>
> > Also, the short subject is misleading because this *just*
> > does not move flags.
> >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: KP Singh <kpsingh@kernel.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))
>
> Absolutely fine with that, if you prefer.
It can be either, it more depends on if a new function
is needed in the first place.
E.g. if you are worried about maintaining you could just
as well define a constant containing the mask, right?
>
> > 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?
> >
>
> Daniel preferred to have this check here.
How many call sites?
BR, Jarkko
On Sun, Aug 28, 2022 at 5:57 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri, Aug 26, 2022 at 09:14:09AM +0200, Roberto Sassu wrote:
> > On Fri, 2022-08-26 at 08:42 +0300, Jarkko Sakkinen wrote:
> > > On Thu, Aug 18, 2022 at 05:29:23PM +0200,
> > > roberto.sassu@huaweicloud.com 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_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.
> >
> > Hi Jarkko
> >
> > sorry, forgot to update the commit description. Will do it.
> >
> > > Also, the short subject is misleading because this *just*
> > > does not move flags.
> > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: KP Singh <kpsingh@kernel.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))
> >
> > Absolutely fine with that, if you prefer.
>
> It can be either, it more depends on if a new function
> is needed in the first place.
>
> E.g. if you are worried about maintaining you could just
> as well define a constant containing the mask, right?
+1 A mask is better.
>
> >
> > > 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?
> > >
> >
> > Daniel preferred to have this check here.
>
> How many call sites?
>
> BR, Jarkko
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(-)
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);
--
2.25.1On 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);
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
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
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
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
© 2016 - 2026 Red Hat, Inc.