RE: [net] fq_codel: fix NULL pointer deref in fq_codel_reset
> -Original Message- > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > Sent: Monday, June 11, 2018 1:23 PM > To: Keller, Jacob E > Cc: Kirsher, Jeffrey T ; da...@davemloft.net; > netdev@vger.kernel.org; nhor...@redhat.com; sassm...@redhat.com; > jogre...@redhat.com; Eric Dumazet > Subject: Re: [net] fq_codel: fix NULL pointer deref in fq_codel_reset > > On Mon, Jun 11, 2018 at 12:57 PM, Keller, Jacob E > wrote: > > > > I'm open to alternative suggestinos for fixing this, I think Eric suggested > > that > maybe we should just remove the ->reset() call from qdisc_destroy..? > > You can't remove ->reset() for non-failure call path. > > For failure path, yeah, but it is much simpler to just make > q->flows_cnt be 0 for this specific case. Alright. I'll rework the patch to set flows_cnt to 0 when init fails. Thanks, Jake
RE: [net] fq_codel: fix NULL pointer deref in fq_codel_reset
> -Original Message- > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > Sent: Monday, June 11, 2018 1:03 PM > To: Kirsher, Jeffrey T > Cc: David Miller ; Keller, Jacob E > ; Linux Kernel Network Developers > ; nhor...@redhat.com; sassm...@redhat.com; > jogre...@redhat.com; Eric Dumazet > Subject: Re: [net] fq_codel: fix NULL pointer deref in fq_codel_reset > > Making q->flows_cnt 0 is simpler and easier to understand. Feel free to propose such a patch :) Thanks, Jake
Re: [net] fq_codel: fix NULL pointer deref in fq_codel_reset
On Mon, Jun 11, 2018 at 12:57 PM, Keller, Jacob E wrote: > > I'm open to alternative suggestinos for fixing this, I think Eric suggested > that maybe we should just remove the ->reset() call from qdisc_destroy..? You can't remove ->reset() for non-failure call path. For failure path, yeah, but it is much simpler to just make q->flows_cnt be 0 for this specific case.
Re: [net] fq_codel: fix NULL pointer deref in fq_codel_reset
On Mon, Jun 11, 2018 at 10:00 AM, Jeff Kirsher wrote: > > We could mitigate some of these issues by changing fq_codel_init to more > explicitly cleanup after itself when failing. For example, we could > ensure that q->flowcnt was set to 0 so that the loop over each flow in > fq_codel_reset would not trigger. However, this would not prevent a NULL > pointer dereference when attempting to memset the q->backlogs. Are you saying memset(ptr, 0, 0) is not nop?? :-/ Making q->flows_cnt 0 is simpler and easier to understand.
RE: [net] fq_codel: fix NULL pointer deref in fq_codel_reset
> -Original Message- > From: Kirsher, Jeffrey T > Sent: Monday, June 11, 2018 10:00 AM > To: da...@davemloft.net > Cc: Keller, Jacob E ; netdev@vger.kernel.org; > nhor...@redhat.com; sassm...@redhat.com; jogre...@redhat.com; Eric > Dumazet ; Kirsher, Jeffrey T > > Subject: [net] fq_codel: fix NULL pointer deref in fq_codel_reset > > From: Jacob Keller > > The function qdisc_create_dftl attempts to create a default qdisc. If > this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy > function calls the ->reset op on the qdisc. > > In the case of sch_fq_codel.c, this function will panic when the qdisc > wasn't properly initialized: > >kernel: BUG: unable to handle kernel NULL pointer dereference at > 0008 >kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] >kernel: PGD 0 P4D 0 >kernel: Oops: [#1] SMP PTI >kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp llc > devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei joydev > i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 ixgbe > drm_kms_helper isci ttm firewire_ohci >kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf > ipmi_msghandler [last unloaded: i40e] >kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE > 4.16.13custom-fq- > codel-test+ #3 >kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS > SE5C600.86B.02.05.0004.051120151007 05/11/2015 >kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] >kernel: RSP: 0018:bfbf4c1fb620 EFLAGS: 00010246 >kernel: RAX: 0400 RBX: RCX: 05b9 >kernel: RDX: RSI: 9d03264a60c0 RDI: 9cfd17b31c00 >kernel: RBP: 0001 R08: 000260c0 R09: b679c3e9 >kernel: R10: f1dab06a0e80 R11: 9cfd163af800 R12: 9cfd17b31c00 >kernel: R13: 0001 R14: 9cfd153de600 R15: 0001 >kernel: FS: 7fdec2f92800() GS:9d032648() > knlGS: >kernel: CS: 0010 DS: ES: CR0: 80050033 >kernel: CR2: 0008 CR3: 000c1956a006 CR4: 000606e0 >kernel: Call Trace: >kernel: qdisc_destroy+0x56/0x140 >kernel: qdisc_create_dflt+0x8b/0xb0 >kernel: mq_init+0xc1/0xf0 >kernel: qdisc_create_dflt+0x5a/0xb0 >kernel: dev_activate+0x205/0x230 >kernel: __dev_open+0xf5/0x160 >kernel: __dev_change_flags+0x1a3/0x210 >kernel: dev_change_flags+0x21/0x60 >kernel: do_setlink+0x660/0xdf0 >kernel: ? down_trylock+0x25/0x30 >kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] >kernel: ? rtnl_newlink+0x816/0x990 >kernel: ? _xfs_buf_find+0x327/0x580 [xfs] >kernel: ? _cond_resched+0x15/0x30 >kernel: ? kmem_cache_alloc+0x20/0x1b0 >kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 >kernel: ? rtnl_calcit.isra.30+0x100/0x100 >kernel: ? netlink_rcv_skb+0x4c/0x120 >kernel: ? netlink_unicast+0x19e/0x260 >kernel: ? netlink_sendmsg+0x1ff/0x3c0 >kernel: ? sock_sendmsg+0x36/0x40 >kernel: ? ___sys_sendmsg+0x295/0x2f0 >kernel: ? ebitmap_cmp+0x6d/0x90 >kernel: ? dev_get_by_name_rcu+0x73/0x90 >kernel: ? skb_dequeue+0x52/0x60 >kernel: ? __inode_wait_for_writeback+0x7f/0xf0 >kernel: ? bit_waitqueue+0x30/0x30 >kernel: ? fsnotify_grab_connector+0x3c/0x60 >kernel: ? __sys_sendmsg+0x51/0x90 >kernel: ? do_syscall_64+0x74/0x180 >kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 >kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 > 00 00 00 > 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 > 8b 3b e8 > 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 >kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: bfbf4c1fb620 >kernel: CR2: 0008 >kernel: ---[ end trace e81a62bede66274e ]--- > > This occurs because if fq_codel_init fails, it has left the private data >
[net] fq_codel: fix NULL pointer deref in fq_codel_reset
From: Jacob Keller The function qdisc_create_dftl attempts to create a default qdisc. If this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy function calls the ->reset op on the qdisc. In the case of sch_fq_codel.c, this function will panic when the qdisc wasn't properly initialized: kernel: BUG: unable to handle kernel NULL pointer dereference at 0008 kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] kernel: PGD 0 P4D 0 kernel: Oops: [#1] SMP PTI kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp llc devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei joydev i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 ixgbe drm_kms_helper isci ttm firewire_ohci kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf ipmi_msghandler [last unloaded: i40e] kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE 4.16.13custom-fq-codel-test+ #3 kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS SE5C600.86B.02.05.0004.051120151007 05/11/2015 kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] kernel: RSP: 0018:bfbf4c1fb620 EFLAGS: 00010246 kernel: RAX: 0400 RBX: RCX: 05b9 kernel: RDX: RSI: 9d03264a60c0 RDI: 9cfd17b31c00 kernel: RBP: 0001 R08: 000260c0 R09: b679c3e9 kernel: R10: f1dab06a0e80 R11: 9cfd163af800 R12: 9cfd17b31c00 kernel: R13: 0001 R14: 9cfd153de600 R15: 0001 kernel: FS: 7fdec2f92800() GS:9d032648() knlGS: kernel: CS: 0010 DS: ES: CR0: 80050033 kernel: CR2: 0008 CR3: 000c1956a006 CR4: 000606e0 kernel: Call Trace: kernel: qdisc_destroy+0x56/0x140 kernel: qdisc_create_dflt+0x8b/0xb0 kernel: mq_init+0xc1/0xf0 kernel: qdisc_create_dflt+0x5a/0xb0 kernel: dev_activate+0x205/0x230 kernel: __dev_open+0xf5/0x160 kernel: __dev_change_flags+0x1a3/0x210 kernel: dev_change_flags+0x21/0x60 kernel: do_setlink+0x660/0xdf0 kernel: ? down_trylock+0x25/0x30 kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] kernel: ? rtnl_newlink+0x816/0x990 kernel: ? _xfs_buf_find+0x327/0x580 [xfs] kernel: ? _cond_resched+0x15/0x30 kernel: ? kmem_cache_alloc+0x20/0x1b0 kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 kernel: ? rtnl_calcit.isra.30+0x100/0x100 kernel: ? netlink_rcv_skb+0x4c/0x120 kernel: ? netlink_unicast+0x19e/0x260 kernel: ? netlink_sendmsg+0x1ff/0x3c0 kernel: ? sock_sendmsg+0x36/0x40 kernel: ? ___sys_sendmsg+0x295/0x2f0 kernel: ? ebitmap_cmp+0x6d/0x90 kernel: ? dev_get_by_name_rcu+0x73/0x90 kernel: ? skb_dequeue+0x52/0x60 kernel: ? __inode_wait_for_writeback+0x7f/0xf0 kernel: ? bit_waitqueue+0x30/0x30 kernel: ? fsnotify_grab_connector+0x3c/0x60 kernel: ? __sys_sendmsg+0x51/0x90 kernel: ? do_syscall_64+0x74/0x180 kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 00 00 00 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 8b 3b e8 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: bfbf4c1fb620 kernel: CR2: 0008 kernel: ---[ end trace e81a62bede66274e ]--- This occurs because if fq_codel_init fails, it has left the private data in an incomplete state. For example, if tcf_block_get fails, (as in the above panic), then q->flows and q->backlogs will be NULL. Thus they will cause NULL pointer access when attempting to reset them in fq_codel_reset. We could mitigate some of these issues by changing fq_codel_init to more explicitly cleanup after itself when failing. For example, we could ensure that q->flowcnt was set to 0 so that the loop over each flow in fq_codel_reset would not trigger. However, this would not prevent a NULL pointer dereference when attempting to memset the q->backlogs. Instead, just add a NULL check prior to attempting to reset these fields. Cc: Eric Dumazet Signed-off-by: Jacob Keller Tested-by: Andrew Bowers Signed-off-by: Jeff Kirsher --- net/sched/sch_fq_codel.c | 15 +-- 1 file changed, 9
