Skip to content

Commit

Permalink
net/sched: act_gate: fix NULL dereference in tcf_gate_init()
Browse files Browse the repository at this point in the history 
it is possible to see a KASAN use-after-free, immediately followed by a
NULL dereference crash, with the following command:

 # tc action add action gate index 3 cycle-time 100000000ns \
 > cycle-time-ext 100000000ns clockid CLOCK_TAI

 BUG: KASAN: use-after-free in tcf_action_init_1+0x8eb/0x960
 Write of size 1 at addr ffff88810a5908bc by task tc/883

 CPU: 0 PID: 883 Comm: tc Not tainted 5.7.0+ torvalds#188
 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
 Call Trace:
  dump_stack+0x75/0xa0
  print_address_description.constprop.6+0x1a/0x220
  kasan_report.cold.9+0x37/0x7c
  tcf_action_init_1+0x8eb/0x960
  tcf_action_init+0x157/0x2a0
  tcf_action_add+0xd9/0x2f0
  tc_ctl_action+0x2a3/0x39d
  rtnetlink_rcv_msg+0x5f3/0x920
  netlink_rcv_skb+0x120/0x380
  netlink_unicast+0x439/0x630
  netlink_sendmsg+0x714/0xbf0
  sock_sendmsg+0xe2/0x110
  ____sys_sendmsg+0x5b4/0x890
  ___sys_sendmsg+0xe9/0x160
  __sys_sendmsg+0xd3/0x170
  do_syscall_64+0x9a/0x370
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

[...]

 KASAN: null-ptr-deref in range [0x0000000000000070-0x0000000000000077]
 CPU: 0 PID: 883 Comm: tc Tainted: G    B             5.7.0+ torvalds#188
 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
 RIP: 0010:tcf_action_fill_size+0xa3/0xf0
 [....]
 RSP: 0018:ffff88813a48f250 EFLAGS: 00010212
 RAX: dffffc0000000000 RBX: 0000000000000094 RCX: ffffffffa47c3eb6
 RDX: 000000000000000e RSI: 0000000000000008 RDI: 0000000000000070
 RBP: ffff88810a590800 R08: 0000000000000004 R09: ffffed1027491e03
 R10: 0000000000000003 R11: ffffed1027491e03 R12: 0000000000000000
 R13: 0000000000000000 R14: dffffc0000000000 R15: ffff88810a590800
 FS:  00007f62cae8ce40(0000) GS:ffff888147c00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f62c9d20a10 CR3: 000000013a52a000 CR4: 0000000000340ef0
 Call Trace:
  tcf_action_init+0x172/0x2a0
  tcf_action_add+0xd9/0x2f0
  tc_ctl_action+0x2a3/0x39d
  rtnetlink_rcv_msg+0x5f3/0x920
  netlink_rcv_skb+0x120/0x380
  netlink_unicast+0x439/0x630
  netlink_sendmsg+0x714/0xbf0
  sock_sendmsg+0xe2/0x110
  ____sys_sendmsg+0x5b4/0x890
  ___sys_sendmsg+0xe9/0x160
  __sys_sendmsg+0xd3/0x170
  do_syscall_64+0x9a/0x370
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

this is caused by the test on 'cycletime_ext', that is still unassigned
when the action is newly created. This makes the action .init() return 0
without calling tcf_idr_insert(), hence the UAF + crash.

rework the logic that prevents zero values of cycle-time, as follows:

1) 'tcfg_cycletime_ext' seems to be unused in the action software path,
   and it was already possible by other means to obtain non-zero
   cycletime and zero cycletime-ext. So, removing that test should not
   cause any damage.
2) while at it, we must prevent overwriting configuration data with wrong
   ones: use a temporary variable for 'tcfg_cycletime', and validate it
   preserving the original semantic (that allowed computing the cycle
   time as the sum of all intervals, when not specified by
   TCA_GATE_CYCLE_TIME).
3) remove the test on 'tcfg_cycletime', no more useful, and avoid
   returning -EFAULT, which did not seem an appropriate return value for
   a wrong netlink attribute.

v2: remove useless 'return;' at the end of void gate_get_start_time()

Fixes: a51c328 ("net: qos: introduce a gate control flow action")
CC: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
  • Loading branch information
@dcaratti @intel-lab-lkp
dcaratti authored and intel-lab-lkp committed Jun 15, 2020
1 parent 96144c5 commit 2d28964
Showing 1 changed file with 13 additions and 23 deletions.
36 changes: 13 additions & 23 deletions net/sched/act_gate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ static ktime_t gate_get_time(struct tcf_gate *gact)
return KTIME_MAX;
}

static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
static void gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
{
struct tcf_gate_params *param = &gact->param;
ktime_t now, base, cycle;
Expand All @@ -43,18 +43,13 @@ static int gate_get_start_time(struct tcf_gate *gact, ktime_t *start)

if (ktime_after(base, now)) {
*start = base;
return 0;
return;
}

cycle = param->tcfg_cycletime;

/* cycle time should not be zero */
if (!cycle)
return -EFAULT;

n = div64_u64(ktime_sub_ns(now, base), cycle);
*start = ktime_add_ns(base, (n + 1) * cycle);
return 0;
}

static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
Expand Down Expand Up @@ -287,12 +282,12 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
enum tk_offsets tk_offset = TK_OFFS_TAI;
struct nlattr *tb[TCA_GATE_MAX + 1];
struct tcf_chain *goto_ch = NULL;
u64 cycletime, basetime = 0;
struct tcf_gate_params *p;
s32 clockid = CLOCK_TAI;
struct tcf_gate *gact;
struct tc_gate *parm;
int ret = 0, err;
u64 basetime = 0;
u32 gflags = 0;
s32 prio = -1;
ktime_t start;
Expand Down Expand Up @@ -375,26 +370,28 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&gact->tcf_lock);
p = &gact->param;

if (tb[TCA_GATE_CYCLE_TIME]) {
p->tcfg_cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
if (!p->tcfg_cycletime_ext)
goto chain_put;
}
if (tb[TCA_GATE_CYCLE_TIME])
cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);

if (tb[TCA_GATE_ENTRY_LIST]) {
err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
if (err < 0)
goto chain_put;
}

if (!p->tcfg_cycletime) {
if (!cycletime) {
struct tcfg_gate_entry *entry;
ktime_t cycle = 0;

list_for_each_entry(entry, &p->entries, list)
cycle = ktime_add_ns(cycle, entry->interval);
p->tcfg_cycletime = cycle;
cycletime = cycle;
if (!cycletime) {
err = -EINVAL;
goto chain_put;
}
}
p->tcfg_cycletime = cycletime;

if (tb[TCA_GATE_CYCLE_TIME_EXT])
p->tcfg_cycletime_ext =
Expand All @@ -408,14 +405,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
gact->tk_offset = tk_offset;
hrtimer_init(&gact->hitimer, clockid, HRTIMER_MODE_ABS_SOFT);
gact->hitimer.function = gate_timer_func;

err = gate_get_start_time(gact, &start);
if (err < 0) {
NL_SET_ERR_MSG(extack,
"Internal error: failed get start time");
release_entry_list(&p->entries);
goto chain_put;
}
gate_get_start_time(gact, &start);

gact->current_close_time = start;
gact->current_gate_status = GATE_ACT_GATE_OPEN | GATE_ACT_PENDING;
Expand Down

0 comments on commit 2d28964

Please sign in to comment.