netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net v2 00/18] net/sched: validate the control action with all the other parameters
@ 2019-03-20 13:59 Davide Caratti
  2019-03-20 13:59 ` [PATCH net v2 01/18] net/sched: prepare TC actions to properly validate the control action Davide Caratti
                   ` (20 more replies)
  0 siblings, 21 replies; 23+ messages in thread
From: Davide Caratti @ 2019-03-20 13:59 UTC (permalink / raw)
  To: Cong Wang, Vlad Buslov, Jamal Hadi Salim, Jiri Pirko,
	David S . Miller, netdev
  Cc: Paolo Abeni

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


^ permalink raw reply	[flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-03-22  3:04 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-20 13:59 [PATCH net v2 00/18] net/sched: validate the control action with all the other parameters Davide Caratti
2019-03-20 13:59 ` [PATCH net v2 01/18] net/sched: prepare TC actions to properly validate the control action Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 02/18] net/sched: act_bpf: validate the control action inside init() Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 03/18] net/sched: act_csum: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 04/18] net/sched: act_gact: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 05/18] net/sched: act_ife: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 06/18] net/sched: act_mirred: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 07/18] net/sched: act_connmark: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 08/18] net/sched: act_nat: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 09/18] net/sched: act_pedit: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 10/18] net/sched: act_police: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 11/18] net/sched: act_sample: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 12/18] net/sched: act_simple: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 13/18] net/sched: act_skbedit: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 14/18] net/sched: act_skbmod: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 15/18] net/sched: act_tunnel_key: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 16/18] net/sched: act_vlan: " Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 17/18] net/sched: don't dereference a->goto_chain to read the chain index Davide Caratti
2019-03-20 14:00 ` [PATCH net v2 18/18] net/sched: let actions use RCU to access 'goto_chain' Davide Caratti
2019-03-20 19:36 ` [PATCH net v2 00/18] net/sched: validate the control action with all the other parameters David Miller
2019-03-21 20:27 ` David Miller
2019-03-22  2:17 ` Cong Wang
2019-03-22  3:03   ` Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).