diff mbox series

[net] net/sched: sch_taprio: fix undefined behavior in ktime_mono_to_any

Message ID 20211108180815.1822479-1-eric.dumazet@gmail.com (mailing list archive)
State Accepted
Commit 6dc25401cba4d428328eade8ceae717633fdd702
Delegated to: Netdev Maintainers
Headers show
Series [net] net/sched: sch_taprio: fix undefined behavior in ktime_mono_to_any | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers warning 3 maintainers not CCed: xiyou.wangcong@gmail.com jiri@resnulli.us jhs@mojatatu.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch warning WARNING: Possible repeated word: 'Google'
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Nov. 8, 2021, 6:08 p.m. UTC
From: Eric Dumazet <edumazet@google.com>

1) if q->tk_offset == TK_OFFS_MAX, then get_tcp_tstamp() calls
   ktime_mono_to_any() with out-of-bound value.

2) if q->tk_offset is changed in taprio_parse_clockid(),
   taprio_get_time() might also call ktime_mono_to_any()
   with out-of-bound value as sysbot found:

UBSAN: array-index-out-of-bounds in kernel/time/timekeeping.c:908:27
index 3 is out of range for type 'ktime_t *[3]'
CPU: 1 PID: 25668 Comm: kworker/u4:0 Not tainted 5.15.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: bat_events batadv_iv_send_outstanding_bat_ogm_packet
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 ubsan_epilogue+0xb/0x5a lib/ubsan.c:151
 __ubsan_handle_out_of_bounds.cold+0x62/0x6c lib/ubsan.c:291
 ktime_mono_to_any+0x1d4/0x1e0 kernel/time/timekeeping.c:908
 get_tcp_tstamp net/sched/sch_taprio.c:322 [inline]
 get_packet_txtime net/sched/sch_taprio.c:353 [inline]
 taprio_enqueue_one+0x5b0/0x1460 net/sched/sch_taprio.c:420
 taprio_enqueue+0x3b1/0x730 net/sched/sch_taprio.c:485
 dev_qdisc_enqueue+0x40/0x300 net/core/dev.c:3785
 __dev_xmit_skb net/core/dev.c:3869 [inline]
 __dev_queue_xmit+0x1f6e/0x3630 net/core/dev.c:4194
 batadv_send_skb_packet+0x4a9/0x5f0 net/batman-adv/send.c:108
 batadv_iv_ogm_send_to_if net/batman-adv/bat_iv_ogm.c:393 [inline]
 batadv_iv_ogm_emit net/batman-adv/bat_iv_ogm.c:421 [inline]
 batadv_iv_send_outstanding_bat_ogm_packet+0x6d7/0x8e0 net/batman-adv/bat_iv_ogm.c:1701
 process_one_work+0x9b2/0x1690 kernel/workqueue.c:2298
 worker_thread+0x658/0x11f0 kernel/workqueue.c:2445
 kthread+0x405/0x4f0 kernel/kthread.c:327
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

Fixes: 7ede7b03484b ("taprio: make clock reference conversions easier")
Fixes: 54002066100b ("taprio: Adjust timestamps for TCP packets")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: Vedang Patel <vedang.patel@intel.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/sched/sch_taprio.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Vinicius Costa Gomes Nov. 8, 2021, 11:53 p.m. UTC | #1
Hi Eric,

Eric Dumazet <eric.dumazet@gmail.com> writes:

> From: Eric Dumazet <edumazet@google.com>
>
> 1) if q->tk_offset == TK_OFFS_MAX, then get_tcp_tstamp() calls
>    ktime_mono_to_any() with out-of-bound value.
>
> 2) if q->tk_offset is changed in taprio_parse_clockid(),
>    taprio_get_time() might also call ktime_mono_to_any()
>    with out-of-bound value as sysbot found:
>
> UBSAN: array-index-out-of-bounds in kernel/time/timekeeping.c:908:27
> index 3 is out of range for type 'ktime_t *[3]'
> CPU: 1 PID: 25668 Comm: kworker/u4:0 Not tainted 5.15.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: bat_events batadv_iv_send_outstanding_bat_ogm_packet
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>  ubsan_epilogue+0xb/0x5a lib/ubsan.c:151
>  __ubsan_handle_out_of_bounds.cold+0x62/0x6c lib/ubsan.c:291
>  ktime_mono_to_any+0x1d4/0x1e0 kernel/time/timekeeping.c:908
>  get_tcp_tstamp net/sched/sch_taprio.c:322 [inline]
>  get_packet_txtime net/sched/sch_taprio.c:353 [inline]
>  taprio_enqueue_one+0x5b0/0x1460 net/sched/sch_taprio.c:420
>  taprio_enqueue+0x3b1/0x730 net/sched/sch_taprio.c:485
>  dev_qdisc_enqueue+0x40/0x300 net/core/dev.c:3785
>  __dev_xmit_skb net/core/dev.c:3869 [inline]
>  __dev_queue_xmit+0x1f6e/0x3630 net/core/dev.c:4194
>  batadv_send_skb_packet+0x4a9/0x5f0 net/batman-adv/send.c:108
>  batadv_iv_ogm_send_to_if net/batman-adv/bat_iv_ogm.c:393 [inline]
>  batadv_iv_ogm_emit net/batman-adv/bat_iv_ogm.c:421 [inline]
>  batadv_iv_send_outstanding_bat_ogm_packet+0x6d7/0x8e0 net/batman-adv/bat_iv_ogm.c:1701
>  process_one_work+0x9b2/0x1690 kernel/workqueue.c:2298
>  worker_thread+0x658/0x11f0 kernel/workqueue.c:2445
>  kthread+0x405/0x4f0 kernel/kthread.c:327
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>
> Fixes: 7ede7b03484b ("taprio: make clock reference conversions easier")
> Fixes: 54002066100b ("taprio: Adjust timestamps for TCP packets")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Cc: Vedang Patel <vedang.patel@intel.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> ---

I just tested this and it indeed fixes a crash when the user tries to
configure the txtime-assisted mode using CLOCK_MONOTONIC.

Reviewed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
patchwork-bot+netdevbpf@kernel.org Nov. 10, 2021, 3:20 a.m. UTC | #2
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  8 Nov 2021 10:08:15 -0800 you wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> 1) if q->tk_offset == TK_OFFS_MAX, then get_tcp_tstamp() calls
>    ktime_mono_to_any() with out-of-bound value.
> 
> 2) if q->tk_offset is changed in taprio_parse_clockid(),
>    taprio_get_time() might also call ktime_mono_to_any()
>    with out-of-bound value as sysbot found:
> 
> [...]

Here is the summary with links:
  - [net] net/sched: sch_taprio: fix undefined behavior in ktime_mono_to_any
    https://git.kernel.org/netdev/net/c/6dc25401cba4

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 9ab068fa2672be8febd227b96cbcf646bf51f661..377f896bdedc4575316c3bab1d661d9c88142ec0 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -95,18 +95,22 @@  static ktime_t sched_base_time(const struct sched_gate_list *sched)
 	return ns_to_ktime(sched->base_time);
 }
 
-static ktime_t taprio_get_time(struct taprio_sched *q)
+static ktime_t taprio_mono_to_any(const struct taprio_sched *q, ktime_t mono)
 {
-	ktime_t mono = ktime_get();
+	/* This pairs with WRITE_ONCE() in taprio_parse_clockid() */
+	enum tk_offsets tk_offset = READ_ONCE(q->tk_offset);
 
-	switch (q->tk_offset) {
+	switch (tk_offset) {
 	case TK_OFFS_MAX:
 		return mono;
 	default:
-		return ktime_mono_to_any(mono, q->tk_offset);
+		return ktime_mono_to_any(mono, tk_offset);
 	}
+}
 
-	return KTIME_MAX;
+static ktime_t taprio_get_time(const struct taprio_sched *q)
+{
+	return taprio_mono_to_any(q, ktime_get());
 }
 
 static void taprio_free_sched_cb(struct rcu_head *head)
@@ -319,7 +323,7 @@  static ktime_t get_tcp_tstamp(struct taprio_sched *q, struct sk_buff *skb)
 		return 0;
 	}
 
-	return ktime_mono_to_any(skb->skb_mstamp_ns, q->tk_offset);
+	return taprio_mono_to_any(q, skb->skb_mstamp_ns);
 }
 
 /* There are a few scenarios where we will have to modify the txtime from
@@ -1352,6 +1356,7 @@  static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
 		}
 	} else if (tb[TCA_TAPRIO_ATTR_SCHED_CLOCKID]) {
 		int clockid = nla_get_s32(tb[TCA_TAPRIO_ATTR_SCHED_CLOCKID]);
+		enum tk_offsets tk_offset;
 
 		/* We only support static clockids and we don't allow
 		 * for it to be modified after the first init.
@@ -1366,22 +1371,24 @@  static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
 
 		switch (clockid) {
 		case CLOCK_REALTIME:
-			q->tk_offset = TK_OFFS_REAL;
+			tk_offset = TK_OFFS_REAL;
 			break;
 		case CLOCK_MONOTONIC:
-			q->tk_offset = TK_OFFS_MAX;
+			tk_offset = TK_OFFS_MAX;
 			break;
 		case CLOCK_BOOTTIME:
-			q->tk_offset = TK_OFFS_BOOT;
+			tk_offset = TK_OFFS_BOOT;
 			break;
 		case CLOCK_TAI:
-			q->tk_offset = TK_OFFS_TAI;
+			tk_offset = TK_OFFS_TAI;
 			break;
 		default:
 			NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
 			err = -EINVAL;
 			goto out;
 		}
+		/* This pairs with READ_ONCE() in taprio_mono_to_any */
+		WRITE_ONCE(q->tk_offset, tk_offset);
 
 		q->clockid = clockid;
 	} else {