Re: [PATCH bpf] bpf: mask speculative out-of-bounds skb helper loads

[bot+bpf-ci]

> bpf: mask speculative out-of-bounds skb helper loads

TL;DR: The nospec masking may be incomplete: inline classic BPF ABS/MSH loads
from convert_bpf_ld_abs() on efficient-unaligned-access architectures bypass
the helpers, so a short packet could still transiently read out of bounds.

> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> +static __always_inline const void *
> +bpf_skb_load_nospec(const struct sk_buff *skb, const void *data,
> +		    int headlen, int offset, int len)
> +{

[ ... ]

> +	/* Out-of-bounds transient offsets select NULL as the load address. */
> +	return (const void *)(((unsigned long)data + (long)offset) & mask);
> +}
> +
>  BPF_CALL_4(bpf_skb_load_helper_8, const struct sk_buff *, skb, const void *,
>  	   data, int, headlen, int, offset)
>  {

[ ... ]

>  	if (headlen - offset >= len)
> -		return *(u8 *)(data + offset);
> +		return *(const u8 *)bpf_skb_load_nospec(skb, data, headlen,
> +							  offset, len);

This isn't a bug in the hunk itself, but is the mitigation complete?

On architectures with HAVE_EFFICIENT_UNALIGNED_ACCESS, a classic
BPF_LD|BPF_ABS load with k >= 0, and BPF_LDX|BPF_MSH|BPF_B, do not reach
these helpers.  convert_bpf_ld_abs() emits an inline check and load
instead:

    TMP = H - k;
    if (TMP s< size) goto helper;
    A = *(size *)(D + k);   /* BPF_REG_D == cached skb->data */

This is the same bounds-check-then-load pattern described in the commit
message, and nothing masks it.  An unprivileged SO_ATTACH_FILTER program
chooses k, and packet size controls the linear length.  After training the
branch with long packets, a short packet could transiently load from
skb->data + k past the linear area.  The value could then go through
BPF_LSH/BPF_TAX into a BPF_IND load, which reaches
bpf_skb_load_helper_*() and passes the new mask because the shifted index
is in range.

The commit message says "For indirect loads, packet data can determine the
offset", yet the patch also masks ABS loads that go through the helpers.

Should convert_bpf_ld_abs() get the same treatment (for example, always
calling the helpers, or masking D + k in the emitted code), or should the
changelog explain why constant-offset loads are out of scope?

[ ... ]


---
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

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/43500835329