Re: [PATCH bpf] bpf: fix classic bpf reads from negative offset outside of linear skb portion

[Daniel Borkmann]

On 1/22/25 9:04 PM, Maciej Żenczykowski wrote:
> We're received reports of cBPF code failing to accept DHCP packets.
> "BPF filter for DHCP not working (android14-6.1-lts + android-14.0.0_r74)"

I presume this is cBPF on AF_PACKET, right?

> The relevant Android code is at:
>    https://cs.android.com/android/platform/superproject/main/+/main:packages/modules/NetworkStack/jni/network_stack_utils_jni.cpp;l=95;drc=9df50aef1fd163215dcba759045706253a5624f5
> which uses a lot of macros from:
>    https://cs.android.com/android/platform/superproject/main/+/main:packages/modules/Connectivity/bpf/headers/include/bpf/BpfClassic.h;drc=c58cfb7c7da257010346bd2d6dcca1c0acdc8321
>
> This is widely used and does work on the vast majority of drivers,
> but is exposing a core kernel cBPF bug related to driver skb layout.
>
> Root cause is iwlwifi driver, specifically on (at least):
>    Dell 7212: Intel Dual Band Wireless AC 8265
>    Dell 7220: Intel Wireless AC 9560
>    Dell 7230: Intel Wi-Fi 6E AX211
> delivers frames where the UDP destination port is not in the skb linear
> portion, while the cBPF code is using SKF_NET_OFF relative addressing.
>
> simplified from above, effectively:
>    BPF_STMT(BPF_LDX | BPF_B | BPF_MSH, SKF_NET_OFF)
>    BPF_STMT(BPF_LD  | BPF_H | BPF_IND, SKF_NET_OFF + 2)
>    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 68, 1, 0)
>    BPF_STMT(BPF_RET | BPF_K, 0)
>    BPF_STMT(BPF_RET | BPF_K, 0xFFFFFFFF)
> fails to match udp dport=68 packets.
>
> Specifically the 3rd cBPF instruction fails to match the condition:
>    if (ptr >= skb->head && ptr + size <= skb_tail_pointer(skb))
> within bpf_internal_load_pointer_neg_helper() and thus returns NULL,
> which results in reading -EFAULT.
>
> This is because bpf_skb_load_helper_{8,16,32} don't include the
> "data past headlen do skb_copy_bits()" logic from the non-negative
> offset branch in the negative offset branch.
>
> Note: I don't know sparc assembly, so this doesn't fix sparc...
> ideally we should just delete bpf_internal_load_pointer_neg_helper()
> This seems to have always been broken (but not pre-git era, since
> obviously there was no eBPF helpers back then), but stuff older
> than 5.4 is no longer LTS supported anyway, so using 5.4 as fixes tag.
>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> Cc: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> Cc: Willem de Bruijn <willemb@xxxxxxxxxx>
> Reported-by: Matt Moeller <moeller.matt@xxxxxxxxx>
> Closes: https://issuetracker.google.com/384636719 [Treble - GKI partner internal]
> Signed-off-by: Maciej Żenczykowski <maze@xxxxxxxxxx>
> Fixes: 219d54332a09 ("Linux 5.4")

Hm, so not strictly broken, just that using relative SKF_NET_OFF offset
is limited in that it requires the data to be in linear section. Some
potential workarounds that come to mind:

1) When you say vast majority of drivers, have you checked how much they
   typically pull into linear section and whether it would make sense also
   for iwlwifi to do the same? (It sounds like start of network header is
   already in non-linear part for iwlwifi?)

2) Perhaps rework the filter to avoid relying on SKF_NET_OFF.. tradeoff
   are more instructions, e.g.,

  # tcpdump -dd udp dst port 68
  { 0x28, 0, 0, 0x0000000c },
  { 0x15, 0, 4, 0x000086dd },
  { 0x30, 0, 0, 0x00000014 },
  { 0x15, 0, 11, 0x00000011 },
  { 0x28, 0, 0, 0x00000038 },
  { 0x15, 8, 9, 0x00000044 },
  { 0x15, 0, 8, 0x00000800 },
  { 0x30, 0, 0, 0x00000017 },
  { 0x15, 0, 6, 0x00000011 },
  { 0x28, 0, 0, 0x00000014 },
  { 0x45, 4, 0, 0x00001fff },
  { 0xb1, 0, 0, 0x0000000e },
  { 0x48, 0, 0, 0x00000010 },
  { 0x15, 0, 1, 0x00000044 },
  { 0x6, 0, 0, 0x00040000 },
  { 0x6, 0, 0, 0x00000000 },

3) Use eBPF asm, e.g. you can pull in data into linear section via helper
   bpf_skb_pull_data() if needed, or use bpf_skb_load_bytes() which works
   for linear & non-linear data.

4) What about pulling in linear data in AF_PACKET code right before the
   cBPF filter is run (perhaps usage of SKF_NET_OFF could be detected and
   then only done if really needed by the filter)?

Thanks,
Daniel