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

[Du Cheng <ducheng2@gmail.com>]

Le Thu, Apr 15, 2021 at 08:02:45PM +0200, Eric Dumazet a écrit :
> 
> 
> 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.
> 

Hi Eric,

Oh It must be I did not read your email carefully, and misundertood. I am sorry.
I sent a V3 following your suggestion. Please help review. Thank you!

Regards,
DU Cheng