netdev - Re: [PATCH] net: sched: tapr: remove WARN_ON() in taprio_get_start

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f9f5164-0247-8930-5400-90b7762247b1@gmail.com>
Date:   Thu, 15 Apr 2021 08:56:09 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Du Cheng <ducheng2@...il.com>, Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        syzbot+d50710fd0873a9c6b40c@...kaller.appspotmail.com
Subject: Re: [PATCH] net: sched: tapr: remove WARN_ON() in
 taprio_get_start_time()



On 4/15/21 8:39 AM, Du Cheng wrote:
> There is a reproducible sequence from the userland that will trigger a WARN_ON()
> condition in taprio_get_start_time, which causes kernel to panic if configured
> as "panic_on_warn". Remove this WARN_ON() to prevent kernel from crashing by
> userland-initiated syscalls.
> 
> Reported as bug on syzkaller:
> https://syzkaller.appspot.com/bug?extid=d50710fd0873a9c6b40c
> 
> Reported-by: syzbot+d50710fd0873a9c6b40c@...kaller.appspotmail.com
> Signed-off-by: Du Cheng <ducheng2@...il.com>
> ---
> Detailed explanation:
> 
> In net/sched/sched_taprio.c:999
> The condition WARN_ON(!cycle) will be triggered if cycle == 0. Value of cycle
> comes from sched->cycle_time, where sched is of type(struct sched_gate_list*).
> 
> sched->cycle_time is accumulated within `parse_taprio_schedule()` during
> `taprio_init()`, in the following 2 ways:
> 
> 1. from nla_get_s64(tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME]);
> 2. (if zero) from parse_sched_list(..., tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST], ...);
> 
> note: tb is a map parsed from netlink attributes provided via sendmsg() from the userland:
> 
> If both two attributes (TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME,
> TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST) contain 0 values or are missing, this will result
> in sched->cycle_time == 0 and hence trigger the WARN_ON(!cycle).
> 
> Reliable reproducable steps:
> 1. add net device team0 
> 2. add team_slave_0, team_slave_1
> 3. sendmsg(struct msghdr {
> 	.iov = struct nlmsghdr {
> 		.type = RTM_NEWQDISC,
> 	}
> 	struct tcmsg {
> 		.tcm_ifindex = ioctl(SIOCGIFINDEX, "team0"),
> 		.nlattr[] = {
> 			TCA_KIND: "taprio",
> 			TCA_OPTIONS: {
> 				.nlattr = {
> 					TCA_TAPRIO_ATTR_PRIOMAP: ...,
> 					TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST: {0},
> 					TCA_TAPRIO_ATTR_SCHED_CLICKID: 0,
> 				}
> 			}
> 		}
> 	}
> }
> 
> Callstack:
> 
> parse_taprio_schedule()
> taprio_change()
> taprio_init()
> qdisc_create()
> tc_modify_qdisc()
> rtnetlink_rcv_msg()
> ...
> sendmsg()
> 
> These steps are extracted from syzkaller reproducer:
> https://syzkaller.appspot.com/text?tag=ReproC&x=15727cf1900000
> 
>  net/sched/sch_taprio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 8287894541e3..5f2ff0f15d5c 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -996,7 +996,7 @@ static int taprio_get_start_time(struct Qdisc *sch,
>  	 * something went really wrong. In that case, we should warn about this
>  	 * inconsistent state and return error.
>  	 */
> -	if (WARN_ON(!cycle))
> +	if (!cycle) {
>  		return -EFAULT;
>  
>  	/* Schedule the start time for the beginning of the next
> 

This 'fix' is wrong, not even compiled, thus not tested.

 sched->cycle_time MUST not be zero ever, there are plenty
of other places where other crashes will happen.

You are silencing a condition that should be caught much earlier.

There is even a fat comment to explain this, that can be partially seen in your patch.


Correct fix will probably be :

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 8287894541e3ce5f290be2e592c0dcbdf2ec6b60..189c617a582a2eecc92e35187379d4c2889289df 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -901,6 +901,8 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
 
                list_for_each_entry(entry, &new->entries, list)
                        cycle = ktime_add_ns(cycle, entry->interval);
+               if (!cycle)
+                       return -EINVAL;
                new->cycle_time = cycle;
        }