Re: [PATCH 07/12] bpf: Return hashes of maps in BPF_OBJ_GET_INFO_BY_FD

[KP Singh <kpsingh@kernel.org>]

On Wed, Jun 11, 2025 at 5:04 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jun 11, 2025 at 7:27 AM KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Mon, Jun 9, 2025 at 11:30 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Jun 6, 2025 at 4:29 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > [...]
> >
> > > >
> > > > +       if (map->ops->map_get_hash && map->frozen && map->excl_prog_sha) {
> > > > +               err = map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, &map->sha);
> > >
> > > & in &map->sha looks suspicious. Should be just map->sha ?
> >
> > yep, fixed.
> >
> > >
> > > > +               if (err != 0)
> > > > +                       return err;
> > > > +       }
> > > > +
> > > > +       if (info.hash) {
> > > > +               char __user *uhash = u64_to_user_ptr(info.hash);
> > > > +
> > > > +               if (!map->ops->map_get_hash)
> > > > +                       return -EINVAL;
> > > > +
> > > > +               if (info.hash_size < SHA256_DIGEST_SIZE)
> > >
> > > Similar to prog let's == here?
> >
> > Thanks, yeah agreed.
> >
> > >
> > > > +                       return -EINVAL;
> > > > +
> > > > +               info.hash_size  = SHA256_DIGEST_SIZE;
> > > > +
> > > > +               if (map->excl_prog_sha && map->frozen) {
> > > > +                       if (copy_to_user(uhash, map->sha, SHA256_DIGEST_SIZE) !=
> > > > +                           0)
> > > > +                               return -EFAULT;
> > >
> > > I would drop above and keep below part only.
> > >
> > > > +               } else {
> > > > +                       u8 sha[SHA256_DIGEST_SIZE];
> > > > +
> > > > +                       err = map->ops->map_get_hash(map, SHA256_DIGEST_SIZE,
> > > > +                                                    sha);
> > >
> > > Here the kernel can write into map->sha and then copy it to uhash.
> > > I think the concern was to disallow 2nd map_get_hash on exclusive
> > > and frozen map, right?
> > > But I think that won't be an issue for signed lskel loader.
> > > Since the map is frozen the user space cannot modify it.
> > > Since the map is exclusive another bpf prog cannot modify it.
> > > If user space calls map_get_hash 2nd time the sha will be
> > > exactly the same until loader prog writes into the map.
> > > So I see no harm generalizing this bit of code.
> > > I don't have a particular use case in mind,
> > > but it seems fine to allow user space to recompute sha
> > > of exclusive and frozen map.
> > > The loader will check the sha of its map as the very first operation,
> > > so if user space did two map_get_hash() it just wasted cpu cycles.
> > > If user space is calling map_get_hash() while loader prog
> > > reads and writes into it the map->sha will change, but
> > > it doesn't matter to the loader program anymore.
> > >
> > > Also I wouldn't special case the !info.hash case for exclusive maps.
> > > It seems cleaner to waste few bytes on stack in
> > > skel_obj_get_info_by_fd() later in patch 9.
> > > Let it point to valid u8 sha[] on stack.
> > > The skel won't use it, but this way we can kernel behavior
> > > consistent.
> > > if info.hash != NULL -> compute sha, update map->sha, copy to user space.
> >
> > Here's what I updated it to:
> >
> >     if (info.hash) {
> >         char __user *uhash = u64_to_user_ptr(info.hash);
> >
> >         if (!map->ops->map_get_hash)
> >             return -EINVAL;
> >
> >         if (info.hash_size != SHA256_DIGEST_SIZE)
> >             return -EINVAL;
> >
> >         if (!map->excl_prog_sha || !map->frozen)
> >             return -EINVAL;
> >
> >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >          I think we still need this check as we want the program to
> > have exclusive control over the map when the hash is being calculated
> > right?
>
> Why add such a restriction?
> Whether it's frozen or exclusive or both it still races with map_get_hash.
> It's up to the user to make sure that the computed hash
> will be meaningful.

Sure, yeah. I removed the check, they can use the hash in many ways,
even if racy.

- KP

> I would allow for all maps.