On Thu, 2025-07-31 at 10:03 -0700, Alexei Starovoitov wrote:
> On Wed, Jul 30, 2025 at 10:32 AM James Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > bpf_key.has_ref is used to distinguish between real key pointers
> > and
> > the fake key pointers that are used for system keyrings (to ensure
> > the
> > actual pointers to system keyrings are never visible outside
> > certs/system_keyring.c). The keyrings subsystem has an exported
> > function to do this, so use that in the bpf keyring code
> > eliminating
> > the need to store has_ref.
> >
> > Signed-off-by: James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > v2: use unsigned long for pointer to int conversion
> > ---
> > kernel/trace/bpf_trace.c | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index e7bf00d1cd05..c0ccd55a4d91 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1244,7 +1244,6 @@ static const struct bpf_func_proto
> > bpf_get_func_arg_cnt_proto = {
> > #ifdef CONFIG_KEYS
> > struct bpf_key {
> > struct key *key;
> > - bool has_ref;
> > };
> >
> > __bpf_kfunc_start_defs();
> > @@ -1297,7 +1296,6 @@ __bpf_kfunc struct bpf_key
> > *bpf_lookup_user_key(s32 serial, u64 flags)
> > }
> >
> > bkey->key = key_ref_to_ptr(key_ref);
> > - bkey->has_ref = true;
> >
> > return bkey;
> > }
> > @@ -1335,7 +1333,6 @@ __bpf_kfunc struct bpf_key
> > *bpf_lookup_system_key(u64 id)
> > return NULL;
> >
> > bkey->key = (struct key *)(unsigned long)id;
> > - bkey->has_ref = false;
> >
> > return bkey;
> > }
> > @@ -1349,7 +1346,7 @@ __bpf_kfunc struct bpf_key
> > *bpf_lookup_system_key(u64 id)
> > */
> > __bpf_kfunc void bpf_key_put(struct bpf_key *bkey)
> > {
> > - if (bkey->has_ref)
> > + q
> > key_put(bkey->key);
>
> Should be (u64) to avoid truncation ?
It can't be: gcc only allows pointer to unsigned long conversion, so
the statement
if (system_keyring_id_check((u64)bkey->key) < 0)
produces a pointer to int conversion error. Since the function
prototype is u64 the conversion from unsigned long to u64 happens
automatically.
> But is it really the case that id==1 and id==2 are exposed to UAPI
> already?
>
> As far as I can see lookup_user_key() does:
> default:
> key_ref = ERR_PTR(-EINVAL);
> if (id < 1)
> goto error;
>
> key = key_lookup(id);
>
> so only id==0 is invalid, but id=1 can be a valid user key, no?
Well, remember the id as pointer trick is only used for the system_key
lookup. What you get back from user_key lookup is a real pointer to
the key (regardless of what serial id you pass in) so there's no chance
of getting 1 or 2 back for a user key.
However, if you were thinking of overloading key look up, it is
currently the case, in spite of the check in lookup above, that user
key serial numbers begin at three thanks to this code in
key.c:key_alloc_serial()
do {
get_random_bytes(&key->serial, sizeof(key->serial));
key->serial >>= 1; /* negative numbers are not permitted */
} while (key->serial < 3);
David said he would prefer, if we to allow system keyring lookup here,
to use negative ids (like keyrings) for them.
Regards,
James
