Re: [PATCH v2] net: sched: tapr: remove WARN_ON() in taprio_get_start_time()

[Eric Dumazet <eric.dumazet@gmail.com>]

On 4/15/21 9:59 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@syzkaller.appspotmail.com
> Signed-off-by: Du Cheng <ducheng2@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
> 


NACK

I already gave feedback in v1 why this fix is not correct.