On Thu, Jul 14, 2022 at 9:58 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Jul 14, 2022 at 02:43:02PM -0700, Andrii Nakryiko wrote:
> > If BPF array map is bigger than 4GB, element pointer calculation can
> > overflow because both index and elem_size are u32. Fix this everywhere
> > by forcing 64-bit multiplication. Extract this formula into separate
> > small helper and use it consistently in various places.
> >
> > Speculative-preventing formula utilizing index_mask trick is left as is,
> > but explicit u64 casts are added in both places.
> >
> > Fixes: c85d69135a91 ("bpf: move memory size checks to bpf_map_charge_init()")
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > kernel/bpf/arraymap.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> > index fe40d3b9458f..d3dfc28dbb05 100644
> > --- a/kernel/bpf/arraymap.c
> > +++ b/kernel/bpf/arraymap.c
> > @@ -156,6 +156,11 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
> > return &array->map;
> > }
> >
> > +static void *array_map_elem_ptr(struct bpf_array* array, u32 index)
> > +{
> > + return array->value + (u64)array->elem_size * (index & array->index_mask);
> > +}
> > +
> > /* Called from syscall or from eBPF program */
> > static void *array_map_lookup_elem(struct bpf_map *map, void *key)
> > {
> > @@ -165,7 +170,7 @@ static void *array_map_lookup_elem(struct bpf_map *map, void *key)
> > if (unlikely(index >= array->map.max_entries))
> > return NULL;
> >
> > - return array->value + array->elem_size * (index & array->index_mask);
> > + return array->value + (u64)array->elem_size * (index & array->index_mask)
> > }
> >
> > static int array_map_direct_value_addr(const struct bpf_map *map, u64 *imm,
> > @@ -339,7 +344,7 @@ static int array_map_update_elem(struct bpf_map *map, void *key, void *value,
> > value, map->value_size);
> > } else {
> > val = array->value +
> > - array->elem_size * (index & array->index_mask);
> > + (u64)array->elem_size * (index & array->index_mask);
> > if (map_flags & BPF_F_LOCK)
> > copy_map_value_locked(map, val, value, false);
> > else
> > @@ -408,8 +413,7 @@ static void array_map_free_timers(struct bpf_map *map)
> > return;
> >
> > for (i = 0; i < array->map.max_entries; i++)
> > - bpf_timer_cancel_and_free(array->value + array->elem_size * i +
> > - map->timer_off);
> > + bpf_timer_cancel_and_free(array_map_elem_ptr(array, i) + map->timer_off);
> > }
> >
> > /* Called when map->refcnt goes to zero, either from workqueue or from syscall */
> > @@ -420,7 +424,7 @@ static void array_map_free(struct bpf_map *map)
> >
> > if (map_value_has_kptrs(map)) {
> > for (i = 0; i < array->map.max_entries; i++)
> > - bpf_map_free_kptrs(map, array->value + array->elem_size * i);
> > + bpf_map_free_kptrs(map, array_map_elem_ptr(array, i));
>
> This is incorrect. There is no need to mask pointer here.
> There is no security issue here.
Hm.. not sure what happened, but this is not the version of the patch
that I intended to send, sorry about that. I'll resend correct
version.
