* [PATCH net] sch_htb: fix null pointer dereference on a null new_q
@ 2021-03-30 14:27 wangyunjian
2021-03-30 21:00 ` patchwork-bot+netdevbpf
2021-06-03 9:52 ` Maxim Mikityanskiy
0 siblings, 2 replies; 4+ messages in thread
From: wangyunjian @ 2021-03-30 14:27 UTC (permalink / raw)
To: netdev; +Cc: kuba, xiyou.wangcong, jhs, jiri, chenchanghu, Yunjian Wang
From: Yunjian Wang <wangyunjian@huawei.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.
Addresses-Coverity: ("Dereference after null check")
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.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);
+ }
}
}
--
2.23.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] sch_htb: fix null pointer dereference on a null new_q
2021-03-30 14:27 [PATCH net] sch_htb: fix null pointer dereference on a null new_q wangyunjian
@ 2021-03-30 21:00 ` patchwork-bot+netdevbpf
2021-06-03 9:52 ` Maxim Mikityanskiy
1 sibling, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-03-30 21:00 UTC (permalink / raw)
To: wangyunjian; +Cc: netdev, kuba, xiyou.wangcong, jhs, jiri, chenchanghu
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Tue, 30 Mar 2021 22:27:48 +0800 you wrote:
> From: Yunjian Wang <wangyunjian@huawei.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.
>
> [...]
Here is the summary with links:
- [net] sch_htb: fix null pointer dereference on a null new_q
https://git.kernel.org/netdev/net/c/ae81feb7338c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] sch_htb: fix null pointer dereference on a null new_q
2021-03-30 14:27 [PATCH net] sch_htb: fix null pointer dereference on a null new_q wangyunjian
2021-03-30 21:00 ` patchwork-bot+netdevbpf
@ 2021-06-03 9:52 ` Maxim Mikityanskiy
2021-06-04 3:31 ` wangyunjian
1 sibling, 1 reply; 4+ messages in thread
From: Maxim Mikityanskiy @ 2021-06-03 9:52 UTC (permalink / raw)
To: wangyunjian, netdev
Cc: kuba, xiyou.wangcong, jhs, jiri, chenchanghu, David S. Miller
On 2021-03-30 17:27, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.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@huawei.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
> + }
> }
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH net] sch_htb: fix null pointer dereference on a null new_q
2021-06-03 9:52 ` Maxim Mikityanskiy
@ 2021-06-04 3:31 ` wangyunjian
0 siblings, 0 replies; 4+ messages in thread
From: wangyunjian @ 2021-06-04 3:31 UTC (permalink / raw)
To: Maxim Mikityanskiy, netdev
Cc: kuba, xiyou.wangcong, jhs, jiri, chenchanghu, David S. Miller
> -----Original Message-----
> From: Maxim Mikityanskiy [mailto:maximmi@nvidia.com]
> Sent: Thursday, June 3, 2021 5:53 PM
> To: wangyunjian <wangyunjian@huawei.com>; netdev@vger.kernel.org
> Cc: kuba@kernel.org; xiyou.wangcong@gmail.com; jhs@mojatatu.com;
> jiri@resnulli.us; chenchanghu <chenchanghu@huawei.com>; David S. Miller
> <davem@davemloft.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@huawei.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@huawei.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
>
> > + }
> > }
> > }
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-04 3:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 14:27 [PATCH net] sch_htb: fix null pointer dereference on a null new_q wangyunjian
2021-03-30 21:00 ` patchwork-bot+netdevbpf
2021-06-03 9:52 ` Maxim Mikityanskiy
2021-06-04 3:31 ` wangyunjian
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).