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

[Paul Chaignon <paul.chaignon@gmail.com>]

On Mon, Jul 21, 2025 at 05:08:05PM -0700, Eduard Zingerman wrote:
> On Mon, 2025-07-21 at 14:57 +0200, Paul Chaignon wrote:

Thanks for the review!

[...]

> > @@ -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().

Nice catch, thanks! It's fixed and tested in the v2.

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

Yes, this is not necessary; I got carried away. Per John's suggestion, I
still kept it in the v2 and converted data and data_end for consistency.
Hopefully, that'll be less confusing to future readers.

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