netdev - Re: [PATCH net 1/2] net: ethernet: mtk_eth

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:   Wed, 22 Mar 2023 15:33:52 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Felix Fietkau <nbd@....name>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block
 refcounting

On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote:
> Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call
> flow_block_cb_incref unconditionally, even for a newly allocated cb.
> Fixes a use-after-free bug. Also fix the accidentally inverted refcount
> check on unbind.

Hi Felix,

my 2c worth.

Firstly, it's usually best to have one fix per patch.

Second, this seems to conflicts with:

  [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for
        offloading flows from wlan devices

So I guess that series will need to be rebased on this one at some point.

> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Signed-off-by: Felix Fietkau <nbd@....name>
> ---
>  .../net/ethernet/mediatek/mtk_ppe_offload.c   | 23 +++++++++++--------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 81afd5ee3fbf..43cc510b51bd 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	struct mtk_eth *eth = mac->hw;
>  	static LIST_HEAD(block_cb_list);
>  	struct flow_block_cb *block_cb;
> +	bool register_block = false;
>  	flow_setup_cb_t *cb;
>  
>  	if (!eth->soc->offload_version)
> @@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
>  	switch (f->command) {
>  	case FLOW_BLOCK_BIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
> -		if (block_cb) {
> -			flow_block_cb_incref(block_cb);
> -			return 0;
> +		if (!block_cb) {
> +			block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> +			if (IS_ERR(block_cb))
> +				return PTR_ERR(block_cb);
> +
> +			register_block = true;
>  		}
> -		block_cb = flow_block_cb_alloc(cb, dev, dev, NULL);
> -		if (IS_ERR(block_cb))
> -			return PTR_ERR(block_cb);
>  
> -		flow_block_cb_add(block_cb, f);
> -		list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		flow_block_cb_incref(block_cb);
> +
> +		if (register_block) {
> +			flow_block_cb_add(block_cb, f);
> +			list_add_tail(&block_cb->driver_list, &block_cb_list);
> +		}
>  		return 0;

I think the existing code was more idiomatic, and could
be maintained by simply adding a call to flow_block_cb_incref()
before the call to flow_block_cb_add().

@@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f)
 		if (IS_ERR(block_cb))
 			return PTR_ERR(block_cb);
 
+		flow_block_cb_incref(block_cb);
 		flow_block_cb_add(block_cb, f);
 		list_add_tail(&block_cb->driver_list, &block_cb_list);
 		return 0;

>  	case FLOW_BLOCK_UNBIND:
>  		block_cb = flow_block_cb_lookup(f->block, cb, dev);
>  		if (!block_cb)
>  			return -ENOENT;
>  
> -		if (flow_block_cb_decref(block_cb)) {
> +		if (!flow_block_cb_decref(block_cb)) {
>  			flow_block_cb_remove(block_cb, f);
>  			list_del(&block_cb->driver_list);
>  		}
> -- 
> 2.39.0
>