RE: [PATCH net] sch_htb: fix null pointer dereference on a null new_q

-----Original Message-----
From: Maxim Mikityanskiy [mailto:maximmi@nvidia.com]
Sent: Thursday, June 3, 2021 5:53 PM
To: wangyunjian <redacted>; netdev@vger.kernel.org
Cc: kuba@kernel.org; xiyou.wangcong@gmail.com; jhs@mojatatu.com;
jiri@resnulli.us; chenchanghu [off-list ref]; David S. Miller
[off-list ref]
Subject: Re: [PATCH net] sch_htb: fix null pointer dereference on a null new_q

On 2021-03-30 17:27, wangyunjian wrote:

quoted

From: Yunjian Wang <redacted>

sch_htb: fix null pointer dereference on a null new_q

Currently if new_q is null, the null new_q pointer will be dereference
when 'q->offload' is true. Fix this by adding a braces around
htb_parent_to_leaf_offload() to avoid it.

I admit there is a NULL pointer dereference bug, but I believe this fix is not
correct.

quoted

Addresses-Coverity: ("Dereference after null check")
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")

Please Cc the authors of the patches you fix, I found your commit accidentally.

quoted

Signed-off-by: Yunjian Wang <redacted>
---
  net/sched/sch_htb.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index

62e12cb41a3e..081c11d5717c 100644

--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c

@@ -1675,9 +1675,10 @@ static int htb_delete(struct Qdisc *sch, unsigned

long arg,

quoted

  					  cl->parent->common.classid,
  					  NULL);
  		if (q->offload) {
-			if (new_q)
+			if (new_q) {
  				htb_set_lockdep_class_child(new_q);
-			htb_parent_to_leaf_offload(sch, dev_queue, new_q);
+				htb_parent_to_leaf_offload(sch, dev_queue, new_q);

Yes, new_q can be NULL at this point, which will crash in qdisc_refcount_inc,
however, dropping the rest of the code of htb_parent_to_leaf_offload creates
another bug. For example, htb_graft_helper properly handles the case when
new_q is NULL, and by skipping this call you create an inconsistency:
dev_queue->qdisc will still point to the old qdisc, but cl->parent->leaf.q will
point to the new one (which will be noop_qdisc, because new_q was NULL). The
code is based on an assumption that these two pointers are the same, so it can
lead to refcount leaks.

The correct fix would be to add a NULL pointer check to protect
qdisc_refcount_inc inside htb_parent_to_leaf_offload.

OK, I will send a patch to fix it.

Thanks

(Also, while reviewing this code, I found out that leaf.q being noop_qdisc isn't
handled well in other places that read leaf.q->dev_queue - I'll have to address it
myself.)

Thanks,
Max

quoted

+			}
  		}
  	}