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+d50710fd0873a9c6b...@syzkaller.appspotmail.com
> Signed-off-by: Du Cheng <duche...@gmail.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;
}
