netdev - Re: [PATCH net] sch_htb: fix null pointer dereference on a null new

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a0ea3a1-0d31-83f4-0ba1-80154c37d048@nvidia.com>
Date:   Thu, 3 Jun 2021 12:52:38 +0300
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     wangyunjian <wangyunjian@...wei.com>, <netdev@...r.kernel.org>
CC:     <kuba@...nel.org>, <xiyou.wangcong@...il.com>, <jhs@...atatu.com>,
        <jiri@...nulli.us>, <chenchanghu@...wei.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] sch_htb: fix null pointer dereference on a null new_q

On 2021-03-30 17:27, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@...wei.com>
> 
> 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.

> 
> 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.

> 
> Signed-off-by: Yunjian Wang <wangyunjian@...wei.com>
> ---
>   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,
>   					  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.

(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

> +			}
>   		}
>   	}
>   
>