netdev - [PATCH net 2/2] sch_htb: Fix offload cleanup in htb_destroy on htb

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]

Date:   Thu, 11 Mar 2021 16:42:06 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        <netdev@...r.kernel.org>, Maxim Mikityanskiy <maximmi@...dia.com>,
        <syzbot+b53a709f04722ca12a3c@...kaller.appspotmail.com>
Subject: [PATCH net 2/2] sch_htb: Fix offload cleanup in htb_destroy on htb_init failure

htb_init may fail to do the offload if it's not supported or if a
runtime error happens when allocating direct qdiscs. In those cases
TC_HTB_CREATE command is not sent to the driver, however, htb_destroy
gets called anyway and attempts to send TC_HTB_DESTROY.

It shouldn't happen, because the driver didn't receive TC_HTB_CREATE,
and also because the driver may not support ndo_setup_tc at all, while
q->offload is true, and htb_destroy mistakenly thinks the offload is
supported. Trying to call ndo_setup_tc in the latter case will lead to a
NULL pointer dereference.

This commit fixes the issues with htb_destroy by deferring assignment of
q->offload until after the TC_HTB_CREATE command. The necessary cleanup
of the offload entities is already done in htb_init.

Reported-by: syzbot+b53a709f04722ca12a3c@...kaller.appspotmail.com
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Suggested-by: Eric Dumazet <eric.dumazet@...il.com>
Signed-off-by: Maxim Mikityanskiy <maximmi@...dia.com>
Reviewed-by: Tariq Toukan <tariqt@...dia.com>
---
 net/sched/sch_htb.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index b23203159996..62e12cb41a3e 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1020,6 +1020,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	struct nlattr *tb[TCA_HTB_MAX + 1];
 	struct tc_htb_glob *gopt;
 	unsigned int ntx;
+	bool offload;
 	int err;
 
 	qdisc_watchdog_init(&q->watchdog, sch);
@@ -1044,9 +1045,9 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	if (gopt->version != HTB_VER >> 16)
 		return -EINVAL;
 
-	q->offload = nla_get_flag(tb[TCA_HTB_OFFLOAD]);
+	offload = nla_get_flag(tb[TCA_HTB_OFFLOAD]);
 
-	if (q->offload) {
+	if (offload) {
 		if (sch->parent != TC_H_ROOT)
 			return -EOPNOTSUPP;
 
@@ -1076,7 +1077,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 		q->rate2quantum = 1;
 	q->defcls = gopt->defcls;
 
-	if (!q->offload)
+	if (!offload)
 		return 0;
 
 	for (ntx = 0; ntx < q->num_direct_qdiscs; ntx++) {
@@ -1107,12 +1108,14 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	if (err)
 		goto err_free_qdiscs;
 
+	/* Defer this assignment, so that htb_destroy skips offload-related
+	 * parts (especially calling ndo_setup_tc) on errors.
+	 */
+	q->offload = true;
+
 	return 0;
 
 err_free_qdiscs:
-	/* TC_HTB_CREATE call failed, avoid any further calls to the driver. */
-	q->offload = false;
-
 	for (ntx = 0; ntx < q->num_direct_qdiscs && q->direct_qdiscs[ntx];
 	     ntx++)
 		qdisc_put(q->direct_qdiscs[ntx]);
-- 
2.25.1