From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CF5F7C43381 for ; Wed, 20 Mar 2019 14:00:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9D09D2175B for ; Wed, 20 Mar 2019 14:00:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727882AbfCTOAr (ORCPT ); Wed, 20 Mar 2019 10:00:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54486 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726506AbfCTOAr (ORCPT ); Wed, 20 Mar 2019 10:00:47 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 284C63132C25; Wed, 20 Mar 2019 14:00:46 +0000 (UTC) Received: from new-host.redhat.com (ovpn-204-166.brq.redhat.com [10.40.204.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id 763CC19C58; Wed, 20 Mar 2019 14:00:40 +0000 (UTC) From: Davide Caratti To: Cong Wang , Vlad Buslov , Jamal Hadi Salim , Jiri Pirko , "David S . Miller" , netdev Cc: Paolo Abeni Subject: [PATCH net v2 00/18] net/sched: validate the control action with all the other parameters Date: Wed, 20 Mar 2019 14:59:58 +0100 Message-Id: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Wed, 20 Mar 2019 14:00:46 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org currently, the kernel checks for bad values of the control action in tcf_action_init_1(), after a successful call to the action's init() function. When the control action is 'goto chain', this causes two undesired behaviors: 1. "misconfigured action after replace that causes kernel crash": if users replace a valid TC action with another one having invalid control action, all the new configuration data (including the bad control action) are applied successfully, even if the kernel returned an error. As a consequence, it's possible to trigger a NULL pointer dereference in the traffic path of every TC action (1), replacing the control action with 'goto chain x', when chain doesn't exist. 2. "refcount leak that makes kmemleak complain" when a valid 'goto chain' action is overwritten with another action, the kernel forgets to decrease refcounts in the chain. The above problems can be fixed if we validate the control action in each action's init() function, the same way as we are already doing for all the other configuration parameters. Now that chains can be released after an action is replaced, we need to care about concurrent access of 'goto_chain' pointer: ensure we access it through RCU, like we did with most action-specific configuration parameters. - Patch 1 removes the wrong checks and provides functions that can be used to properly validate control actions in individual actions - Patch 2 to 16 fix individual actions, and add TDC selftest code to verify the correct behavior (2) - Patch 17 and 18 fix concurrent access issues on 'goto_chain', that can be observed after the chain refcount leak is fixed. Changes since v1: - reword the cover letter - condense the extack message in case tc_action_check_ctrlact() is called with invalid parameters. - add tcf_action_set_ctrlact() to avoid code duplication an make the RCU-ification of 'goto_chain' easier. - fix errors in act_ife, act_simple, act_skbedit, and avoid useless 'goto end' in act_connmark, thanks a lot to Vlad Buslov. - avoid dereferencing 'goto_chain' in tcf_gact_goto_chain_index(), so we don't have to care about the grace period there. - let actions respect the grace period when they release chains, thanks to Cong Wang and Vlad Buslov. Changes since RFC: - include a fix for all TC actions - add a selftest for each TC action - squash fix for refcount leaks into a single patch, the first in the series, thanks to Cong Wang - ensure that chain refcount is released without tcfa_lock held, thanks to Vlad Buslov Notes: (1) act_ipt didn't need any fix, as the control action is constantly equal to TC_ACT_OK. (2) the selftest for act_simple fails because userspace tc backend for 'simple' does not parse the control action correctly (and hardcodes it to TC_ACT_PIPE). Davide Caratti (18): net/sched: prepare TC actions to properly validate the control action net/sched: act_bpf: validate the control action inside init() net/sched: act_csum: validate the control action inside init() net/sched: act_gact: validate the control action inside init() net/sched: act_ife: validate the control action inside init() net/sched: act_mirred: validate the control action inside init() net/sched: act_connmark: validate the control action inside init() net/sched: act_nat: validate the control action inside init() net/sched: act_pedit: validate the control action inside init() net/sched: act_police: validate the control action inside init() net/sched: act_sample: validate the control action inside init() net/sched: act_simple: validate the control action inside init() net/sched: act_skbedit: validate the control action inside init() net/sched: act_skbmod: validate the control action inside init() net/sched: act_tunnel_key: validate the control action inside init() net/sched: act_vlan: validate the control action inside init() net/sched: don't dereference a->goto_chain to read the chain index net/sched: let actions use RCU to access 'goto_chain' include/net/act_api.h | 9 +- include/net/sch_generic.h | 1 + include/net/tc_act/tc_gact.h | 2 +- net/sched/act_api.c | 101 ++++++++++-------- net/sched/act_bpf.c | 25 +++-- net/sched/act_connmark.c | 22 +++- net/sched/act_csum.c | 22 +++- net/sched/act_gact.c | 15 ++- net/sched/act_ife.c | 35 +++--- net/sched/act_ipt.c | 11 +- net/sched/act_mirred.c | 22 +++- net/sched/act_nat.c | 15 ++- net/sched/act_pedit.c | 18 +++- net/sched/act_police.c | 13 ++- net/sched/act_sample.c | 21 +++- net/sched/act_simple.c | 54 +++++++--- net/sched/act_skbedit.c | 20 +++- net/sched/act_skbmod.c | 20 +++- net/sched/act_tunnel_key.c | 19 +++- net/sched/act_vlan.c | 22 +++- net/sched/cls_api.c | 2 +- .../tc-testing/tc-tests/actions/bpf.json | 25 +++++ .../tc-testing/tc-tests/actions/connmark.json | 25 +++++ .../tc-testing/tc-tests/actions/csum.json | 25 +++++ .../tc-testing/tc-tests/actions/gact.json | 25 +++++ .../tc-testing/tc-tests/actions/ife.json | 25 +++++ .../tc-testing/tc-tests/actions/mirred.json | 25 +++++ .../tc-testing/tc-tests/actions/nat.json | 25 +++++ .../tc-testing/tc-tests/actions/pedit.json | 51 +++++++++ .../tc-testing/tc-tests/actions/police.json | 25 +++++ .../tc-testing/tc-tests/actions/sample.json | 25 +++++ .../tc-testing/tc-tests/actions/simple.json | 25 +++++ .../tc-testing/tc-tests/actions/skbedit.json | 25 +++++ .../tc-testing/tc-tests/actions/skbmod.json | 25 +++++ .../tc-tests/actions/tunnel_key.json | 25 +++++ .../tc-testing/tc-tests/actions/vlan.json | 25 +++++ 36 files changed, 749 insertions(+), 121 deletions(-) create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json -- 2.20.1