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] [day] [month] [year] [list]

Message-ID: <fea90700-7bd2-8bc6-48cf-d60fbec7e3c8@gmail.com>
Date:   Wed, 19 Dec 2018 22:30:05 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...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 12/19/18 6:49 PM, Alexei Starovoitov wrote:
> 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.

Will need to see why its not failing for me. But thanks for
catching.

> 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))
>

thanks. Will use bpf_ctx_range() check instead.


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

typo :(

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

Creating fix now thanks.