netdev - Re: [PATCH bpf-next v5 03/11] bpf: Add write access to tcp

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]

Date:   Tue, 9 Jan 2018 23:41:31 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>
CC:     Kernel Team <Kernel-team@...com>, Blake Matheny <bmatheny@...com>,
        "Alexei Starovoitov" <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Neal Cardwell" <ncardwell@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH bpf-next v5 03/11] bpf: Add write access to tcp_sock and
 sock fields


On 1/9/18, 3:22 PM, "netdev-owner@...r.kernel.org on behalf of Eric Dumazet" <netdev-owner@...r.kernel.org on behalf of eric.dumazet@...il.com> wrote:

    On Tue, 2018-01-09 at 13:06 -0800, Lawrence Brakmo wrote:
    > This patch adds a macro, SOCK_OPS_SET_FIELD, for writing to
    > struct tcp_sock or struct sock fields. This required adding a new
    > field "temp" to struct bpf_sock_ops_kern for temporary storage that
    > is used by sock_ops_convert_ctx_access. It is used to store and recover
    > the contents of a register, so the register can be used to store the
    > address of the sk. Since we cannot overwrite the dst_reg because it
    > contains the pointer to ctx, nor the src_reg since it contains the value
    > we want to store, we need an extra register to contain the address
    > of the sk.
    > 
    > Also adds the macro SOCK_OPS_GET_OR_SET_FIELD that calls one of the
    > GET or SET macros depending on the value of the TYPE field.
    > 
    > Signed-off-by: Lawrence Brakmo <brakmo@...com>
    > ---
    >  include/linux/filter.h |  3 +++
    >  include/net/tcp.h      |  2 +-
    >  net/core/filter.c      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
    >  3 files changed, 52 insertions(+), 1 deletion(-)
    > 
    > diff --git a/include/linux/filter.h b/include/linux/filter.h
    > index 425056c..139a9a3 100644
    > --- a/include/linux/filter.h
    > +++ b/include/linux/filter.h
    > @@ -1007,6 +1007,9 @@ struct bpf_sock_ops_kern {
    >  		u32 replylong[4];
    >  	};
    >  	u32	is_fullsock;
    > +	u64	temp;			/* Used by sock_ops_convert_ctx_access
    > +					 * as temporary storaage of a register
    
    
    typo on storage
    
Thanks, will fix.
	
    > +					 */
    >  };
    >  
    >  #endif /* __LINUX_FILTER_H__ */
    > diff --git a/include/net/tcp.h b/include/net/tcp.h
    > index 6939e69..108d16a 100644
    > --- a/include/net/tcp.h
    > +++ b/include/net/tcp.h
    > @@ -2010,7 +2010,7 @@ static inline int tcp_call_bpf(struct sock *sk, int op)
    >  	struct bpf_sock_ops_kern sock_ops;
    >  	int ret;
    >  
    > -	memset(&sock_ops, 0, sizeof(sock_ops));
    > +	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
    > 
    
    This lacks proper comment, stating that every field added after .temp
    in struct bpf_sock_ops_kern is considered as scratch and could contain
    garbage.
    
I assumed that one would modify this initialization appropriately when adding
a new field. But it would certainly make sense to indicate that new, non-scratch,
fields should be added before temp so there would be no need to modify this
initialization code. Makes sense?