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

[KP Singh]

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?

        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;
    }




- KP