From patchwork Thu Jan 10 16:25:25 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Taehee Yoo X-Patchwork-Id: 1023022 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netfilter-devel-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Q7y+8gl7"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 43bBDt0gYvz9sN4 for ; Fri, 11 Jan 2019 03:25:34 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729209AbfAJQZd (ORCPT ); Thu, 10 Jan 2019 11:25:33 -0500 Received: from mail-pf1-f195.google.com ([209.85.210.195]:45830 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727771AbfAJQZd (ORCPT ); Thu, 10 Jan 2019 11:25:33 -0500 Received: by mail-pf1-f195.google.com with SMTP id g62so5511848pfd.12 for ; Thu, 10 Jan 2019 08:25:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=7+zHmS6Uv/lKobbECjQ0GEJWYlaZkIaTMnf/QSg1eto=; b=Q7y+8gl7bPfA0nKC6OavhJP1z0GXHSdyMF7hihLHRALJcTnoRtacfe2vXRLjvfgupp 8Svq2xTJiN2d4/HJqphFSC2c/kCtUIg7SVhNbixa+vD3N9huMLgKZds8JTRGyWwEov5G ZKJPxTHeaLqYKkhQx0ac9YLTDpb6zG25S5XYWKyPQDvSJvsr+dVIeMyetqBsMGD8nQE9 V4888R4fh2x2rBe/okXLzaxY+KgqrcdwFBH3X7hF95W7pOsDMal74aI4MeTaaIM2VCt5 X3RzmkBmk/ami8C5UelWcxxW2WjaZ6aqNDZUYStraZJ7BLC/W19kn8D8PsKBiXd/wp6B pQ5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=7+zHmS6Uv/lKobbECjQ0GEJWYlaZkIaTMnf/QSg1eto=; b=d2vQnSlskGpH+kFIU3Obyw2sgTvN1jsIDBNfK+ivM2AZVvsQeCydFaEApj2CRfGQQr qdqWiSNWEuGdYyyJmXLhmBKjWrVg58FCSk0rP0mrOrkrH/Xg8YZA8YKKXsfrKKm2wEov M5VTr0wneGKzfKEsbYyCz+FB/9zlt+hMMvo/JUff6ynzg8xB3UxN/43c4tzzaHbOi5kc p+fRvRCZh2pLD3t6vmjW+hviAYHQ/AGyPIjuYbRbVdVC35B3mR4eNzJ2AdQthn6Bgek7 IEeN9vnQTrYOhe7jQABu5ny6KWH7nddfWPvM2d/yueHeBjDXWirWF4TdcJIvx8ASe8a+ RmTA== X-Gm-Message-State: AJcUukfr6B1pnvQjqwk8GROxfBlgVzpuzzBW3klHY8i6g74ZgXzOeqTQ laikUv7GbMwSvY2eXneNbR4= X-Google-Smtp-Source: ALg8bN5DaACbEVdjPmX/u8IwFBvlZnB+19Tc3truic0eib+eWg2qjSkQ8ZB+chQFo62X3wNOQ/Vp4w== X-Received: by 2002:a65:50c1:: with SMTP id s1mr9715733pgp.350.1547137531575; Thu, 10 Jan 2019 08:25:31 -0800 (PST) Received: from ap-To-be-filled-by-O-E-M.8.8.8.8 ([14.33.120.60]) by smtp.gmail.com with ESMTPSA id d21sm57478764pfo.162.2019.01.10.08.25.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Jan 2019 08:25:30 -0800 (PST) From: Taehee Yoo To: pablo@netfilter.org, netfilter-devel@vger.kernel.org Cc: ap420073@gmail.com Subject: [PATCH nf 2/2] netfilter: nft_compat: protect lists between select_ops and init Date: Fri, 11 Jan 2019 01:25:25 +0900 Message-Id: <20190110162525.1117-1-ap420073@gmail.com> X-Mailer: git-send-email 2.17.1 Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org nft_{match/target}_select_ops() find xt modules in the list. if the module is found, it is just used without any holding module or increasing reference counts. and the reference count is increased in nft_{match/target}_init() function. But, xt modules can be removed anytime if reference count is 0. CPU0 CPU1 nft_{match/target}_select_ops nft_{match/target}_destroy nft_{match/target}_init The scenario is like this: 1. ->select_ops() found a module in the list and keeps an element of it without increasing reference count. 2. ->destroy() frees an element of list. 3. nft_{match/target}_init try to use the pointer of element, but this is already freed. Test commands: while : do iptables-nft -t nat -I PREROUTING -m string --string ap --algo \ kmp -j MASQUERADE & nft flush ruleset & done Result: [ 682.340431] BUG: KASAN: use-after-free in nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.340431] Read of size 4 at addr ffff8881079a73a0 by task iptables-nft/8136 [ 682.340431] [ 682.340431] CPU: 1 PID: 8136 Comm: iptables-nft Not tainted 4.20.0+ #59 [ 682.373297] Call Trace: [ 682.373297] dump_stack+0xc9/0x16b [ 682.373297] ? show_regs_print_info+0x5/0x5 [ 682.373297] ? kmsg_dump_rewind_nolock+0xd9/0xd9 [ 682.402587] ? nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.402587] print_address_description+0x6a/0x270 [ 682.402587] ? nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.402587] ? nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.402587] kasan_report+0x12a/0x16f [ 682.429087] ? nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.433653] nft_target_init+0xb96/0xbe0 [nft_compat] [ 682.433653] ? target_compat_from_user.isra.5+0x80/0x80 [nft_compat] [ 682.433653] ? nf_tables_newrule+0xe46/0x2570 [nf_tables] [ 682.433653] ? ksm_do_scan+0x2bc0/0x3300 [ 682.433653] ? hlock_class+0x140/0x140 [ 682.433653] ? nf_tables_newrule+0xe46/0x2570 [nf_tables] [ 682.465582] ? memcpy+0x39/0x50 [ 682.465582] ? __kmalloc+0x134/0x2e0 [ 682.465582] ? nf_tables_newrule+0xe46/0x2570 [nf_tables] [ 682.465582] nf_tables_newrule+0x10a4/0x2570 [nf_tables] [ ... ] [ 682.828711] [ 682.828711] Allocated by task 8134: [ 682.828711] kasan_kmalloc+0xa5/0xd0 [ 682.828711] kmem_cache_alloc_trace+0x117/0x290 [ 682.828711] nft_target_select_ops+0x481/0x8e0 [nft_compat] [ 682.828711] nf_tables_expr_parse+0x2a1/0x510 [nf_tables] [ 682.828711] nf_tables_newrule+0xd0c/0x2570 [nf_tables] [ 682.828711] nfnetlink_rcv_batch+0xd2e/0x1550 [nfnetlink] [ 682.828711] nfnetlink_rcv+0x2ee/0x350 [nfnetlink] [ 682.828711] netlink_unicast+0x414/0x5e0 [ 682.828711] netlink_sendmsg+0x7b8/0xcf0 [ 682.937632] ___sys_sendmsg+0x689/0x990 [ 682.937632] __sys_sendmsg+0xd2/0x210 [ 682.937632] do_syscall_64+0x138/0x560 [ 682.937632] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 682.937632] [ 682.937632] Freed by task 8140: [ 682.937632] __kasan_slab_free+0x135/0x180 [ 682.937632] kfree+0xdb/0x280 [ 682.937632] rcu_process_callbacks+0x947/0x24d0 [ 682.937632] __do_softirq+0x2a5/0xa3b [ 682.937632] Fixes: 0935d5588400 ("netfilter: nf_tables: asynchronous release") Signed-off-by: Taehee Yoo --- net/netfilter/nft_compat.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c index 83adabdec900..8f3114871d20 100644 --- a/net/netfilter/nft_compat.c +++ b/net/netfilter/nft_compat.c @@ -29,6 +29,7 @@ struct nft_xt { struct list_head head; struct nft_expr_ops ops; unsigned int refcnt; + bool use; /* Unlike other expressions, ops doesn't have static storage duration. * nft core assumes they do. We use kfree_rcu so that nft core can @@ -48,7 +49,7 @@ struct nft_xt_match_priv { static bool nft_xt_put(struct nft_xt *xt) { mutex_lock(&nft_xt_lock); - if (--xt->refcnt == 0) { + if (--xt->refcnt == 0 && !xt->use) { list_del(&xt->head); kfree_rcu(xt, rcu_head); mutex_unlock(&nft_xt_lock); @@ -280,6 +281,7 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, mutex_lock(&nft_xt_lock); nft_xt = container_of(expr->ops, struct nft_xt, ops); nft_xt->refcnt++; + nft_xt->use = false; mutex_unlock(&nft_xt_lock); return 0; } @@ -495,6 +497,7 @@ __nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, mutex_lock(&nft_xt_lock); nft_xt = container_of(expr->ops, struct nft_xt, ops); nft_xt->refcnt++; + nft_xt->use = false; mutex_unlock(&nft_xt_lock); return 0; } @@ -780,6 +783,7 @@ nft_match_select_ops(const struct nft_ctx *ctx, struct xt_match *match = nft_match->ops.data; if (nft_match_cmp(match, mt_name, rev, family)) { + nft_match->use = true; mutex_unlock(&nft_xt_lock); return &nft_match->ops; } @@ -803,6 +807,7 @@ nft_match_select_ops(const struct nft_ctx *ctx, } nft_match->refcnt = 0; + nft_match->use = true; nft_match->ops.type = &nft_match_type; nft_match->ops.eval = nft_match_eval; nft_match->ops.init = nft_match_init; @@ -885,6 +890,7 @@ nft_target_select_ops(const struct nft_ctx *ctx, continue; if (nft_target_cmp(target, tg_name, rev, family)) { + nft_target->use = true; mutex_unlock(&nft_xt_lock); return &nft_target->ops; } @@ -913,6 +919,7 @@ nft_target_select_ops(const struct nft_ctx *ctx, } nft_target->refcnt = 0; + nft_target->use = true; nft_target->ops.type = &nft_target_type; nft_target->ops.size = NFT_EXPR_SIZE(XT_ALIGN(target->targetsize)); nft_target->ops.init = nft_target_init;