Skip to content

bpf: Fix u32 overflow issue in map batch operations - #13431

Open
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1150846=>bpf-next
Open

bpf: Fix u32 overflow issue in map batch operations#13431
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf-next_basefrom
series/1150846=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf: Fix u32 overflow issue in map batch operations
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 519307be687a17c183b187da955b4c1da18cac58
Author: Masoud Aghasi <maghasi@disroot.org>

bpf: Fix u32 overflow issue in map batch operations

Several map batch operation implementations such as
generic_map_lookup_batch() use calculations in the form of
"values + cp * map->value_size" to compute the desired userspace memory
address for reading or writing. This can overflow the u32 type
(the result of "cp * map->value_size") when the map size exceeds 4GB.

The patch resolves the mentioned issues by converting the cp's type
to size_t in the affected places.

> Fixes: cb4d03ab499d ("bpf: Add generic support for lookup batch op")

The single Fixes: tag covers only one of the three commits whose code this
patch corrects.

cb4d03ab499d introduced generic_map_lookup_batch() - that tag is correct
for the changes in kernel/bpf/syscall.c at generic_map_lookup_batch().

The other hunks fix different commits:

generic_map_delete_batch() and generic_map_update_batch() were added by
aa2e93b8e58e ("bpf: Add generic support for update and delete batch ops"),
which introduced the buggy expressions:

    +       key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);
    +       if (copy_from_user(value, values + cp * value_size, value_size))

__htab_map_lookup_and_delete_batch() was added by 057996380a42 ("bpf: Add
batch ops to all htab bpf map"), which introduced:

    +   if (bucket_cnt && (copy_to_user(ukeys + total * key_size, keys,
    +       key_size * bucket_cnt) ||
    +       copy_to_user(uvalues + total * value_size, values,

Without these tags the stable backport scope is wrong: a tree that has
aa2e93b8e58e or 057996380a42 but predates cb4d03ab499d (or vice versa)
will not be matched.

Would it make sense to add:

  Fixes: aa2e93b8e58e ("bpf: Add generic support for update and delete batch ops")
  Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map")

> This patch resolves the mentioned issues by converting the cp's type
> to size_t in the effected places.

The changelog misdescribes what the patch does, in two ways.

First, no type is converted. The diff only adds (size_t) casts at the use
sites; every declaration is untouched. generic_map_lookup_batch() still
declares "u32 value_size, cp, max_count;" at kernel/bpf/syscall.c,
generic_map_update_batch() still declares "u32 value_size, cp, max_count;",
and generic_map_delete_batch() still declares "u32 cp, max_count;".

This matters beyond wording, because actually converting cp to size_t would
have been a bug: all three functions report progress with

    copy_to_user(&uattr->batch.count, &cp, sizeof(cp))

and uattr->batch.count is __u32 in the uapi, so a size_t cp would write
8 bytes into a 4-byte field on 64-bit. Casting at the use site, as the
patch actually does, is the correct approach - the changelog should
describe it that way so a reviewer does not "simplify" it later.

Second, there is no cp in the hashtab.c hunk. __htab_map_lookup_and_delete_batch()
uses total (declared "u32 bucket_cnt, total, key_size, value_size;" at
kernel/bpf/hashtab.c), so "the cp's type" does not describe that change.

(Also "effected places" should read "affected places".)



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: Fix u32 overflow issue in map batch operations
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32723998558

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d83fba2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: ce36e38
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 05ea1b6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 1555de3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 48b69cc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 48b69cc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e875ae
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

Several map batch operation implementations such as
generic_map_lookup_batch() use calculations in the form of
"values + cp * map->value_size" to compute the desired userspace memory
address for reading or writing. This can overflow the u32 type
(the result of "cp * map->value_size") when the map size exceeds 4GB.

generic_map_lookup_batch() may corrupt values for some keys in
userspace memory, and in some cases it mismatches values for some keys
while still reporting success.

Other batch operations may fail to delete or update some keys,
or the syscall may return unexpected errors.

This patch resolves the mentioned issues by converting the cp's type
to size_t in the effected places.

I created a BPF and a userspace C program to demonstrate the issue.
Example BPF program:
```c

struct my_value {
  char buf[0x10000000];
};

struct {
  __uint(type, BPF_MAP_TYPE_ARRAY);
  __uint(max_entries, 17);
  __type(key, int);
  __type(value, struct my_value);
} map SEC(".maps");

char LICENSE[] SEC("license") = "GPL";
```

Example userspace program:
```c

int main(int argc, char *argv[]) {
  struct bpf_object *obj;
  char filename[256];
  int err, ret = 0;
  struct bpf_map *map;
  int map_fd;
  const __u64 max_entries = 17;
  const __u64 value_size = 0x10000000; // 256MB
  char *values, *keys;
  __u32 count, out_batch;

  if (argc != 2) {
    printf("Usage: %s [bpf_prog.o]", argv[0]);
    return EXIT_FAILURE;
  }

  snprintf(filename, sizeof(filename), "%s", argv[1]);
  obj = bpf_object__open(filename);
  if (libbpf_get_error(obj)) {
    printf("BPF open failed!\n");
    return EXIT_FAILURE;
  }

  values = calloc(max_entries, value_size);
  if (!values) {
    printf("calloc values failed!\n");
    goto err_out;
  }

  keys = calloc(max_entries, sizeof(__u32));
  if (!keys) {
    printf("calloc keys failed!\n");
    goto err_out;
  }

  err = bpf_object__load(obj);
  if (err) {
    printf("BPF load failed! err:%d\n", err);
    goto err_out;
  }

  map = bpf_object__find_map_by_name(obj, "map");
  if (!map) {
    printf("map not found!\n");
    goto err_out;
  }

  map_fd = bpf_map__fd(map);
  if (map_fd < 0) {
    printf("invalid map FD!\n");
    goto err_out;
  }

  __u32 key;
  for (__u64 i = 0; i < max_entries; i++) {
    memset(values + (i * value_size), i & 0xFF, value_size);
    key = i;
    err = bpf_map_update_elem(map_fd, &key, values + (i * value_size), BPF_ANY);
    if (err) {
      printf("bpf_map_update_elem failed: %d\n", err);
      goto err_out;
    }
  }

  count = max_entries;
  err = bpf_map_lookup_batch(map_fd, NULL, &out_batch, keys, values, &count,
                             NULL);
  if (err) {
    printf("bpf_map_lookup_batch failed: %d\n", err);
    goto err_out;
  }

  printf("count: %u, out_batch: %u\n", count, out_batch);

  for (__u64 i = 0; i < max_entries; i++) {
    for (__u64 j = 0; j < value_size; j++) {
      if (values[(i * value_size) + j] != (unsigned char)(i & 0xFF)) {
        printf("Invalid map entry, key: %u, value: %hhu, i: %llu, j: %llu\n",
               *(((__u32 *)keys) + i), values[(i * value_size) + j], i, j);
        goto err_out;
      }
    }
  }

  printf("Finished with no errors!\n");
  ret = EXIT_SUCCESS;
  goto out;

err_out:
  ret = EXIT_FAILURE;

out:
  bpf_object__close(obj);
  if (values)
    free(values);
  if (keys)
    free(keys);
  return ret;
}
```

The test needs enough free memory (around 14GB for the total system).
The key/value mismatch occurs when max_entries is 17 and value_size
is 0x10000000 (256MB). Reducing max_entries to 16 eliminates the issue.

Fixes: cb4d03a ("bpf: Add generic support for lookup batch op")

Signed-off-by: Masoud Aghasi <maghasi@disroot.org>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: cd35e1b
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150846
version: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant