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]

Message-ID: <20230322223713.4e339b35@kernel.org>
Date:   Wed, 22 Mar 2023 22:37:13 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     Felix Fietkau <nbd@....name>, netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: ethernet: mtk_eth_soc: fix flow block
 refcounting

On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote:
> 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.  
> 
> Firstly, it's usually best to have one fix per patch.

Or at least reword the commit message to make it look like it's fixing
the refcounting logic?

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

That'd be my go to fix as well, FWIW.

Alternatively - the block cannot be removed until FLOW_BLOCK_UNBIND is
called, right? So the register_block bool can be skipped and
refcount taken after flow_block_cb_add() / list_add_tail().
That way at least the new block handling doesn't have to be split
into two chunks.