netdev - Re: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220125224524.fkodqvknsluihw74@kafai-mbp.dhcp.thefacebook.com>
Date:   Tue, 25 Jan 2022 14:45:24 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Jakub Sitnicki <jakub@...udflare.com>
CC:     <menglong8.dong@...il.com>, <ast@...nel.org>,
        <daniel@...earbox.net>, <andrii@...nel.org>,
        <songliubraving@...com>, <yhs@...com>, <john.fastabend@...il.com>,
        <kpsingh@...nel.org>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <mengensun@...cent.com>, <flyingpeng@...cent.com>,
        <mungerjiang@...cent.com>, Menglong Dong <imagedong@...cent.com>
Subject: Re: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct
 bpf_sock'

On Tue, Jan 25, 2022 at 08:24:27PM +0100, Jakub Sitnicki wrote:
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b0383d371b9a..891a182a749a 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5500,7 +5500,11 @@ struct bpf_sock {
> >  	__u32 src_ip4;
> >  	__u32 src_ip6[4];
> >  	__u32 src_port;		/* host byte order */
> > -	__u32 dst_port;		/* network byte order */
> > +	__u32 dst_port;		/* low 16-bits are in network byte order,
> > +				 * and high 16-bits are filled by 0.
> > +				 * So the real port in host byte order is
> > +				 * bpf_ntohs((__u16)dst_port).
> > +				 */
> >  	__u32 dst_ip4;
> >  	__u32 dst_ip6[4];
> >  	__u32 state;
> 
> I'm probably missing something obvious, but is there anything stopping
> us from splitting the field, so that dst_ports is 16-bit wide?
> 
> I gave a quick check to the change below and it seems to pass verifier
> checks and sock_field tests.
> 
> IDK, just an idea. Didn't give it a deeper thought.
> 
> --8<--
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 4a2f7041ebae..344d62ccafba 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5574,7 +5574,8 @@ struct bpf_sock {
>  	__u32 src_ip4;
>  	__u32 src_ip6[4];
>  	__u32 src_port;		/* host byte order */
> -	__u32 dst_port;		/* network byte order */
> +	__u16 unused;
> +	__u16 dst_port;		/* network byte order */
This will break the existing bpf prog.

>  	__u32 dst_ip4;
>  	__u32 dst_ip6[4];
>  	__u32 state;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index a06931c27eeb..c56b8ba82de5 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8276,7 +8276,6 @@ bool bpf_sock_is_valid_access(int off, int size, enum bpf_access_type type,
>  	case offsetof(struct bpf_sock, family):
>  	case offsetof(struct bpf_sock, type):
>  	case offsetof(struct bpf_sock, protocol):
> -	case offsetof(struct bpf_sock, dst_port):
>  	case offsetof(struct bpf_sock, src_port):
>  	case offsetof(struct bpf_sock, rx_queue_mapping):
>  	case bpf_ctx_range(struct bpf_sock, src_ip4):
> @@ -8285,6 +8284,9 @@ bool bpf_sock_is_valid_access(int off, int size, enum bpf_access_type type,
>  	case bpf_ctx_range_till(struct bpf_sock, dst_ip6[0], dst_ip6[3]):
>  		bpf_ctx_record_field_size(info, size_default);
>  		return bpf_ctx_narrow_access_ok(off, size, size_default);
> +	case offsetof(struct bpf_sock, dst_port):
> +		bpf_ctx_record_field_size(info, sizeof(__u16));
> +		return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
>  	}
> 
>  	return size == size_default;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 4a2f7041ebae..344d62ccafba 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5574,7 +5574,8 @@ struct bpf_sock {
>  	__u32 src_ip4;
>  	__u32 src_ip6[4];
>  	__u32 src_port;		/* host byte order */
> -	__u32 dst_port;		/* network byte order */
> +	__u16 unused;
> +	__u16 dst_port;		/* network byte order */
>  	__u32 dst_ip4;
>  	__u32 dst_ip6[4];
>  	__u32 state;