| Message ID | 20190729082433.28981-1-baijiaju1990@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3] net: sched: Fix a possible null-pointer dereference in dequeue_func() | expand |
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>
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.
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.
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
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; }
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(-)