Re: [PATCH bpf-next 1/2] bpf: Take the end of the verifier log out and append it later

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: rolling log window corruption and bad NUL cut in
  bpf_vlog_unstash()
--

commit 77a6b70f8e6a46314ce083390eb008b9b5d0b118
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Take the end of the verifier log out and append it later

This commit adds bpf_vlog_stash() and bpf_vlog_unstash() to copy the tail
of the verifier log out of the user buffer, reset the log position, and
append the stashed text later.

> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> index 1253118dfd051..84659a757b43c 100644
> --- a/kernel/bpf/log.c
> +++ b/kernel/bpf/log.c
> @@ -177,6 +177,53 @@ void bpf_vlog_reset(struct bpf_verifier_log *log, u64 new_pos)
>  		log->ubuf = NULL;
>  }
>  
> +/*
> + * Take what was written since @pos out of the log. bpf_vlog_unstash() appends it to what is
> + * written in between. The text is in the buffer of the user and nowhere else. What the buffer
> + * doesn't hold, the start of the text if it's rolling and the end if it's fixed, is counted.
> + */
> +int bpf_vlog_stash(struct bpf_verifier_log *log, u64 pos, struct bpf_vlog_stash *stash)
> +{
> +	u64 from = max(pos, log->start_pos), to = log->end_pos, i;
> +	u32 off, n;
> +
> +	if (!log->ubuf)
> +		from = to;
> +	else if (log->level & BPF_LOG_FIXED)
> +		to = clamp(log->len_total - 1ULL, from, to);
> +	stash->lost_start = from - pos;
> +	stash->lost_end = log->end_pos - to;
> +	stash->len = to - from;
> +	/* as large as the buffer at worst: fail before the OOM killer is called */
> +	stash->buf = kvzalloc(stash->len, GFP_KERNEL_ACCOUNT | __GFP_NOWARN | __GFP_RETRY_MAYFAIL);
> +	if (!stash->buf)
> +		return -ENOMEM;
> +	/* two pieces if it wraps around */
> +	for (i = from; i < to && log->ubuf; i += n) {
> +		div_u64_rem(i, log->len_total, &off);
> +		n = min(to - i, log->len_total - off);
> +		if (copy_from_user(stash->buf + (i - from), log->ubuf + off, n))
> +			log->ubuf = NULL;
> +	}
> +	bpf_vlog_reset(log, pos);
> +	return 0;
> +}
> +
> +void bpf_vlog_unstash(struct bpf_verifier_log *log, struct bpf_vlog_stash *stash)
> +{
> +	u64 i, n;
> +
> +	log->end_pos += stash->lost_start;
> +	for (i = 0; i < stash->len; i += n) {
> +		n = min(stash->len - i, sizeof(log->kbuf) - 1);
> +		bpf_log(log, "%.*s", (int)n, stash->buf + i);
> +		cond_resched();
> +	}

[Severity: Medium]
Can advancing log->end_pos by stash->lost_start without updating
log->start_pos leave an unwritten gap inside the rolling log window?

When a rolling log has wrapped past pos (log->start_pos > pos),
bpf_vlog_stash() records stash->lost_start = log->start_pos - pos and calls
bpf_vlog_reset(log, pos), which resets both log->start_pos and log->end_pos
to pos.  After intermediate text is logged up to E1, bpf_vlog_unstash()
adds stash->lost_start to log->end_pos (advancing it to E2) without
writing to log->ubuf in [E1, E2) or advancing log->start_pos.

This relies on the replay loop emitting log->len_total - 1 bytes so
bpf_verifier_vlog() pushes log->start_pos up to E2.  If the loop emits
fewer bytes (for example, because another thread wrote a zero byte into the
user buffer, or bpf_vlog_reset() earlier shrank log->end_pos -
log->start_pos below log->len_total - 1), log->start_pos stays below E2 and
bpf_vlog_finalize() rotates the NUL terminator at E1 and stale bytes in
[E1, E2) into the middle of the caller's log buffer.

In addition, because the replay loop formats stash->buf in chunks of
sizeof(log->kbuf) - 1 bytes via "%.*s", a zero byte in one chunk only
truncates that chunk in vscnprintf() instead of cutting the rest of
stash->buf, while still advancing log->end_pos by fewer than stash->len
bytes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108053937.4134138-1-alexei.starovoitov@gmail.com?part=1