On Mon, Jun 9, 2025 at 11:30 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Jun 6, 2025 at 4:29 PM KP Singh <kpsingh@xxxxxxxxxx> 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
