On Thu, Jul 31, 2025 at 10:27 AM James Bottomley
<james.bottom...@hansenpartnership.com> wrote:
>
> On Thu, 2025-07-31 at 10:03 -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 30, 2025 at 10:32 AM James Bottomley
> > <james.bottom...@hansenpartnership.com> 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.bottom...@hansenpartnership.com>
> > >
> > > ---
> > > 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);
I see. That's what I was missing.
> David said he would prefer, if we to allow system keyring lookup here,
> to use negative ids (like keyrings) for them.
Makes sense to me as well.
Do you want to do a follow up or respin this set ?
Would be great if he can ack this set too.
