[PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Matteo Croce]

When a matchall classifier is added, there is a small time interval in
which tp->root is NULL. If we receive a packet in this small time slice
a NULL pointer dereference will happen, leading to a kernel panic:

    # tc qdisc replace dev eth0 ingress
    # tc filter add dev eth0 parent ffff: matchall action gact drop
    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000034
    Mem abort info:
      ESR = 0x96000005
      Exception class = DABT (current EL), IL = 32 bits
      SET = 0, FnV = 0
      EA = 0, S1PTW = 0
    Data abort info:
      ISV = 0, ISS = 0x00000005
      CM = 0, WnR = 0
    user pgtable: 4k pages, 39-bit VAs, pgdp = 00000000a623d530
    [0000000000000034] pgd=0000000000000000, pud=0000000000000000
    Internal error: Oops: 96000005 [#1] SMP
    Modules linked in: cls_matchall sch_ingress nls_iso8859_1 nls_cp437 vfat fat m25p80 spi_nor mtd xhci_plat_hcd xhci_hcd phy_generic sfp mdio_i2c usbcore i2c_mv64xxx marvell10g mvpp2 usb_common spi_orion mvmdio i2c_core sbsa_gwdt phylink ip_tables x_tables autofs4
    Process ksoftirqd/0 (pid: 9, stack limit = 0x0000000009de7d62)
    CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.1.0-rc6 #21
    Hardware name: Marvell 8040 MACCHIATOBin Double-shot (DT)
    pstate: 40000005 (nZcv daif -PAN -UAO)
    pc : mall_classify+0x28/0x78 [cls_matchall]
    lr : tcf_classify+0x78/0x138
    sp : ffffff80109db9d0
    x29: ffffff80109db9d0 x28: ffffffc426058800
    x27: 0000000000000000 x26: ffffffc425b0dd00
    x25: 0000000020000000 x24: 0000000000000000
    x23: ffffff80109dbac0 x22: 0000000000000001
    x21: ffffffc428ab5100 x20: ffffffc425b0dd00
    x19: ffffff80109dbac0 x18: 0000000000000000
    x17: 0000000000000000 x16: 0000000000000000
    x15: 0000000000000000 x14: 0000000000000000
    x13: ffffffbf108ad288 x12: dead000000000200
    x11: 00000000f0000000 x10: 0000000000000001
    x9 : ffffffbf1089a220 x8 : 0000000000000001
    x7 : ffffffbebffaa950 x6 : 0000000000000000
    x5 : 000000442d6ba000 x4 : 0000000000000000
    x3 : ffffff8008735ad8 x2 : ffffff80109dbac0
    x1 : ffffffc425b0dd00 x0 : ffffff8010592078
    Call trace:
     mall_classify+0x28/0x78 [cls_matchall]
     tcf_classify+0x78/0x138
     __netif_receive_skb_core+0x29c/0xa20
     __netif_receive_skb_one_core+0x34/0x60
     __netif_receive_skb+0x28/0x78
     netif_receive_skb_internal+0x2c/0xc0
     napi_gro_receive+0x1a0/0x1d8
     mvpp2_poll+0x928/0xb18 [mvpp2]
     net_rx_action+0x108/0x378
     __do_softirq+0x128/0x320
     run_ksoftirqd+0x44/0x60
     smpboot_thread_fn+0x168/0x1b0
     kthread+0x12c/0x130
     ret_from_fork+0x10/0x1c
    Code: aa0203f3 aa1e03e0 d503201f f9400684 (b9403480)
    ---[ end trace fc71e2ef7b8ab5a5 ]---
    Kernel panic - not syncing: Fatal exception in interrupt
    SMP: stopping secondary CPUs
    Kernel Offset: disabled
    CPU features: 0x002,00002000
    Memory Limit: none
    Rebooting in 1 seconds..

Fix this by adding a NULL check in mall_classify().

Fixes: ed76f5edccc9 ("net: sched: protect filter_chain list with filter_chain_lock mutex")
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 net/sched/cls_matchall.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index a13bc351a414..3d021f2aad1c 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -32,6 +32,9 @@ static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 {
 	struct cls_mall_head *head = rcu_dereference_bh(tp->root);
 
+	if (unlikely(!head))
+		return -1;
+
 	if (tc_skip_sw(head->flags))
 		return -1;
 
-- 
2.21.0

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Matteo Croce]

On Thu, May 2, 2019 at 10:51 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> When a matchall classifier is added, there is a small time interval in
> which tp->root is NULL. If we receive a packet in this small time slice
> a NULL pointer dereference will happen, leading to a kernel panic:
>

Hi,

I forgot to mark it as v2. Will someone handle it, or I have to
resubmit a v2 or v3?

Regards,
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Cong Wang]

On Thu, May 2, 2019 at 1:55 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Thu, May 2, 2019 at 10:51 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > When a matchall classifier is added, there is a small time interval in
> > which tp->root is NULL. If we receive a packet in this small time slice
> > a NULL pointer dereference will happen, leading to a kernel panic:
> >
>
> Hi,
>
> I forgot to mark it as v2. Will someone handle it, or I have to
> resubmit a v2 or v3?

As long as David doesn't get confused on which one he should apply,
it is okay.

Thanks.

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Cong Wang]

On Thu, May 2, 2019 at 1:51 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> When a matchall classifier is added, there is a small time interval in
> which tp->root is NULL. If we receive a packet in this small time slice
> a NULL pointer dereference will happen, leading to a kernel panic:
...
> Fix this by adding a NULL check in mall_classify().
>
> Fixes: ed76f5edccc9 ("net: sched: protect filter_chain list with filter_chain_lock mutex")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>


Thanks for the update!

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[David Miller]

From: Matteo Croce <mcroce@redhat.com>
Date: Thu,  2 May 2019 10:51:05 +0200

> When a matchall classifier is added, there is a small time interval in
> which tp->root is NULL. If we receive a packet in this small time slice
> a NULL pointer dereference will happen, leading to a kernel panic:
 ...
> Fix this by adding a NULL check in mall_classify().
> 
> Fixes: ed76f5edccc9 ("net: sched: protect filter_chain list with filter_chain_lock mutex")
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Applied and queued up for -stable, thanks.

[PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Matteo Croce]

When a matchall classifier is added, there is a small time interval in
which tp->root is NULL. If we receive a packet in this small time slice
a NULL pointer dereference will happen, leading to a kernel panic:

    # tc qdisc replace dev eth0 ingress
    # tc filter add dev eth0 parent ffff: matchall action gact drop
    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000034
    Mem abort info:
      ESR = 0x96000005
      Exception class = DABT (current EL), IL = 32 bits
      SET = 0, FnV = 0
      EA = 0, S1PTW = 0
    Data abort info:
      ISV = 0, ISS = 0x00000005
      CM = 0, WnR = 0
    user pgtable: 4k pages, 39-bit VAs, pgdp = 00000000a623d530
    [0000000000000034] pgd=0000000000000000, pud=0000000000000000
    Internal error: Oops: 96000005 [#1] SMP
    Modules linked in: cls_matchall sch_ingress nls_iso8859_1 nls_cp437 vfat fat m25p80 spi_nor mtd xhci_plat_hcd xhci_hcd phy_generic sfp mdio_i2c usbcore i2c_mv64xxx marvell10g mvpp2 usb_common spi_orion mvmdio i2c_core sbsa_gwdt phylink ip_tables x_tables autofs4
    Process ksoftirqd/0 (pid: 9, stack limit = 0x0000000009de7d62)
    CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.1.0-rc6 #21
    Hardware name: Marvell 8040 MACCHIATOBin Double-shot (DT)
    pstate: 40000005 (nZcv daif -PAN -UAO)
    pc : mall_classify+0x28/0x78 [cls_matchall]
    lr : tcf_classify+0x78/0x138
    sp : ffffff80109db9d0
    x29: ffffff80109db9d0 x28: ffffffc426058800
    x27: 0000000000000000 x26: ffffffc425b0dd00
    x25: 0000000020000000 x24: 0000000000000000
    x23: ffffff80109dbac0 x22: 0000000000000001
    x21: ffffffc428ab5100 x20: ffffffc425b0dd00
    x19: ffffff80109dbac0 x18: 0000000000000000
    x17: 0000000000000000 x16: 0000000000000000
    x15: 0000000000000000 x14: 0000000000000000
    x13: ffffffbf108ad288 x12: dead000000000200
    x11: 00000000f0000000 x10: 0000000000000001
    x9 : ffffffbf1089a220 x8 : 0000000000000001
    x7 : ffffffbebffaa950 x6 : 0000000000000000
    x5 : 000000442d6ba000 x4 : 0000000000000000
    x3 : ffffff8008735ad8 x2 : ffffff80109dbac0
    x1 : ffffffc425b0dd00 x0 : ffffff8010592078
    Call trace:
     mall_classify+0x28/0x78 [cls_matchall]
     tcf_classify+0x78/0x138
     __netif_receive_skb_core+0x29c/0xa20
     __netif_receive_skb_one_core+0x34/0x60
     __netif_receive_skb+0x28/0x78
     netif_receive_skb_internal+0x2c/0xc0
     napi_gro_receive+0x1a0/0x1d8
     mvpp2_poll+0x928/0xb18 [mvpp2]
     net_rx_action+0x108/0x378
     __do_softirq+0x128/0x320
     run_ksoftirqd+0x44/0x60
     smpboot_thread_fn+0x168/0x1b0
     kthread+0x12c/0x130
     ret_from_fork+0x10/0x1c
    Code: aa0203f3 aa1e03e0 d503201f f9400684 (b9403480)
    ---[ end trace fc71e2ef7b8ab5a5 ]---
    Kernel panic - not syncing: Fatal exception in interrupt
    SMP: stopping secondary CPUs
    Kernel Offset: disabled
    CPU features: 0x002,00002000
    Memory Limit: none
    Rebooting in 1 seconds..

Fix this by creating a dummy action in the init which is skipped
by mall_classify, and remove it upon the first action set in
mall_change. Doing this we avoid adding an extra check in the classify.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 net/sched/cls_matchall.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index a13bc351a414..8aaa9d80c2ca 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -27,6 +27,10 @@ struct cls_mall_head {
 	struct rcu_work rwork;
 };
 
+static const struct cls_mall_head match_none = {
+	.flags = TCA_CLS_FLAGS_SKIP_SW
+};
+
 static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 			 struct tcf_result *res)
 {
@@ -42,6 +46,8 @@ static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 
 static int mall_init(struct tcf_proto *tp)
 {
+	rcu_assign_pointer(tp->root, &match_none);
+
 	return 0;
 }
 
@@ -114,7 +120,7 @@ static void mall_destroy(struct tcf_proto *tp, bool rtnl_held,
 {
 	struct cls_mall_head *head = rtnl_dereference(tp->root);
 
-	if (!head)
+	if (head == &match_none)
 		return;
 
 	tcf_unbind_filter(tp, &head->res);
@@ -132,7 +138,7 @@ static void *mall_get(struct tcf_proto *tp, u32 handle)
 {
 	struct cls_mall_head *head = rtnl_dereference(tp->root);
 
-	if (head && head->handle == handle)
+	if (head != &match_none && head->handle == handle)
 		return head;
 
 	return NULL;
@@ -178,7 +184,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	if (!tca[TCA_OPTIONS])
 		return -EINVAL;
 
-	if (head)
+	if (head != &match_none)
 		return -EEXIST;
 
 	err = nla_parse_nested(tb, TCA_MATCHALL_MAX, tca[TCA_OPTIONS],
@@ -253,7 +259,7 @@ static void mall_walk(struct tcf_proto *tp, struct tcf_walker *arg,
 	if (arg->count < arg->skip)
 		goto skip;
 
-	if (!head)
+	if (head == &match_none)
 		return;
 	if (arg->fn(tp, head, arg) < 0)
 		arg->stop = 1;
@@ -298,7 +304,7 @@ static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
 	struct nlattr *nest;
 	int cpu;
 
-	if (!head)
+	if (!head || head == &match_none)
 		return skb->len;
 
 	t->tcm_handle = head->handle;
-- 
2.21.0

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Cong Wang]

On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> When a matchall classifier is added, there is a small time interval in
> which tp->root is NULL. If we receive a packet in this small time slice
> a NULL pointer dereference will happen, leading to a kernel panic:

Hmm, why not just check tp->root against NULL in mall_classify()?

Also, which is the offending commit here? Please add a Fixes: tag.

Thanks.

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Matteo Croce]

On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > When a matchall classifier is added, there is a small time interval in
> > which tp->root is NULL. If we receive a packet in this small time slice
> > a NULL pointer dereference will happen, leading to a kernel panic:
>
> Hmm, why not just check tp->root against NULL in mall_classify()?
>
> Also, which is the offending commit here? Please add a Fixes: tag.
>
> Thanks.

Hi,

I just want to avoid an extra check which would be made for every packet.
Probably the benefit over a check is negligible, but it's still a
per-packet thing.
If you prefer a simple check, I can make a v2 that way.

For the fixes tag, I didn't put it as I'm not really sure about the
offending commit. I guess it's the following, what do you think?

commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7
Author: Vlad Buslov <vladbu@mellanox.com>
Date:   Mon Feb 11 10:55:38 2019 +0200

    net: sched: protect filter_chain list with filter_chain_lock mutex

-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Cong Wang]

On Wed, May 1, 2019 at 2:27 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote:
> > >
> > > When a matchall classifier is added, there is a small time interval in
> > > which tp->root is NULL. If we receive a packet in this small time slice
> > > a NULL pointer dereference will happen, leading to a kernel panic:
> >
> > Hmm, why not just check tp->root against NULL in mall_classify()?
> >
> > Also, which is the offending commit here? Please add a Fixes: tag.
> >
> > Thanks.
>
> Hi,
>
> I just want to avoid an extra check which would be made for every packet.
> Probably the benefit over a check is negligible, but it's still a
> per-packet thing.
> If you prefer a simple check, I can make a v2 that way.

Yeah, I think that is better, you can add an unlikely() for performance
concern, as NULL is a rare case.


>
> For the fixes tag, I didn't put it as I'm not really sure about the
> offending commit. I guess it's the following, what do you think?
>
> commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7
> Author: Vlad Buslov <vladbu@mellanox.com>
> Date:   Mon Feb 11 10:55:38 2019 +0200
>
>     net: sched: protect filter_chain list with filter_chain_lock mutex

I think you are right, this is the commit introduced the code
that inserts the tp before fully initializing it. Please Cc Vlad
for your v2, in case we blame a wrong commit here.


BTW, it looks like cls_cgroup needs a same fix. Please audit
other tc filters as well.

Thanks!

Re: [PATCH net] cls_matchall: avoid panic when receiving a packet before filter set

[Vlad Buslov]

On Thu 02 May 2019 at 03:48, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, May 1, 2019 at 2:27 AM Matteo Croce <mcroce@redhat.com> wrote:
>>
>> On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >
>> > On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote:
>> > >
>> > > When a matchall classifier is added, there is a small time interval in
>> > > which tp->root is NULL. If we receive a packet in this small time slice
>> > > a NULL pointer dereference will happen, leading to a kernel panic:
>> >
>> > Hmm, why not just check tp->root against NULL in mall_classify()?
>> >
>> > Also, which is the offending commit here? Please add a Fixes: tag.
>> >
>> > Thanks.
>>
>> Hi,
>>
>> I just want to avoid an extra check which would be made for every packet.
>> Probably the benefit over a check is negligible, but it's still a
>> per-packet thing.
>> If you prefer a simple check, I can make a v2 that way.
>
> Yeah, I think that is better, you can add an unlikely() for performance
> concern, as NULL is a rare case.
>
>
>>
>> For the fixes tag, I didn't put it as I'm not really sure about the
>> offending commit. I guess it's the following, what do you think?
>>
>> commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7
>> Author: Vlad Buslov <vladbu@mellanox.com>
>> Date:   Mon Feb 11 10:55:38 2019 +0200
>>
>>     net: sched: protect filter_chain list with filter_chain_lock mutex
>
> I think you are right, this is the commit introduced the code
> that inserts the tp before fully initializing it. Please Cc Vlad
> for your v2, in case we blame a wrong commit here.
>
>
> BTW, it looks like cls_cgroup needs a same fix. Please audit
> other tc filters as well.
>
> Thanks!

Sorry for late response. This is indeed the offending commit that should
be referenced by fixes tag.

Thanks for fixing this, Matteo!