netdev - Re: [PATCH net] net: sched: fix NULL pointer dereference in mq

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]

Message-ID: <c135ae5a-37ff-aa89-a3f7-976799181a04@huawei.com>
Date: Mon, 29 May 2023 09:10:23 +0800
From: shaozhengchao <shaozhengchao@...wei.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
CC: <netdev@...r.kernel.org>, <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <weiyongjun1@...wei.com>, <yuehaibing@...wei.com>,
	<wanghai38@...wei.com>, Peilin Ye <yepeilin.cs@...il.com>
Subject: Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach



On 2023/5/29 3:05, Jamal Hadi Salim wrote:
> On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@...wei.com> wrote:
>>
>> When use the following command to test:
>> 1)ip link add bond0 type bond
>> 2)ip link set bond0 up
>> 3)tc qdisc add dev bond0 root handle ffff: mq
>> 4)tc qdisc replace dev bond0 parent ffff:fff1 handle ffff: mq
>>
> 
> This is fixed by Peilin in this ongoing discussion:
> https://lore.kernel.org/netdev/cover.1684887977.git.peilin.ye@bytedance.com/
> 
> cheers,
> jamal
> 
>
Hi jamal:
	Thank you for your reply. I have notice Peilin's patches before,
and test after the patch is incorporated in local host. But it still
triggers the problem.
	Peilin's patches can be filtered out when the query result of
qdisc_lookup is of the ingress type. Here is 4/6 patch in his patches.
+if (q->flags & TCQ_F_INGRESS) {
+	NL_SET_ERR_MSG(extack,
+		       "Cannot regraft ingress or clsact Qdiscs");
+	return -EINVAL;
+}
	However, the query result of my test case in qdisc_lookup is mq.
Therefore, the patch cannot solve my problem.
Thank you.

Zhengchao Shao
> 
>> The kernel reports NULL pointer dereference issue. The stack information
>> is as follows:
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> Internal error: Oops: 0000000096000006 [#1] SMP
>> Modules linked in:
>> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> pc : mq_attach+0x44/0xa0
>> lr : qdisc_graft+0x20c/0x5cc
>> sp : ffff80000e2236a0
>> x29: ffff80000e2236a0 x28: ffff0000c0e59d80 x27: ffff0000c0be19c0
>> x26: ffff0000cae3e800 x25: 0000000000000010 x24: 00000000fffffff1
>> x23: 0000000000000000 x22: ffff0000cae3e800 x21: ffff0000c9df4000
>> x20: ffff0000c9df4000 x19: 0000000000000000 x18: ffff80000a934000
>> x17: ffff8000f5b56000 x16: ffff80000bb08000 x15: 0000000000000000
>> x14: 0000000000000000 x13: 6b6b6b6b6b6b6b6b x12: 6b6b6b6b00000001
>> x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
>> x8 : ffff0000c0be0730 x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000008
>> x5 : ffff0000cae3e864 x4 : 0000000000000000 x3 : 0000000000000001
>> x2 : 0000000000000001 x1 : ffff8000090bc23c x0 : 0000000000000000
>> Call trace:
>> mq_attach+0x44/0xa0
>> qdisc_graft+0x20c/0x5cc
>> tc_modify_qdisc+0x1c4/0x664
>> rtnetlink_rcv_msg+0x354/0x440
>> netlink_rcv_skb+0x64/0x144
>> rtnetlink_rcv+0x28/0x34
>> netlink_unicast+0x1e8/0x2a4
>> netlink_sendmsg+0x308/0x4a0
>> sock_sendmsg+0x64/0xac
>> ____sys_sendmsg+0x29c/0x358
>> ___sys_sendmsg+0x90/0xd0
>> __sys_sendmsg+0x7c/0xd0
>> __arm64_sys_sendmsg+0x2c/0x38
>> invoke_syscall+0x54/0x114
>> el0_svc_common.constprop.1+0x90/0x174
>> do_el0_svc+0x3c/0xb0
>> el0_svc+0x24/0xec
>> el0t_64_sync_handler+0x90/0xb4
>> el0t_64_sync+0x174/0x178
>>
>> This is because when mq is added for the first time, qdiscs in mq is set
>> to NULL in mq_attach(). Therefore, when replacing mq after adding mq, we
>> need to initialize qdiscs in the mq before continuing to graft. Otherwise,
>> it will couse NULL pointer dereference issue in mq_attach(). And the same
>> issue will occur in the attach functions of mqprio, taprio and htb.
>> ffff:fff1 means that the repalce qdisc is ingress. Ingress does not allow
>> any qdisc to be attached. Therefore, ffff:fff1 is incorrectly used, and
>> the command should be dropped.
>>
>> Fixes: 6ec1c69a8f64 ("net_sched: add classful multiqueue dummy scheduler")
>> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com>
>> ---
>>   net/sched/sch_api.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index fdb8f429333d..dbc9cf5eea89 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -1596,6 +1596,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>>                                          NL_SET_ERR_MSG(extack, "Qdisc parent/child loop detected");
>>                                          return -ELOOP;
>>                                  }
>> +                               if (clid == TC_H_INGRESS) {
>> +                                       NL_SET_ERR_MSG(extack, "Ingress cannot graft directly");
>> +                                       return -EINVAL;
>> +                               }
>>                                  qdisc_refcount_inc(q);
>>                                  goto graft;
>>                          } else {
>> --
>> 2.34.1
>>
>>
>