From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933215AbdKGAGw (ORCPT ); Mon, 6 Nov 2017 19:06:52 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:52590 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932787AbdKFXyc (ORCPT ); Mon, 6 Nov 2017 18:54:32 -0500 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 From: Ben Hutchings To: linux-kernel@vger.kernel.org, stable@vger.kernel.org CC: akpm@linux-foundation.org, "Eric Dumazet" , "Dmitry Vyukov" , "David S. Miller" Date: Mon, 06 Nov 2017 23:03:02 +0000 Message-ID: X-Mailer: LinuxStableQueue (scripts by bwh) Subject: [PATCH 3.16 171/294] net_sched: fix error recovery at qdisc creation In-Reply-To: X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 3.16.50-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Eric Dumazet commit 87b60cfacf9f17cf71933c6e33b66e68160af71d upstream. Dmitry reported uses after free in qdisc code [1] The problem here is that ops->init() can return an error. qdisc_create_dflt() then call ops->destroy(), while qdisc_create() does _not_ call it. Four qdisc chose to call their own ops->destroy(), assuming their caller would not. This patch makes sure qdisc_create() calls ops->destroy() and fixes the four qdisc to avoid double free. [1] BUG: KASAN: use-after-free in mq_destroy+0x242/0x290 net/sched/sch_mq.c:33 at addr ffff8801d415d440 Read of size 8 by task syz-executor2/5030 CPU: 0 PID: 5030 Comm: syz-executor2 Not tainted 4.3.5-smp-DEV #119 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 0000000000000046 ffff8801b435b870 ffffffff81bbbed4 ffff8801db000400 ffff8801d415d440 ffff8801d415dc40 ffff8801c4988510 ffff8801b435b898 ffffffff816682b1 ffff8801b435b928 ffff8801d415d440 ffff8801c49880c0 Call Trace: [] __dump_stack lib/dump_stack.c:15 [inline] [] dump_stack+0x6c/0x98 lib/dump_stack.c:51 [] kasan_object_err+0x21/0x70 mm/kasan/report.c:158 [] print_address_description mm/kasan/report.c:196 [inline] [] kasan_report_error+0x1b4/0x4b0 mm/kasan/report.c:285 [] kasan_report mm/kasan/report.c:305 [inline] [] __asan_report_load8_noabort+0x43/0x50 mm/kasan/report.c:326 [] mq_destroy+0x242/0x290 net/sched/sch_mq.c:33 [] qdisc_destroy+0x12d/0x290 net/sched/sch_generic.c:953 [] qdisc_create_dflt+0xf0/0x120 net/sched/sch_generic.c:848 [] attach_default_qdiscs net/sched/sch_generic.c:1029 [inline] [] dev_activate+0x6ad/0x880 net/sched/sch_generic.c:1064 [] __dev_open+0x221/0x320 net/core/dev.c:1403 [] __dev_change_flags+0x15e/0x3e0 net/core/dev.c:6858 [] dev_change_flags+0x8e/0x140 net/core/dev.c:6926 [] dev_ifsioc+0x446/0x890 net/core/dev_ioctl.c:260 [] dev_ioctl+0x1ba/0xb80 net/core/dev_ioctl.c:546 [] sock_do_ioctl+0x99/0xb0 net/socket.c:879 [] sock_ioctl+0x2a0/0x390 net/socket.c:958 [] vfs_ioctl fs/ioctl.c:44 [inline] [] do_vfs_ioctl+0x8a8/0xe50 fs/ioctl.c:611 [] SYSC_ioctl fs/ioctl.c:626 [inline] [] SyS_ioctl+0x94/0xc0 fs/ioctl.c:617 [] entry_SYSCALL_64_fastpath+0x12/0x17 Signed-off-by: Eric Dumazet Reported-by: Dmitry Vyukov Signed-off-by: David S. Miller [bwh: Backported to 3.16: adjust context] Signed-off-by: Ben Hutchings --- net/sched/sch_api.c | 2 ++ net/sched/sch_hhf.c | 8 ++++++-- net/sched/sch_mq.c | 10 +++------- net/sched/sch_mqprio.c | 19 ++++++------------- net/sched/sch_sfq.c | 3 ++- 5 files changed, 19 insertions(+), 23 deletions(-) --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -978,6 +978,8 @@ qdisc_create(struct net_device *dev, str return sch; } + /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ + ops->destroy(sch); err_out3: dev_put(dev); kfree((char *) sch - sch->padded); --- a/net/sched/sch_hhf.c +++ b/net/sched/sch_hhf.c @@ -644,7 +644,9 @@ static int hhf_init(struct Qdisc *sch, s q->hhf_arrays[i] = hhf_zalloc(HHF_ARRAYS_LEN * sizeof(u32)); if (!q->hhf_arrays[i]) { - hhf_destroy(sch); + /* Note: hhf_destroy() will be called + * by our caller. + */ return -ENOMEM; } } @@ -655,7 +657,9 @@ static int hhf_init(struct Qdisc *sch, s q->hhf_valid_bits[i] = hhf_zalloc(HHF_ARRAYS_LEN / BITS_PER_BYTE); if (!q->hhf_valid_bits[i]) { - hhf_destroy(sch); + /* Note: hhf_destroy() will be called + * by our caller. + */ return -ENOMEM; } } --- a/net/sched/sch_mq.c +++ b/net/sched/sch_mq.c @@ -52,7 +52,7 @@ static int mq_init(struct Qdisc *sch, st /* pre-allocate qdiscs, attachment can't fail */ priv->qdiscs = kcalloc(dev->num_tx_queues, sizeof(priv->qdiscs[0]), GFP_KERNEL); - if (priv->qdiscs == NULL) + if (!priv->qdiscs) return -ENOMEM; for (ntx = 0; ntx < dev->num_tx_queues; ntx++) { @@ -60,18 +60,14 @@ static int mq_init(struct Qdisc *sch, st qdisc = qdisc_create_dflt(dev_queue, default_qdisc_ops, TC_H_MAKE(TC_H_MAJ(sch->handle), TC_H_MIN(ntx + 1))); - if (qdisc == NULL) - goto err; + if (!qdisc) + return -ENOMEM; priv->qdiscs[ntx] = qdisc; qdisc->flags |= TCQ_F_ONETXQUEUE; } sch->flags |= TCQ_F_MQROOT; return 0; - -err: - mq_destroy(sch); - return -ENOMEM; } static void mq_attach(struct Qdisc *sch) --- a/net/sched/sch_mqprio.c +++ b/net/sched/sch_mqprio.c @@ -117,20 +117,17 @@ static int mqprio_init(struct Qdisc *sch /* pre-allocate qdisc, attachment can't fail */ priv->qdiscs = kcalloc(dev->num_tx_queues, sizeof(priv->qdiscs[0]), GFP_KERNEL); - if (priv->qdiscs == NULL) { - err = -ENOMEM; - goto err; - } + if (!priv->qdiscs) + return -ENOMEM; for (i = 0; i < dev->num_tx_queues; i++) { dev_queue = netdev_get_tx_queue(dev, i); qdisc = qdisc_create_dflt(dev_queue, default_qdisc_ops, TC_H_MAKE(TC_H_MAJ(sch->handle), TC_H_MIN(i + 1))); - if (qdisc == NULL) { - err = -ENOMEM; - goto err; - } + if (!qdisc) + return -ENOMEM; + priv->qdiscs[i] = qdisc; qdisc->flags |= TCQ_F_ONETXQUEUE; } @@ -143,7 +140,7 @@ static int mqprio_init(struct Qdisc *sch priv->hw_owned = 1; err = dev->netdev_ops->ndo_setup_tc(dev, qopt->num_tc); if (err) - goto err; + return err; } else { netdev_set_num_tc(dev, qopt->num_tc); for (i = 0; i < qopt->num_tc; i++) @@ -157,10 +154,6 @@ static int mqprio_init(struct Qdisc *sch sch->flags |= TCQ_F_MQROOT; return 0; - -err: - mqprio_destroy(sch); - return err; } static void mqprio_attach(struct Qdisc *sch) --- a/net/sched/sch_sfq.c +++ b/net/sched/sch_sfq.c @@ -770,9 +770,10 @@ static int sfq_init(struct Qdisc *sch, s q->ht = sfq_alloc(sizeof(q->ht[0]) * q->divisor); q->slots = sfq_alloc(sizeof(q->slots[0]) * q->maxflows); if (!q->ht || !q->slots) { - sfq_destroy(sch); + /* Note: sfq_destroy() will be called by our caller */ return -ENOMEM; } + for (i = 0; i < q->divisor; i++) q->ht[i] = SFQ_EMPTY_SLOT;