| 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
| ||
|
Message-Id: <cover.1553085663.git.dcaratti@redhat.com>
Date: Wed, 20 Mar 2019 14:59:58 +0100
From: Davide Caratti <dcaratti@...hat.com>
To: Cong Wang <xiyou.wangcong@...il.com>,
Vlad Buslov <vladbu@...lanox.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Jiri Pirko <jiri@...nulli.us>,
"David S . Miller" <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>
Cc: Paolo Abeni <pabeni@...hat.com>
Subject: [PATCH net v2 00/18] net/sched: validate the control action with all the other parameters
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 <x> 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
Powered by blists - more mailing lists