Re: [PATCH 3/3] datatype: fix double-free resulting in use-after-free in datatype_free

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, May 01, 2020 at 09:59:35PM +0200, michael-dev wrote:
> Am 01.05.2020 21:27, schrieb Pablo Neira Ayuso:
> > On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote:
> > > +	if (dtype == expr->dtype)
> > > +		return; // do not free dtype before incrementing refcnt again
> > 
> > The problem you describe (use-after-free) happens in this case, right?
> 
> The problem is more likely due to concat_expr_parse_udata not calling
> datatype_get,
> because otherwise datatype_get would be in the backtrace of ASAN.
> 
> > 
> >         datatype_set(expr, expr->dtype);
> > 
> > Or am I missing anything?
> 
> But while debugging the above output, I added assert(dtype != expr->dtype)
> here
> and that crashed. So I'm sure something like this is happening.

Right.

# nft add rule ip x y ct state new,established,related,untracked
# nft list ruleset
nft: datatype.c:1086: datatype_set: Assertion `expr->dtype != dtype' failed.
Aborted

> And the whole thing was nasty to debug, so I added this one just be sure it
> does not happen again.
> 
> As ASAN should hit on datatype_get incrementing refcnt if datatype_free had
> actually freed it,
> assert was probaby not seeing an DTYPE_F_ALLOC instance.
> But I dig not deeper here, as I felt this return is safe to add.

I'm going to apply this. I think it's safe to turn this into noop.