netdev - Re: [PATCH bpf-next 1/3] bpf: sockmap, metadata support for reporting size of msg

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: <20181220024949.xrgns7nelsxglkhp@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 19 Dec 2018 18:49:52 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     daniel@...earbox.net, ast@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 1/3] bpf: sockmap, metadata support for
 reporting size of msg

On Sun, Dec 16, 2018 at 03:47:04PM -0800, John Fastabend wrote:
> This adds metadata to sk_msg_md for BPF programs to read the sk_msg
> size.
> 
> When the SK_MSG program is running under an application that is using
> sendfile the data is not copied into sk_msg buffers by default. Rather
> the BPF program uses sk_msg_pull_data to read the bytes in. This
> avoids doing the costly memcopy instructions when they are not in
> fact needed. However, if we don't know the size of the sk_msg we
> have to guess if needed bytes are available by doing a pull request
> which may fail. By including the size of the sk_msg BPF programs can
> check the size before issuing sk_msg_pull_data requests.
> 
> Additionally, the same applies for sendmsg calls when the application
> provides multiple iovs. Here the BPF program needs to pull in data
> to update data pointers but its not clear where the data ends without
> a size parameter. In many cases "guessing" is not easy to do
> and results in multiple calls to pull and without bounded loops
> everything gets fairly tricky.
> 
> Clean this up by including a u32 size field. Note, all writes into
> sk_msg_md are rejected already from sk_msg_is_valid_access so nothing
> additional is needed there.
> 
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>  include/linux/skmsg.h    | 3 +++
>  include/uapi/linux/bpf.h | 1 +
>  net/core/filter.c        | 6 ++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 2a11e9d..eb8f6cb 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -36,6 +36,9 @@ struct sk_msg_sg {
>  	struct scatterlist		data[MAX_MSG_FRAGS + 1];
>  };
>  
> +/* UAPI in filter.c depends on struct sk_msg_sg being first element. If
> + * this is moved filter.c also must be updated.
> + */
>  struct sk_msg {
>  	struct sk_msg_sg		sg;
>  	void				*data;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 597afdb..498badc 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2608,6 +2608,7 @@ struct sk_msg_md {
>  	__u32 local_ip6[4];	/* Stored in network byte order */
>  	__u32 remote_port;	/* Stored in network byte order */
>  	__u32 local_port;	/* stored in host byte order */
> +	__u32 size;		/* Total size of sk_msg */
>  };
>  
>  struct sk_reuseport_md {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index bd0df75..55fd237 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7513,6 +7513,12 @@ static u32 sk_msg_convert_ctx_access(enum bpf_access_type type,
>  		*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->dst_reg,
>  				      offsetof(struct sock_common, skc_num));
>  		break;
> +
> +	case offsetof(struct sk_msg_md, size):
> +		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_msg_sg, size),
> +				      si->dst_reg, si->src_reg,
> +				      offsetof(struct sk_msg_sg, size));
> +		break;

John,

this change broke "test_verifier 129" test.
Now it's failing.
But upon further examination both sk_msg_is_valid_access()
and that test were incorrect.
Here is the bug:
        if (off < 0 || off >= sizeof(struct sk_msg_md))

sizeof() includes padding to 8 bytes.
So out of bounds access passes sk_msg_is_valid_access(),
but rewritten incorrectly by sk_msg_convert_ctx_access()
and the test is testing for wrong thing.
errstr = "R0 !read_ok" is not the message it should be looking for.

After this patch and following adjustment to test_verifier.c
that test is now failing while verifier doing the right thing.
Please submit two patches to fix this:
1 - fix sk_msg_convert_ctx_access
2 - fix test_verifier