[PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Zhengchao Shao]

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

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@huawei.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

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Jamal Hadi Salim]

On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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



> 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@huawei.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
>
>

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[shaozhengchao]

On 2023/5/29 3:05, Jamal Hadi Salim wrote:
> On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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@huawei.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
>>
>>
> 

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Peilin Ye]

On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:
> On 2023/5/29 3:05, Jamal Hadi Salim wrote:
> > On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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/
> >
>       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.

Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
from being regrafted (to elsewhere), and Zhengchao's patch prevents other
Qdiscs from being regrafted to ffff:fff1.

Thanks,
Peilin Ye

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[shaozhengchao]

On 2023/5/29 16:59, Peilin Ye wrote:
> On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:
>> On 2023/5/29 3:05, Jamal Hadi Salim wrote:
>>> On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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/
>>>
>>        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.
> 
> Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
> from being regrafted (to elsewhere), and Zhengchao's patch prevents other
> Qdiscs from being regrafted to ffff:fff1.
> 
> Thanks,
> Peilin Ye
> 
Hi Peilin:
	Thank you for your ack.

Zhengchao Shao

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Jamal Hadi Salim]

On Mon, May 29, 2023 at 4:59 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:
> > On 2023/5/29 3:05, Jamal Hadi Salim wrote:
> > > On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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/
> > >
> >       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.
>
> Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
> from being regrafted (to elsewhere), and Zhengchao's patch prevents other
> Qdiscs from being regrafted to ffff:fff1.


Ok, at first glance it was not obvious.
Do we catch all combinations? for egress (0xffffffff) allowed minor is
0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
is 0xfff1 and 0xfff2(clsact).

cheers,
jamal

> Thanks,
> Peilin Ye
>

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Peilin Ye]

On Mon, May 29, 2023 at 09:53:28AM -0400, Jamal Hadi Salim wrote:
> On Mon, May 29, 2023 at 4:59 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> > On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:
> > > On 2023/5/29 3:05, Jamal Hadi Salim wrote:
> > > > On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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/
> > > >
> > >       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.
> >
> > Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
> > from being regrafted (to elsewhere), and Zhengchao's patch prevents other
> > Qdiscs from being regrafted to ffff:fff1.
> 
> Ok, at first glance it was not obvious.
> Do we catch all combinations? for egress (0xffffffff) allowed minor is
> 0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
> is 0xfff1 and 0xfff2(clsact).

ffff:fff1 is special in tc_modify_qdisc(); if minor isn't fff1,
tc_modify_qdisc() thinks user wants to graft a Qdisc under existing ingress
or clsact Qdisc:

	if (clid != TC_H_INGRESS) {	/* ffff:fff1 */
		p = qdisc_lookup(dev, TC_H_MAJ(clid));
		if (!p) {
			NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
			return -ENOENT;
		}
		q = qdisc_leaf(p, clid);
	} else if (dev_ingress_queue_create(dev)) {
		q = dev_ingress_queue(dev)->qdisc_sleeping;
	}

This will go to the "parent != NULL" path in qdisc_graft(), and
sch_{ingress,clsact} doesn't implement cl_ops->graft(), so -EOPNOTSUPP will
be returned.

In short, yes, I think ffff:fff1 is the only case should be fixed.

By the way I just noticed that currently it is possible to create a e.g.
HTB class with a class ID of ffff:fff1...

  $ tc qdisc add dev eth0 root handle ffff: htb default fff1
  $ tc class add dev eth0 \
            parent ffff: classid ffff:fff1 htb rate 100%

Regrafting a Qdisc to such classes won't work as intended at all.  It's a
separate issue though.

Thanks,
Peilin Ye

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Peilin Ye]

On Sat, May 27, 2023 at 05:37:47PM +0800, Zhengchao Shao 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
> 
> 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@huawei.com>

Tested-by: Peilin Ye <peilin.ye@bytedance.com>

Thanks,
Peilin Ye

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[shaozhengchao]

Hi Peilin:
	Thank you for your reply.
On 2023/5/30 8:17, Peilin Ye wrote:
> On Mon, May 29, 2023 at 09:53:28AM -0400, Jamal Hadi Salim wrote:
>> On Mon, May 29, 2023 at 4:59 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>>> On Mon, May 29, 2023 at 09:10:23AM +0800, shaozhengchao wrote:
>>>> On 2023/5/29 3:05, Jamal Hadi Salim wrote:
>>>>> On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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/
>>>>>
>>>>        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.
>>>
>>> Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
>>> from being regrafted (to elsewhere), and Zhengchao's patch prevents other
>>> Qdiscs from being regrafted to ffff:fff1.
>>
>> Ok, at first glance it was not obvious.
>> Do we catch all combinations? for egress (0xffffffff) allowed minor is
>> 0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
>> is 0xfff1 and 0xfff2(clsact).
> 
> ffff:fff1 is special in tc_modify_qdisc(); if minor isn't fff1,
> tc_modify_qdisc() thinks user wants to graft a Qdisc under existing ingress
> or clsact Qdisc:
> 
> 	if (clid != TC_H_INGRESS) {	/* ffff:fff1 */
> 		p = qdisc_lookup(dev, TC_H_MAJ(clid));
> 		if (!p) {
> 			NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
> 			return -ENOENT;
> 		}
> 		q = qdisc_leaf(p, clid);
> 	} else if (dev_ingress_queue_create(dev)) {
> 		q = dev_ingress_queue(dev)->qdisc_sleeping;
> 	}
> 
> This will go to the "parent != NULL" path in qdisc_graft(), and
> sch_{ingress,clsact} doesn't implement cl_ops->graft(), so -EOPNOTSUPP will
> be returned.
> 
Yes, I agree.
> In short, yes, I think ffff:fff1 is the only case should be fixed.
> 
> By the way I just noticed that currently it is possible to create a e.g.
> HTB class with a class ID of ffff:fff1...
> 
>    $ tc qdisc add dev eth0 root handle ffff: htb default fff1
>    $ tc class add dev eth0 \
>              parent ffff: classid ffff:fff1 htb rate 100%
> 
> Regrafting a Qdisc to such classes won't work as intended at all.  It's a
> separate issue though.
> 
This seems to be another problem.

Zhengchao Shao
> Thanks,
> Peilin Ye
> 

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Paolo Abeni]

On Mon, 2023-05-29 at 17:17 -0700, Peilin Ye wrote:
> On Mon, May 29, 2023 at 09:53:28AM -0400, Jamal Hadi Salim wrote:
> > On Mon, May 29, 2023 at 4:59 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> > > Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
> > > from being regrafted (to elsewhere), and Zhengchao's patch prevents other
> > > Qdiscs from being regrafted to ffff:fff1.
> > 
> > Ok, at first glance it was not obvious.
> > Do we catch all combinations? for egress (0xffffffff) allowed minor is
> > 0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
> > is 0xfff1 and 0xfff2(clsact).
> 
> ffff:fff1 is special in tc_modify_qdisc(); if minor isn't fff1,
> tc_modify_qdisc() thinks user wants to graft a Qdisc under existing ingress
> or clsact Qdisc:
> 
> 	if (clid != TC_H_INGRESS) {	/* ffff:fff1 */
> 		p = qdisc_lookup(dev, TC_H_MAJ(clid));
> 		if (!p) {
> 			NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
> 			return -ENOENT;
> 		}
> 		q = qdisc_leaf(p, clid);
> 	} else if (dev_ingress_queue_create(dev)) {
> 		q = dev_ingress_queue(dev)->qdisc_sleeping;
> 	}
> 
> This will go to the "parent != NULL" path in qdisc_graft(), and
> sch_{ingress,clsact} doesn't implement cl_ops->graft(), so -EOPNOTSUPP will
> be returned.
> 
> In short, yes, I think ffff:fff1 is the only case should be fixed.
> 
> By the way I just noticed that currently it is possible to create a e.g.
> HTB class with a class ID of ffff:fff1...
> 
>   $ tc qdisc add dev eth0 root handle ffff: htb default fff1
>   $ tc class add dev eth0 \
>             parent ffff: classid ffff:fff1 htb rate 100%
> 
> Regrafting a Qdisc to such classes won't work as intended at all.  It's a
> separate issue though.

Jamal, are you ok with the above explanation? Perhaps it would be
worthy to add a specific test-case under tc-testing for this issue?

Thanks!

Paolo

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Peilin Ye]

On Tue, May 30, 2023 at 12:16:10PM +0200, Paolo Abeni wrote:
> Perhaps it would be worthy to add a specific test-case under tc-testing
> for this issue?

FYI I've started creating ingress.json tests for the issues fixed in this
series [1].  Zhengchao, I noticed that you've added a lot of tc-tests
before, would you like to add another one for this issue you fixed?  Or I
can do it if you're not going to.

[1] https://lore.kernel.org/r/cover.1685388545.git.peilin.ye@bytedance.com/

Thanks,
Peilin Ye

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Jamal Hadi Salim]

On Tue, May 30, 2023 at 6:16 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2023-05-29 at 17:17 -0700, Peilin Ye wrote:
> > On Mon, May 29, 2023 at 09:53:28AM -0400, Jamal Hadi Salim wrote:
> > > On Mon, May 29, 2023 at 4:59 AM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> > > > Ack, they are different: patch [4/6] prevents ingress (clsact) Qdiscs
> > > > from being regrafted (to elsewhere), and Zhengchao's patch prevents other
> > > > Qdiscs from being regrafted to ffff:fff1.
> > >
> > > Ok, at first glance it was not obvious.
> > > Do we catch all combinations? for egress (0xffffffff) allowed minor is
> > > 0xfff3 (clsact::) and 0xffff. For ingress (0xfffffff1) allowed minor
> > > is 0xfff1 and 0xfff2(clsact).
> >
> > ffff:fff1 is special in tc_modify_qdisc(); if minor isn't fff1,
> > tc_modify_qdisc() thinks user wants to graft a Qdisc under existing ingress
> > or clsact Qdisc:
> >
> >       if (clid != TC_H_INGRESS) {     /* ffff:fff1 */
> >               p = qdisc_lookup(dev, TC_H_MAJ(clid));
> >               if (!p) {
> >                       NL_SET_ERR_MSG(extack, "Failed to find specified qdisc");
> >                       return -ENOENT;
> >               }
> >               q = qdisc_leaf(p, clid);
> >       } else if (dev_ingress_queue_create(dev)) {
> >               q = dev_ingress_queue(dev)->qdisc_sleeping;
> >       }
> >
> > This will go to the "parent != NULL" path in qdisc_graft(), and
> > sch_{ingress,clsact} doesn't implement cl_ops->graft(), so -EOPNOTSUPP will
> > be returned.
> >
> > In short, yes, I think ffff:fff1 is the only case should be fixed.
> >
> > By the way I just noticed that currently it is possible to create a e.g.
> > HTB class with a class ID of ffff:fff1...
> >
> >   $ tc qdisc add dev eth0 root handle ffff: htb default fff1
> >   $ tc class add dev eth0 \
> >             parent ffff: classid ffff:fff1 htb rate 100%
> >
> > Regrafting a Qdisc to such classes won't work as intended at all.  It's a
> > separate issue though.
>
> Jamal, are you ok with the above explanation? Perhaps it would be
> worthy to add a specific test-case under tc-testing for this issue?
>

I am fine with this one going in as is and then adding tests. I will ACK it.

cheers,
jamal

> Thanks!
>
> Paolo
>

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[Jamal Hadi Salim]

On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao <shaozhengchao@huawei.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
>
> 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@huawei.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
>  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
>
>

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[shaozhengchao]

On 2023/5/31 8:35, Peilin Ye wrote:
> On Tue, May 30, 2023 at 12:16:10PM +0200, Paolo Abeni wrote:
>> Perhaps it would be worthy to add a specific test-case under tc-testing
>> for this issue?
> 
> FYI I've started creating ingress.json tests for the issues fixed in this
> series [1].  Zhengchao, I noticed that you've added a lot of tc-tests
> before, would you like to add another one for this issue you fixed?  Or I
> can do it if you're not going to.
> 
> [1] https://lore.kernel.org/r/cover.1685388545.git.peilin.ye@bytedance.com/
> 
> Thanks,
> Peilin Ye
> 
Hi Peilin:
	Thank you for the notice. I'd like to add the test-case in
another patch.

Zhengchao Shao

Re: [PATCH net] net: sched: fix NULL pointer dereference in mq_attach

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 27 May 2023 17:37:47 +0800 you 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
> 
> 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
> 
> [...]

Here is the summary with links:
  - [net] net: sched: fix NULL pointer dereference in mq_attach
    https://git.kernel.org/netdev/net/c/36eec020fab6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html