[PATCH bpf 0/2] bpf: Fix ring buffer checks on 32-bit position wrap

[PATCH bpf 0/2] bpf: Fix ring buffer checks on 32-bit position wrap

[Igor Malovitsa]

Two more places where ring buffer positions are compared in a way that
breaks once they wrap on a 32-bit kernel. Two earlier fixes of the same
kind are commit 6ff5b56a50c5
("bpf: Fix pending_pos walk on 32-bit ring position wrap") and
commit 3f611e9b820e
("bpf: Fix available-data accounting on 32-bit wrap in overwrite mode").
The remaining two:

- __bpf_ringbuf_reserve() reads consumer_pos before taking the lock, so
  a reservation that stalls long enough can pass the space check with a
  value about 2^32 bytes stale and overwrite records the consumer has
  not read.

- __bpf_user_ringbuf_peek() widens the positions to u64 and compares
  them, so once producer_pos wraps and consumer_pos has not, the ring
  reads as empty from then on and the user-space producer gets -ENOSPC.

Both need about 4 GiB of ring traffic on a 32-bit kernel. Neither
changes anything on 64-bit.

How they were found and tested: a formal model of the ring buffer
protocol, written in Lean on the ORC11 memory model with the help of an
LLM coding assistant, showed both executions. Each was then confirmed on
32-bit QEMU guests (i386, arm and riscv32) by a test-only initcall (not
part of this series) that puts the positions in the wrapped state and
calls the real __bpf_ringbuf_reserve() and __bpf_user_ringbuf_peek():
without the patches it reports both bugs, with them both cases behave
correctly. With each patch, kernel/bpf/ringbuf.c builds without warnings
with W=1 on x86_64 and i386.

No selftest is added: reaching the states needs about 4 GiB of traffic
or instrumentation that only a test build can have.

The ring buffer selftests pass with the series applied on x86_64,
aarch64 and riscv64 (vmtest.sh, test_progs -t ringbuf: ringbuf,
ringbuf_multi, user_ringbuf and verifier_ringbuf, 33 subtests), and on
x86_64 without it. On riscv64 QEMU ran with -cpu max: the cross-compiled
test_progs uses Zbb/Zcb instructions that the CPU vmtest.sh picks lacks.

Reproducer (test-only, not for merging): a late_initcall in ringbuf.c,
built for i386 (tinyconfig), arm and riscv32 (their defconfigs), each
with BPF_SYSCALL, and booted in QEMU. For the first bug, a hook between
the consumer_pos load and the lock in __bpf_ringbuf_reserve() stands in
for the stalled reservation: it moves the positions to where 4 GiB of
traffic would leave them, so the load before the lock still holds the
old 0. Condensed, without the message prefix:

  /* hook, called just before raw_res_spin_lock_irqsave() */
  WRITE_ONCE(rb->consumer_pos, ULONG_MAX - PAGE_SIZE + 1);
  WRITE_ONCE(rb->producer_pos, ULONG_MAX - 7);
  WRITE_ONCE(rb->pending_pos, ULONG_MAX - 7);

  /* initcall: the ring is full, so the reservation must fail */
  rb = bpf_ringbuf_alloc(PAGE_SIZE, NUMA_NO_NODE, false);
  if (__bpf_ringbuf_reserve(rb, 0))
      pr_emerg("cons-under-lock: BUG reproduced (reservation accepted)\n");
  else
      pr_emerg("cons-under-lock: fixed (reservation rejected)\n");

  /* one empty sample at consumer_pos, producer_pos just past the wrap */
  rb = bpf_ringbuf_alloc(PAGE_SIZE, NUMA_NO_NODE, false);
  rb->consumer_pos = ULONG_MAX - 7;
  rb->producer_pos = 0;
  hdr = (void *)rb->data + (rb->consumer_pos & rb->mask);
  hdr->len = 0;
  hdr->pg_off = 0;
  err = __bpf_user_ringbuf_peek(rb, &sample, &size);
  if (err == -ENODATA)
      pr_emerg("user-ringbuf-wrap: BUG reproduced (false empty)\n");
  else if (!err && size == 0)
      pr_emerg("user-ringbuf-wrap: fixed (sample found)\n");

Output without this series:

  BPF_RINGBUF_REPRO: cons-under-lock: BUG reproduced (reservation accepted)
  BPF_RINGBUF_REPRO: user-ringbuf-wrap: BUG reproduced (false empty)

and with it:

  BPF_RINGBUF_REPRO: cons-under-lock: fixed (reservation rejected)
  BPF_RINGBUF_REPRO: user-ringbuf-wrap: fixed (sample found)

Igor Malovitsa (2):
  bpf: Load ringbuf consumer_pos under the spinlock
  bpf: Fix user ringbuf empty check on 32-bit position wrap

 kernel/bpf/ringbuf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)


base-commit: ff47652a4b66c067c765a7ad464d930b5a9367cc
-- 
2.53.0

[PATCH bpf 1/2] bpf: Load ringbuf consumer_pos under the spinlock

[Igor Malovitsa]

__bpf_ringbuf_reserve() loads consumer_pos before taking the spinlock.
Nothing bounds how stale that value is when the lock is finally taken:
other producers may have reserved, and the consumer consumed, any
amount of data meanwhile. bpf_ringbuf_has_space() compares
new_prod_pos - cons_pos as unsigned long, so on 32-bit, once positions
wrap, a value close to 2^32 bytes stale passes the check and the new
record is written over records the consumer has not read yet.

Load consumer_pos after taking the lock. producer_pos cannot move while
the lock is held and the consumer never passes producer_pos, so the
value is less than a ring behind and the difference is exact.

This needs a reservation to stall for about 4 GiB of traffic on a
32-bit kernel; it was found with a formal model of the protocol,
and reproduced using late_initcall.

Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier support for it")
Assisted-by: LLM
Signed-off-by: Igor Malovitsa <igor.malovitsa@gmail.com>
---
 kernel/bpf/ringbuf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index 3f1013d80544..6b86c4db5108 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -473,11 +473,12 @@ static void *__bpf_ringbuf_reserve(struct bpf_ringbuf *rb, u64 size)
 	if (len > ringbuf_total_data_sz(rb))
 		return NULL;
 
-	cons_pos = smp_load_acquire(&rb->consumer_pos);
-
 	if (raw_res_spin_lock_irqsave(&rb->spinlock, flags))
 		return NULL;
 
+	/* Under the lock, so at most one ring behind producer_pos. */
+	cons_pos = smp_load_acquire(&rb->consumer_pos);
+
 	pend_pos = rb->pending_pos;
 	prod_pos = rb->producer_pos;
 	new_prod_pos = prod_pos + len;
-- 
2.53.0

[PATCH bpf 2/2] bpf: Fix user ringbuf empty check on 32-bit position wrap

[Igor Malovitsa]

__bpf_user_ringbuf_peek() copies the unsigned long producer_pos and
consumer_pos into u64 locals and tests cons_pos >= prod_pos for an
empty ring. On 32-bit, once producer_pos wraps past 2^32 and
consumer_pos has not, every drain sees an empty ring and the user-space
producer gets -ENOSPC from then on. The fit check total_len >
prod_pos - cons_pos is wrong in the same state.

Keep the positions in unsigned long and test emptiness through their
difference, as the kernel-producer side does since commit 6ff5b56a50c5
("bpf: Fix pending_pos walk on 32-bit ring position wrap"). On 64-bit
nothing changes: unsigned long is u64, and the signed test differs from
cons_pos >= prod_pos only for positions 2^63 bytes apart.

Fixes: 205715673844 ("bpf: Add bpf_user_ringbuf_drain() helper")
Assisted-by: LLM
Signed-off-by: Igor Malovitsa <igor.malovitsa@gmail.com>
---
 kernel/bpf/ringbuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index 6b86c4db5108..b79cc433d2c6 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -750,7 +750,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
 {
 	int err;
 	u32 hdr_len, sample_len, total_len, flags, *hdr;
-	u64 cons_pos, prod_pos;
+	unsigned long cons_pos, prod_pos;
 
 	/* Synchronizes with smp_store_release() in user-space producer. */
 	prod_pos = smp_load_acquire(&rb->producer_pos);
@@ -759,7 +759,7 @@ static int __bpf_user_ringbuf_peek(struct bpf_ringbuf *rb, void **sample, u32 *s
 
 	/* Synchronizes with smp_store_release() in __bpf_user_ringbuf_sample_release() */
 	cons_pos = smp_load_acquire(&rb->consumer_pos);
-	if (cons_pos >= prod_pos)
+	if ((long)(prod_pos - cons_pos) <= 0)
 		return -ENODATA;
 
 	hdr = (u32 *)((uintptr_t)rb->data + (uintptr_t)(cons_pos & rb->mask));
-- 
2.53.0