Fw: [Bug 60669] Kernel panic when using negative priority for HTB class

Fw: [Bug 60669] Kernel panic when using negative priority for HTB class

[Stephen Hemminger]

Begin forwarded message:

Date: Thu, 1 Aug 2013 02:38:53 -0700
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 60669] Kernel panic when using negative priority for HTB class


https://bugzilla.kernel.org/show_bug.cgi?id=60669

_Vi <vi0oss@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Kernel panic when using HTB |Kernel panic when using
                   |qdisc                       |negative priority for HTB
                   |                            |class

--- Comment #2 from _Vi <vi0oss@gmail.com> ---
Reproducible on 3.11.0-rc3

Looks like negative priority value is essential for the kernel panic.

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: Fw: [Bug 60669] Kernel panic when using negative priority for HTB class

[Cong Wang]

On Fri, 02 Aug 2013 at 04:12 GMT, Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> --- Comment #2 from _Vi <vi0oss@gmail.com> ---
> Reproducible on 3.11.0-rc3
>
> Looks like negative priority value is essential for the kernel panic.
>

There is a check for prio:

                if ((cl->prio = hopt->prio) >= TC_HTB_NUMPRIO)
		                        cl->prio = TC_HTB_NUMPRIO - 1;
					

Re: [Bug 60669] Kernel panic when using negative priority for HTB class

[Stephen Hemminger]

On Fri, 2 Aug 2013 04:50:16 +0000 (UTC)
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> On Fri, 02 Aug 2013 at 04:12 GMT, Stephen Hemminger <stephen@networkplumber.org> wrote:
> >
> > --- Comment #2 from _Vi <vi0oss@gmail.com> ---
> > Reproducible on 3.11.0-rc3
> >
> > Looks like negative priority value is essential for the kernel panic.
> >
> 
> There is a check for prio:
> 
>                 if ((cl->prio = hopt->prio) >= TC_HTB_NUMPRIO)
> 		                        cl->prio = TC_HTB_NUMPRIO - 1;
> 					

This maybe related to or old bugs, 64 bit with current
iproute2 the error is caught at the command level.

The issue is that hopt->prio is u32 but cl->prio is int.

Example:
int main()
{
	int x = -1;
	uint32_t prio = x;
	int cl;

	if ((cl = prio) >= TC_HTB_NUMPRIO)
		cl = TC_HTB_NUMPRIO - 1;

	printf("%d %u %d\n", x, prio, cl);
	return 0;
}
-1 4294967295 -1

Signed/unsigned conversions can bite.

[PATCH net-next] htb: fix sign extension bug

[Stephen Hemminger]

When userspace passes a large priority value 
the assignment of the unsigned value hopt->prio
to  signed int cl->prio causes cl->prio to become negative and the
comparison is with TC_HTB_NUMPRIO is always false.

The result is that HTB crashes by referencing outside
the array when processing packets. With this patch the large value
wraps around like other values outside the normal range.

See: https://bugzilla.kernel.org/show_bug.cgi?id=60669

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

--- a/net/sched/sch_htb.c	2013-06-20 09:22:46.489542435 -0700
+++ b/net/sched/sch_htb.c	2013-08-01 22:12:43.736307055 -0700
@@ -100,7 +100,7 @@ struct htb_class {
 	struct psched_ratecfg	ceil;
 	s64			buffer, cbuffer;/* token bucket depth/rate */
 	s64			mbuffer;	/* max wait time */
-	int			prio;		/* these two are used only by leaves... */
+	u32			prio;		/* these two are used only by leaves... */
 	int			quantum;	/* but stored for parent-to-leaf return */
 
 	struct tcf_proto	*filter_list;	/* class attached filters */

Re: [PATCH net-next] htb: fix sign extension bug

[Eric Dumazet]

On Thu, 2013-08-01 at 22:32 -0700, Stephen Hemminger wrote:
> When userspace passes a large priority value 
> the assignment of the unsigned value hopt->prio
> to  signed int cl->prio causes cl->prio to become negative and the
> comparison is with TC_HTB_NUMPRIO is always false.
> 
> The result is that HTB crashes by referencing outside
> the array when processing packets. With this patch the large value
> wraps around like other values outside the normal range.
> 
> See: https://bugzilla.kernel.org/show_bug.cgi?id=60669
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> --- a/net/sched/sch_htb.c	2013-06-20 09:22:46.489542435 -0700
> +++ b/net/sched/sch_htb.c	2013-08-01 22:12:43.736307055 -0700
> @@ -100,7 +100,7 @@ struct htb_class {
>  	struct psched_ratecfg	ceil;
>  	s64			buffer, cbuffer;/* token bucket depth/rate */
>  	s64			mbuffer;	/* max wait time */
> -	int			prio;		/* these two are used only by leaves... */
> +	u32			prio;		/* these two are used only by leaves... */
>  	int			quantum;	/* but stored for parent-to-leaf return */
>  
>  	struct tcf_proto	*filter_list;	/* class attached filters */
> --

Thanks Stephen.

Acked-by: Eric Dumazet <edumazet@google.com>

It seems appropriate for net tree (and stable)

We probably should report an error from htb_change_class() ?

Re: [PATCH net-next] htb: fix sign extension bug

[Stephen Hemminger]

On Fri, 02 Aug 2013 06:36:20 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Thu, 2013-08-01 at 22:32 -0700, Stephen Hemminger wrote:
> > When userspace passes a large priority value 
> > the assignment of the unsigned value hopt->prio
> > to  signed int cl->prio causes cl->prio to become negative and the
> > comparison is with TC_HTB_NUMPRIO is always false.
> > 
> > The result is that HTB crashes by referencing outside
> > the array when processing packets. With this patch the large value
> > wraps around like other values outside the normal range.
> > 
> > See: https://bugzilla.kernel.org/show_bug.cgi?id=60669
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > --- a/net/sched/sch_htb.c	2013-06-20 09:22:46.489542435 -0700
> > +++ b/net/sched/sch_htb.c	2013-08-01 22:12:43.736307055 -0700
> > @@ -100,7 +100,7 @@ struct htb_class {
> >  	struct psched_ratecfg	ceil;
> >  	s64			buffer, cbuffer;/* token bucket depth/rate */
> >  	s64			mbuffer;	/* max wait time */
> > -	int			prio;		/* these two are used only by leaves... */
> > +	u32			prio;		/* these two are used only by leaves... */
> >  	int			quantum;	/* but stored for parent-to-leaf return */
> >  
> >  	struct tcf_proto	*filter_list;	/* class attached filters */
> > --
> 
> Thanks Stephen.
> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> 
> It seems appropriate for net tree (and stable)
> 
> We probably should report an error from htb_change_class() ?

There are more signed/unsigned bugs lurking here, and opportunities to
shrink the structure. (for example prio could be u8). Overall this code
uses int where unsigned should be used.

Examples:
static inline void htb_add_class_to_row(struct htb_sched *q,
					struct htb_class *cl, int mask)
{
	q->row_mask[cl->level] |= mask;
	while (mask) {
		int prio = ffz(~mask);
		mask &= ~(1 << prio);
		htb_add_to_id_tree(&q->hlevel[cl->level].hprio[prio].row, cl, prio);
	}
}

Could be:
static void htb_add_class_to_row(struct htb_sched *q,
			  	 struct htb_class *cl, u8 mask)
{
	q->row_mask[cl->level] |= mask;
	while (mask) {
		u8 prio = ffz(~mask);
		mask &= ~(1 << prio);
		htb_add_to_id_tree(&q->hlevel[cl->level].hprio[prio].row, cl, prio);
	}
}

Looks like all use of int in HTB should be re-evaluated to see if a smaller unsigned
type could be used instead.

Re: [PATCH net-next] htb: fix sign extension bug

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri, 2 Aug 2013 08:24:04 -0700

> There are more signed/unsigned bugs lurking here, and opportunities to
> shrink the structure. (for example prio could be u8). Overall this code
> uses int where unsigned should be used.

If any of them can cause an OOPS like this one could, let's get that
fixed as soon as possible.

Thanks.

Re: [PATCH net-next] htb: fix sign extension bug

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 02 Aug 2013 06:36:20 -0700

> On Thu, 2013-08-01 at 22:32 -0700, Stephen Hemminger wrote:
>> When userspace passes a large priority value 
>> the assignment of the unsigned value hopt->prio
>> to  signed int cl->prio causes cl->prio to become negative and the
>> comparison is with TC_HTB_NUMPRIO is always false.
>> 
>> The result is that HTB crashes by referencing outside
>> the array when processing packets. With this patch the large value
>> wraps around like other values outside the normal range.
>> 
>> See: https://bugzilla.kernel.org/show_bug.cgi?id=60669
>> 
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> 
>> --- a/net/sched/sch_htb.c	2013-06-20 09:22:46.489542435 -0700
>> +++ b/net/sched/sch_htb.c	2013-08-01 22:12:43.736307055 -0700
>> @@ -100,7 +100,7 @@ struct htb_class {
>>  	struct psched_ratecfg	ceil;
>>  	s64			buffer, cbuffer;/* token bucket depth/rate */
>>  	s64			mbuffer;	/* max wait time */
>> -	int			prio;		/* these two are used only by leaves... */
>> +	u32			prio;		/* these two are used only by leaves... */
>>  	int			quantum;	/* but stored for parent-to-leaf return */
>>  
>>  	struct tcf_proto	*filter_list;	/* class attached filters */
>> --
> 
> Thanks Stephen.
> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> 
> It seems appropriate for net tree (and stable)

Applied and queued up for -stable, thanks.