Re: [PATCH bpf-next 1/2] bpf: Reject narrower access to pointer ctx fields

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2025-07-21 at 14:57 +0200, Paul Chaignon wrote:

[...]

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 72c8b50dca0a..3a4ad9f124e1 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -2577,17 +2577,17 @@ static bool cg_sockopt_is_valid_access(int off, int size,
>  	}
>  
>  	switch (off) {
> -	case offsetof(struct bpf_sockopt, sk):
> +	case bpf_ctx_range_ptr(struct bpf_sockopt, sk):
>  		if (size != sizeof(__u64))
>  			return false;
>  		info->reg_type = PTR_TO_SOCKET;
>  		break;
> -	case offsetof(struct bpf_sockopt, optval):
> +	case bpf_ctx_range_ptr(struct bpf_sockopt, optval):
>  		if (size != sizeof(__u64))
>  			return false;
>  		info->reg_type = PTR_TO_PACKET;
>  		break;
> -	case offsetof(struct bpf_sockopt, optval_end):
> +	case bpf_ctx_range_ptr(struct bpf_sockopt, optval_end):
>  		if (size != sizeof(__u64))
>  			return false;
>  		info->reg_type = PTR_TO_PACKET_END;

Nit: I'd also convert `case offsetof(struct bpf_sockopt, retval):`
     just below.  Otherwise reader would spend some time figuring out
     why `retval` is special (it's not).

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7a72f766aacf..458908c5f1f4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8690,7 +8690,7 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type
>  		if (size != sizeof(__u64))
>  			return false;
>  		break;
> -	case offsetof(struct __sk_buff, sk):
> +	case bpf_ctx_range_ptr(struct __sk_buff, sk):
>  		if (type == BPF_WRITE || size != sizeof(__u64))
>  			return false;
>  		info->reg_type = PTR_TO_SOCK_COMMON_OR_NULL;
> @@ -9268,7 +9268,7 @@ static bool sock_addr_is_valid_access(int off, int size,
>  				return false;
>  		}
>  		break;
> -	case offsetof(struct bpf_sock_addr, sk):
> +	case bpf_ctx_range_ptr(struct bpf_sock_addr, sk):
>  		if (type != BPF_READ)
>  			return false;
>  		if (size != sizeof(__u64))
> @@ -9318,17 +9318,17 @@ static bool sock_ops_is_valid_access(int off, int size,
>  			if (size != sizeof(__u64))
>  				return false;
>  			break;
> -		case offsetof(struct bpf_sock_ops, sk):
> +		case bpf_ctx_range_ptr(struct bpf_sock_ops, sk):
>  			if (size != sizeof(__u64))
>  				return false;
>  			info->reg_type = PTR_TO_SOCKET_OR_NULL;
>  			break;
> -		case offsetof(struct bpf_sock_ops, skb_data):
> +		case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data):
>  			if (size != sizeof(__u64))
>  				return false;
>  			info->reg_type = PTR_TO_PACKET;
>  			break;
> -		case offsetof(struct bpf_sock_ops, skb_data_end):
> +		case bpf_ctx_range_ptr(struct bpf_sock_ops, skb_data_end):
>  			if (size != sizeof(__u64))
>  				return false;
>  			info->reg_type = PTR_TO_PACKET_END;

I think this function is buggy for `skb_hwtstamp` as well.
The skb_hwtstamp field is u64, side_default is sizeof(u32).
So access at `offsetof(struct bpf_sock_ops, skb_hwtstamp) + 4` would
be permitted by the default branch. But this range is not handled by
accompanying sock_ops_convert_ctx_access().


> @@ -9417,7 +9417,7 @@ static bool sk_msg_is_valid_access(int off, int size,
>  		if (size != sizeof(__u64))
>  			return false;
>  		break;
> -	case offsetof(struct sk_msg_md, sk):
> +	case bpf_ctx_range_ptr(struct sk_msg_md, sk):
>  		if (size != sizeof(__u64))
>  			return false;
>  		info->reg_type = PTR_TO_SOCKET;

I don't think this change is necessary, the default branch rejects
access at any not matched offset. Otherwise `data` and `data_end`
should be converted for uniformity.

> @@ -11623,7 +11623,7 @@ static bool sk_lookup_is_valid_access(int off, int size,
>  		return false;
>  
>  	switch (off) {
> -	case offsetof(struct bpf_sk_lookup, sk):
> +	case bpf_ctx_range_ptr(struct bpf_sk_lookup, sk):
>  		info->reg_type = PTR_TO_SOCKET_OR_NULL;
>  		return size == sizeof(__u64);
>  

Same here, the default branch would reject access at the wrong offset already.