| Message ID | 20180429145711.13091-1-ap420073@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | fix module leak and use-after-free | expand |
Taehee Yoo <ap420073@gmail.com> wrote: > The nft_expr_ops might be freed in the nf_tables_expr_destroy but > after this, a member of nft_expr_ops is used. > > Steps to reproduce: > $iptables-compat -I OUTPUT -m cpu --cpu 0 > $iptables-compat -F Oh, same reproducer as 2nd patch? I NORMAL case (non-compat) ->ops is 'static const', so no free occurs. So I thjink it might be better to fix nft_compat to not release the ops structure, but keep it around until rmmod nft_compat. AFAICS we can achive this by using a refcount of two instead of one, and retain the list until rmmod. What do you think? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 981f35e..2ab23e3 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -1873,7 +1873,7 @@ static int nf_tables_newexpr(const struct nft_ctx *ctx, if (ops->init) { err = ops->init(ctx, expr, (const struct nlattr **)info->tb); if (err < 0) - goto err1; + return err; } if (ops->validate) { @@ -1881,16 +1881,14 @@ static int nf_tables_newexpr(const struct nft_ctx *ctx, err = ops->validate(ctx, expr, &data); if (err < 0) - goto err2; + goto err; } return 0; -err2: +err: if (ops->destroy) ops->destroy(ctx, expr); -err1: - expr->ops = NULL; return err; } @@ -2233,16 +2231,13 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk, static void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule) { - struct nft_expr *expr; + struct nft_expr *expr, *next; - /* - * Careful: some expressions might not be initialized in case this - * is called on error from nf_tables_newrule(). - */ expr = nft_expr_first(rule); - while (expr != nft_expr_last(rule) && expr->ops) { + while (expr != nft_expr_last(rule)) { + next = nft_expr_next(expr); nf_tables_expr_destroy(ctx, expr); - expr = nft_expr_next(expr); + expr = next; } kfree(rule); }
The nft_expr_ops might be freed in the nf_tables_expr_destroy but after this, a member of nft_expr_ops is used. Steps to reproduce: $iptables-compat -I OUTPUT -m cpu --cpu 0 $iptables-compat -F Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- net/netfilter/nf_tables_api.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)