Re: [PATCH bpf] bpf: reject dev-bound-only programs on other devices

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 09/18, bestswngs@gmail.com wrote:
> From: Weiming Shi <bestswngs@gmail.com>
> 
> __bpf_offload_dev_match() falls back to comparing offdev pointers after an
> exact netdev mismatch.  Bound-only programs normally have NULL offdevs, so
> unrelated netdevs compare equal; one loaded on an offload-registered netdev
> can instead inherit a real offdev and match a sibling port.  An attacker
> can use bpf(BPF_LINK_CREATE) with a different target ifindex to run
> metadata kfuncs specialized for the bound driver on the target driver's
> xdp_buff.  Running a veth-bound program on tun reads beyond tun's bare
> stack xdp_buff as a veth_xdp_buff.
> 
>   Oops: general protection fault, probably for non-canonical address
>   KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
>   RIP: 0010:veth_xdp_rx_timestamp (drivers/net/veth.c:1673)
>   Call Trace:
>    ...
>    tun_build_skb (drivers/net/tun.c:1739)
>    tun_get_user (drivers/net/tun.c:1856)
>    tun_chr_write_iter (drivers/net/tun.c:2091)
>    vfs_write (fs/read_write.c:595 fs/read_write.c:687)
>    ksys_write (fs/read_write.c:739)
>    do_syscall_64 (arch/x86/entry/syscall_64.c:84)
>    entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:121)
>   Kernel panic - not syncing: Fatal exception in interrupt
> 
> Restrict non-offloaded programs to exact netdev matches and retain the
> shared-offdev fallback only for genuinely offloaded multi-port programs.

Any chance you can also add a testcase for this under
tools/testing/selftests/net/bpf_offload.py?

> Cc: stable@vger.kernel.org
> Fixes: 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
> Reported-by: <co+ac0a8c41de69121d@bugs.sh>
> Assisted-by: LLM
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>  kernel/bpf/offload.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 0d6f55695..8c91d00d4 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -698,11 +698,14 @@ static bool __bpf_offload_dev_match(struct bpf_prog *prog,
>  		return false;
>  	if (offload->netdev == netdev)
>  		return true;
> +	if (!bpf_prog_is_offloaded(prog->aux))
> +		return false;
>  
>  	ondev1 = bpf_offload_find_netdev(offload->netdev);
>  	ondev2 = bpf_offload_find_netdev(netdev);

[..]
 
> -	return ondev1 && ondev2 && ondev1->offdev == ondev2->offdev;
> +	return ondev1 && ondev2 && ondev1->offdev &&
> +	       ondev1->offdev == ondev2->offdev;
>  }
>  
>  bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev)

Why also have a null check here? Why doing !bpf_prog_is_offloaded above is not
enough?