[PATCH] Fix slab corruption with netem

[PATCH] Fix slab corruption with netem

[Guillaume Chazarain]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

Hello,

CONFIG_DEBUG_SLAB found the following bug:
netem_enqueue() in sch_netem.c gets a pointer inside a slab object: 
struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
But then, the slab object may be freed: skb = skb_unshare(skb, GFP_ATOMIC)
cb is still pointing inside the freed skb, so here is a patch to 
initialize cb later, and make it clear
that initializing it sooner is a bad idea.

Thanks.

-- 
Guillaume


[-- Attachment #2: netem_cb_initialization.diff --]
[-- Type: text/x-patch, Size: 845 bytes --]

--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -148,7 +148,8 @@ static int netem_enqueue(struct sk_buff 
 static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
-	struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
+	/* We don't fill cb now as skb_unshare() may invalidate it */
+	struct netem_skb_cb *cb = NULL;
 	struct sk_buff *skb2;
 	int ret;
 	int count = 1;
@@ -200,6 +201,7 @@ static int netem_enqueue(struct sk_buff 
 		skb->data[net_random() % skb_headlen(skb)] ^= 1<<(net_random() % 8);
 	}
 
+	cb = (struct netem_skb_cb *)skb->cb;
 	if (q->gap == 0 		/* not doing reordering */
 	    || q->counter < q->gap 	/* inside last reordering gap */
 	    || q->reorder < get_crandom(&q->reorder_cor)) {

-- 
Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>

Re: [PATCH] Fix slab corruption with netem

[Stephen Hemminger]

Guillaume Chazarain wrote:
> Hello,
>
> CONFIG_DEBUG_SLAB found the following bug:
> netem_enqueue() in sch_netem.c gets a pointer inside a slab object: 
> struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
> But then, the slab object may be freed: skb = skb_unshare(skb, 
> GFP_ATOMIC)
> cb is still pointing inside the freed skb, so here is a patch to 
> initialize cb later, and make it clear
> that initializing it sooner is a bad idea.
>
> Thanks.
>
> ------------------------------------------------------------------------
>
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -148,7 +148,8 @@ static int netem_enqueue(struct sk_buff 
>  static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  {
>  	struct netem_sched_data *q = qdisc_priv(sch);
> -	struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
> +	/* We don't fill cb now as skb_unshare() may invalidate it */
> +	struct netem_skb_cb *cb = NULL;
>   
Would rather leave it unitialized, rather than setting to NULL.
>  	struct sk_buff *skb2;
>  	int ret;
>  	int count = 1;
> @@ -200,6 +201,7 @@ static int netem_enqueue(struct sk_buff 
>  		skb->data[net_random() % skb_headlen(skb)] ^= 1<<(net_random() % 8);
>  	}
>  
> +	cb = (struct netem_skb_cb *)skb->cb;
>  	if (q->gap == 0 		/* not doing reordering */
>  	    || q->counter < q->gap 	/* inside last reordering gap */
>  	    || q->reorder < get_crandom(&q->reorder_cor)) {
>
>   

Re: [PATCH] Fix slab corruption with netem

[Guillaume Chazarain]

Stephen Hemminger wrote :
>> -    struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
>> +    /* We don't fill cb now as skb_unshare() may invalidate it */
>> +    struct netem_skb_cb *cb = NULL;
>>   
> Would rather leave it unitialized, rather than setting to NULL.

I find that strange. If someone mistakenly uses cb before it is 
initialized it could be hard
to figure out why some memory is corrupted. But by initializing cb to 
NULL, it will
always trigger an Oops, so it will be easier to debug in this case.

Cheers.

-- 
Guillaume

Re: [PATCH] Fix slab corruption with netem

[Stephen Hemminger]

Guillaume Chazarain wrote:
> Stephen Hemminger wrote :
>>> -    struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
>>> +    /* We don't fill cb now as skb_unshare() may invalidate it */
>>> +    struct netem_skb_cb *cb = NULL;
>>>   
>> Would rather leave it unitialized, rather than setting to NULL.
>
> I find that strange. If someone mistakenly uses cb before it is 
> initialized it could be hard
> to figure out why some memory is corrupted. But by initializing cb to 
> NULL, it will
> always trigger an Oops, so it will be easier to debug in this case.
>
> Cheers.
>
Gcc will complain if variable is used before initialized, and I would 
rather see it at compile time than runtime.