[PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[Jia-Ju Bai]

In dequeue_func(), there is an if statement on line 74 to check whether
skb is NULL:
    if (skb)

When skb is NULL, it is used on line 77:
    prefetch(&skb->end);

Thus, a possible null-pointer dereference may occur.

To fix this bug, skb->end is used when skb is not NULL.

This bug is found by a static analysis tool STCheck written by us.

Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
v2:
* Add a fix tag.
  Thank Jiri Pirko for helpful advice.
v3:
* Use a correct fix tag.
  Thank Jiri Pirko for helpful advice.

---
 net/sched/sch_codel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
index 25ef172c23df..30169b3adbbb 100644
--- a/net/sched/sch_codel.c
+++ b/net/sched/sch_codel.c
@@ -71,10 +71,10 @@ static struct sk_buff *dequeue_func(struct codel_vars *vars, void *ctx)
 	struct Qdisc *sch = ctx;
 	struct sk_buff *skb = __qdisc_dequeue_head(&sch->q);
 
-	if (skb)
+	if (skb) {
 		sch->qstats.backlog -= qdisc_pkt_len(skb);
-
-	prefetch(&skb->end); /* we'll need skb_shinfo() */
+		prefetch(&skb->end); /* we'll need skb_shinfo() */
+	}
 	return skb;
 }
 
-- 
2.17.0

Re: [PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[Jiri Pirko]

Mon, Jul 29, 2019 at 10:24:33AM CEST, baijiaju1990@gmail.com wrote:
>In dequeue_func(), there is an if statement on line 74 to check whether
>skb is NULL:
>    if (skb)
>
>When skb is NULL, it is used on line 77:
>    prefetch(&skb->end);
>
>Thus, a possible null-pointer dereference may occur.
>
>To fix this bug, skb->end is used when skb is not NULL.
>
>This bug is found by a static analysis tool STCheck written by us.
>
>Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")
>Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[David Miller]

From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Mon, 29 Jul 2019 16:24:33 +0800

> In dequeue_func(), there is an if statement on line 74 to check whether
> skb is NULL:
>     if (skb)
> 
> When skb is NULL, it is used on line 77:
>     prefetch(&skb->end);
> 
> Thus, a possible null-pointer dereference may occur.
> 
> To fix this bug, skb->end is used when skb is not NULL.
> 
> This bug is found by a static analysis tool STCheck written by us.
> 
> Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>

Applied and queued up for -stable.

Re: [PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[Cong Wang]

On Mon, Jul 29, 2019 at 1:24 AM Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>
> In dequeue_func(), there is an if statement on line 74 to check whether
> skb is NULL:
>     if (skb)
>
> When skb is NULL, it is used on line 77:
>     prefetch(&skb->end);
>
> Thus, a possible null-pointer dereference may occur.
>
> To fix this bug, skb->end is used when skb is not NULL.
>
> This bug is found by a static analysis tool STCheck written by us.

It doesn't dereference the pointer, it simply calculates the address
and passes it to gcc builtin prefetch. Both are fine when it is NULL,
as prefetching a NULL address should be okay for kernel.

So your changelog is misleading and it doesn't fix any bug,
although it does very slightly make the code better.

Re: [PATCH v3] net: sched: Fix a possible null-pointer dereference in dequeue_func()

[Eric Dumazet]

On 7/29/19 7:23 PM, Cong Wang wrote:
> On Mon, Jul 29, 2019 at 1:24 AM Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>>
>> In dequeue_func(), there is an if statement on line 74 to check whether
>> skb is NULL:
>>     if (skb)
>>
>> When skb is NULL, it is used on line 77:
>>     prefetch(&skb->end);
>>
>> Thus, a possible null-pointer dereference may occur.
>>
>> To fix this bug, skb->end is used when skb is not NULL.
>>
>> This bug is found by a static analysis tool STCheck written by us.
> 
> It doesn't dereference the pointer, it simply calculates the address
> and passes it to gcc builtin prefetch. Both are fine when it is NULL,
> as prefetching a NULL address should be okay for kernel.
> 
> So your changelog is misleading and it doesn't fix any bug,
> although it does very slightly make the code better.
> 

Original code was intentional.

A prefetch() need to be done as early as possible.

At the time of commit 76e3cc126bb223013a6b9a0e2a51238d1ef2e409
this was pretty clear :

+static struct sk_buff *dequeue(struct codel_vars *vars, struct Qdisc *sch)
+{
+	struct sk_buff *skb = __skb_dequeue(&sch->q);
+
+	prefetch(&skb->end); /* we'll need skb_shinfo() */
+	return skb;
+}


Really static analysis should learn about prefetch(X) being legal for _any_ value of X