netdev - Re: [patch net-next 27/34] nfp: bpf: Convert ndo_setup

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: <20171017073959.2b0553fd@cakuba.netronome.com>
Date:   Tue, 17 Oct 2017 07:39:59 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [patch net-next 27/34] nfp: bpf: Convert ndo_setup_tc offloads
 to block callbacks

On Tue, 17 Oct 2017 14:48:12 +0200, Jiri Pirko wrote:
> Fri, Oct 13, 2017 at 03:08:24AM CEST, jakub.kicinski@...ronome.com wrote:
> >On Thu, 12 Oct 2017 19:18:16 +0200, Jiri Pirko wrote:  
> >> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> index a88bb5b..9e9af88 100644
> >> --- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> +++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> @@ -246,6 +246,10 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
> >>  	void *code;
> >>  	int err;
> >>  
> >> +	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
> >> +	    cls_bpf->common.chain_index)
> >> +		return -EOPNOTSUPP;
> >> +
> >>  	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
> >>  
> >>  	switch (cls_bpf->command) {  
> >
> >It is certainly very ugly but I send a fake struct tc_cls_bpf_offload
> >here for XDP.  Refactoring this mess is pretty high on my priority list
> >but one way or the other this function will be called from XDP so TC
> >checks must stay in the TC handler... :(  
> 
> Okay. But currently, why is it a problem? You don't need the checks for
> xdp path.
> 

static int
nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
		    struct bpf_prog *prog)
{
	struct tc_cls_bpf_offload cmd = {
		.prog = prog,
	};
	int ret;

	if (!nfp_net_ebpf_capable(nn))
		return -EINVAL;

	if (nn->dp.ctrl & NFP_NET_CFG_CTRL_BPF) {
		if (!nn->dp.bpf_offload_xdp)
			return prog ? -EBUSY : 0;
		cmd.command = prog ? TC_CLSBPF_REPLACE : TC_CLSBPF_DESTROY;
	} else {
		if (!prog)
			return 0;
		cmd.command = TC_CLSBPF_ADD;
	}

	ret = nfp_net_bpf_offload(nn, &cmd);
	/* Stop offload if replace not possible */
	if (ret && cmd.command == TC_CLSBPF_REPLACE)
		nfp_bpf_xdp_offload(app, nn, NULL);
	nn->dp.bpf_offload_xdp = prog && !ret;
	return ret;
}

The fake offload struct is at the top of this function.  Dereferencing
cls_bpf->common in nfp_net_bpf_offload() will crash the kernel.  Or am
I missing something?