[PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
initialized.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: John Fastabend <john.r.fastabend@intel.com>
---
 net/sched/sch_mqprio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index fbc6f53..effd4ee 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -215,7 +215,7 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct net_device *dev = qdisc_dev(sch);
 	struct mqprio_sched *priv = qdisc_priv(sch);
 	unsigned char *b = skb_tail_pointer(skb);
-	struct tc_mqprio_qopt opt;
+	struct tc_mqprio_qopt opt = { 0 };
 	struct Qdisc *qdisc;
 	unsigned int i;
 

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Joe Perches]

On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
> mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
> initialized.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: John Fastabend <john.r.fastabend@intel.com>
> ---
>  net/sched/sch_mqprio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index fbc6f53..effd4ee 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -215,7 +215,7 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct mqprio_sched *priv = qdisc_priv(sch);
>  	unsigned char *b = skb_tail_pointer(skb);
> -	struct tc_mqprio_qopt opt;
> +	struct tc_mqprio_qopt opt = { 0 };

I think the best style to use memset so that any
possible struct padding is guaranteed to be zeroed.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

Le mercredi 26 janvier 2011 à 09:43 -0800, Joe Perches a écrit :

> I think the best style to use memset so that any
> possible struct padding is guaranteed to be zeroed.
> 
> 

We use the { 0 } style in net/sched, and there is no padding in this
structure, I checked this point.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Joe Perches]

On Wed, 2011-01-26 at 18:49 +0100, Eric Dumazet wrote:
> Le mercredi 26 janvier 2011 à 09:43 -0800, Joe Perches a écrit :
> > I think the best style to use memset so that any
> > possible struct padding is guaranteed to be zeroed.
> We use the { 0 } style in net/sched,

That's nice, but it's the wrong style.
https://lkml.org/lkml/2010/12/15/63

> and there is no padding in this
> structure, I checked this point.

That may be true right now for this particular
structure, but that style is not future-proof.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 26 Jan 2011 09:43:43 -0800

> On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
>> mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
>> initialized.
>> 
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>  net/sched/sch_mqprio.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>> index fbc6f53..effd4ee 100644
>> --- a/net/sched/sch_mqprio.c
>> +++ b/net/sched/sch_mqprio.c
>> @@ -215,7 +215,7 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
>>  	struct net_device *dev = qdisc_dev(sch);
>>  	struct mqprio_sched *priv = qdisc_priv(sch);
>>  	unsigned char *b = skb_tail_pointer(skb);
>> -	struct tc_mqprio_qopt opt;
>> +	struct tc_mqprio_qopt opt = { 0 };
> 
> I think the best style to use memset so that any
> possible struct padding is guaranteed to be zeroed.

Such padding does not exist, and we won't add such padding since this is
a user visible data structure and thus whose layout is cast in stone.

Anyways, I'm ambivalent to how this is fixed actually.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 26 Jan 2011 09:56:18 -0800

> On Wed, 2011-01-26 at 18:49 +0100, Eric Dumazet wrote:
>> Le mercredi 26 janvier 2011 à 09:43 -0800, Joe Perches a écrit :
>> > I think the best style to use memset so that any
>> > possible struct padding is guaranteed to be zeroed.
>> We use the { 0 } style in net/sched,
> 
> That's nice, but it's the wrong style.
> https://lkml.org/lkml/2010/12/15/63
> 
>> and there is no padding in this
>> structure, I checked this point.
> 
> That may be true right now for this particular
> structure, but that style is not future-proof.

It is future-proof if the data-structure is user-visible and therefore
will never change, as is the case here.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

Le mercredi 26 janvier 2011 à 11:55 -0800, David Miller a écrit :
> From: Joe Perches <joe@perches.com>
> > 
> > I think the best style to use memset so that any
> > possible struct padding is guaranteed to be zeroed.
> 
> Such padding does not exist, and we won't add such padding since this is
> a user visible data structure and thus whose layout is cast in stone.
> 
> Anyways, I'm ambivalent to how this is fixed actually.

I am perfectly aware of this ugly memset() style some people prefer, and
5 % of the time they swap 2nd and 3rd param.

Two patches instead of one ;)

In this particular case, I only used existing codestyle: In the same
file I noticed :

vi +322 net/sched/sch_mqprio.c

static int mqprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
                               struct gnet_dump *d)
{
        struct net_device *dev = qdisc_dev(sch);

        if (cl <= netdev_get_num_tc(dev)) {
                int i;
                struct Qdisc *qdisc;
                struct gnet_stats_queue qstats = {0};
                struct gnet_stats_basic_packed bstats = {0};
                struct netdev_tc_txq tc = dev->tc_to_txq[cl - 1];

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

Le mercredi 26 janvier 2011 à 21:23 +0100, Eric Dumazet a écrit :

> I am perfectly aware of this ugly memset() style some people prefer, and
> 5 % of the time they swap 2nd and 3rd param.
> 
> Two patches instead of one ;)
> 

One random example of memset() troubles : 

commit fe10ae53384e48c51996941b7720ee16995cbcb7
fixed by commit 5b919f833d9d60588d026ad82d17f17e8872c7a9

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Jan 2011 18:21:57 +0100

> mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
> initialized.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: John Fastabend <john.r.fastabend@intel.com>

Applied, thanks Eric.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Joe Perches]

On Wed, 2011-01-26 at 21:23 +0100, Eric Dumazet wrote:
> Le mercredi 26 janvier 2011 à 11:55 -0800, David Miller a écrit :
> > From: Joe Perches <joe@perches.com>
> > > I think the best style to use memset so that any
> > > possible struct padding is guaranteed to be zeroed.
> > Such padding does not exist, and we won't add such padding since this is
> > a user visible data structure and thus whose layout is cast in stone.
> > Anyways, I'm ambivalent to how this is fixed actually.
> I am perfectly aware of this ugly memset() style some people prefer, and
> 5 % of the time they swap 2nd and 3rd param.

Ugly maybe, but correct, definitely.
The same can not be said of the {0}.

This use ends up the same so it doesn't
matter here, it's just a style question.

> Two patches instead of one ;)

How uninteresting.  Any API can be misused.

> In this particular case, I only used existing codestyle: In the same
> file I noticed :

There are memset's in the file as well.
In fact, memset is used in the same function.

cheers, Joe

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

Le mercredi 26 janvier 2011 à 13:24 -0800, Joe Perches a écrit :

> Ugly maybe, but correct, definitely.
> The same can not be said of the {0}.

What about fixing real problems Joe ?

Are you telling me I dont know C ?

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Joe Perches]

On Wed, 2011-01-26 at 22:28 +0100, Eric Dumazet wrote:
> Le mercredi 26 janvier 2011 à 13:24 -0800, Joe Perches a écrit :
> > Ugly maybe, but correct, definitely.
> > The same can not be said of the {0}.
> What about fixing real problems Joe ?

What about it?  You seem to have fixed it.

> Are you telling me I dont know C ?

All I'm saying is that from a style and auditing
perspective, it's better to use memset for all
structs that are exposed to user space.

Other than that, there's no issue here.

cheers, Joe

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Joe Perches]

On Wed, 2011-01-26 at 11:55 -0800, David Miller wrote: 
> From: Joe Perches <joe@perches.com>
> Date: Wed, 26 Jan 2011 09:43:43 -0800
> > On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
> >> mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
> >> initialized.
> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> CC: John Fastabend <john.r.fastabend@intel.com>
> >> ---
> >>  net/sched/sch_mqprio.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> >> index fbc6f53..effd4ee 100644
> >> --- a/net/sched/sch_mqprio.c
> >> +++ b/net/sched/sch_mqprio.c
> >> @@ -215,7 +215,7 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
> >>  	struct net_device *dev = qdisc_dev(sch);
> >>  	struct mqprio_sched *priv = qdisc_priv(sch);
> >>  	unsigned char *b = skb_tail_pointer(skb);
> >> -	struct tc_mqprio_qopt opt;
> >> +	struct tc_mqprio_qopt opt = { 0 };
> > I think the best style to use memset so that any
> > possible struct padding is guaranteed to be zeroed.
> Such padding does not exist, and we won't add such padding since this is
> a user visible data structure and thus whose layout is cast in stone.

/* MQPRIO */
#define TC_QOPT_BITMASK 15
#define TC_QOPT_MAX_QUEUE 16

struct tc_mqprio_qopt {
__u8 num_tc;
__u8 prio_tc_map[TC_QOPT_BITMASK + 1];
__u8 hw;
__u16 count[TC_QOPT_MAX_QUEUE];
__u16 offset[TC_QOPT_MAX_QUEUE];
};

I believe this struct needs to be declared __packed.

It could otherwise be 24 bytes not 22.
Or if char array declarations have a different
alignment requirement, could be any size.

memset is better than {0}.

cheers, Joe

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Changli Gao]

On Thu, Jan 27, 2011 at 2:04 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2011-01-26 at 11:55 -0800, David Miller wrote:
>> From: Joe Perches <joe@perches.com>
>> Date: Wed, 26 Jan 2011 09:43:43 -0800
>> > On Wed, 2011-01-26 at 18:21 +0100, Eric Dumazet wrote:
>> >> mqprio_dump() should make sure all fields of struct tc_mqprio_qopt are
>> >> initialized.
>> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> >> CC: John Fastabend <john.r.fastabend@intel.com>
>> >> ---
>> >>  net/sched/sch_mqprio.c |    2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>> >> index fbc6f53..effd4ee 100644
>> >> --- a/net/sched/sch_mqprio.c
>> >> +++ b/net/sched/sch_mqprio.c
>> >> @@ -215,7 +215,7 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
>> >>    struct net_device *dev = qdisc_dev(sch);
>> >>    struct mqprio_sched *priv = qdisc_priv(sch);
>> >>    unsigned char *b = skb_tail_pointer(skb);
>> >> -  struct tc_mqprio_qopt opt;
>> >> +  struct tc_mqprio_qopt opt = { 0 };
>> > I think the best style to use memset so that any
>> > possible struct padding is guaranteed to be zeroed.
>> Such padding does not exist, and we won't add such padding since this is
>> a user visible data structure and thus whose layout is cast in stone.
>
> /* MQPRIO */
> #define TC_QOPT_BITMASK 15
> #define TC_QOPT_MAX_QUEUE 16
>
> struct tc_mqprio_qopt {
> __u8 num_tc; // 1
> __u8 prio_tc_map[TC_QOPT_BITMASK + 1]; // 16
> __u8 hw; // 1
> __u16 count[TC_QOPT_MAX_QUEUE]; // 32
> __u16 offset[TC_QOPT_MAX_QUEUE]; //32
> };
>
> I believe this struct needs to be declared __packed.
>
> It could otherwise be 24 bytes not 22.
> Or if char array declarations have a different
> alignment requirement, could be any size.
>

The total size is 1 + 16 + 1 + 32 + 32 = 82.

How do you get 24 or 22?

> memset is better than {0}.
>


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Eric Dumazet]

Le mercredi 26 janvier 2011 à 22:04 -0800, Joe Perches a écrit :

> /* MQPRIO */
> #define TC_QOPT_BITMASK 15
> #define TC_QOPT_MAX_QUEUE 16
> 
> struct tc_mqprio_qopt {
> __u8 num_tc;
> __u8 prio_tc_map[TC_QOPT_BITMASK + 1];
> __u8 hw;
> __u16 count[TC_QOPT_MAX_QUEUE];
> __u16 offset[TC_QOPT_MAX_QUEUE];
> };
> 
> I believe this struct needs to be declared __packed.
> 

Oh my god. Yet another ugly thing.

> It could otherwise be 24 bytes not 22.

22 ? You are kidding probably. Its 82 exactly.

Listen, I doubled check my patch, its good, while your rants are lazy.

> Or if char array declarations have a different
> alignment requirement, could be any size.
> 

If if if... could could could...


> memset is better than {0}.
> 

You never stop do you ?

The bigger object is u16, therefore alignof() is 2, not 4

No ABI requires a short (u16) is aligned on 4 byte boundary.

If you find a compiler not respecting this, you can bet linux wont run
at all if compiled with it. mqprio 'potential 2 bytes leak' will hardly
be a problem.

Re: [PATCH net-next-2.6] net_sched: sch_mqprio: dont leak kernel memory

[Pádraig Brady]

On 26/01/11 17:56, Joe Perches wrote:
> On Wed, 2011-01-26 at 18:49 +0100, Eric Dumazet wrote:
>> Le mercredi 26 janvier 2011 à 09:43 -0800, Joe Perches a écrit :
>>> I think the best style to use memset so that any
>>> possible struct padding is guaranteed to be zeroed.
>> We use the { 0 } style in net/sched,
> 
> That's nice, but it's the wrong style.
> https://lkml.org/lkml/2010/12/15/63
> 
>> and there is no padding in this
>> structure, I checked this point.
> 
> That may be true right now for this particular
> structure, but that style is not future-proof.

I think {0,} is a valid init style, because the handling
of the initialization is special and overrides the point
mentioned in the link above.

cheers,
Pádraig.