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] [day] [month] [year] [list]

Date:   Fri, 4 Jun 2021 03:31:17 +0000
From:   wangyunjian <wangyunjian@...wei.com>
To:     Maxim Mikityanskiy <maximmi@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "kuba@...nel.org" <kuba@...nel.org>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        chenchanghu <chenchanghu@...wei.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: RE: [PATCH net] sch_htb: fix null pointer dereference on a null new_q

> -----Original Message-----
> From: Maxim Mikityanskiy [mailto:maximmi@...dia.com]
> Sent: Thursday, June 3, 2021 5:53 PM
> To: wangyunjian <wangyunjian@...wei.com>; netdev@...r.kernel.org
> Cc: kuba@...nel.org; xiyou.wangcong@...il.com; jhs@...atatu.com;
> jiri@...nulli.us; chenchanghu <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.

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
> 
> > +			}
> >   		}
> >   	}
> >
> >