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

[Alexei Starovoitov]

On Wed, Jun 11, 2025 at 7:27 AM KP Singh <kpsingh at kernel.org> wrote:
>
> On Mon, Jun 9, 2025 at 11:30 PM Alexei Starovoitov
> <alexei.starovoitov at gmail.com> wrote:
> >
> > On Fri, Jun 6, 2025 at 4:29 PM KP Singh <kpsingh at 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.
I would allow for all maps.
The callback will work for arrays initially, but that
can be improved in the future.

>         err = map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, map->sha);
>         if (err != 0)
>             return err;
>
>         if (copy_to_user(uhash, map->sha, SHA256_DIGEST_SIZE) != 0)
>             return -EFAULT;
>     } else if (info.hash_size) {
>         return -EINVAL;
>     }

yep.