[PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[zhudi]

From: Di Zhu <zhudi21@huawei.com>

when we use syzkaller to fuzz-test our kernel, one NULL pointer dereference
BUG happened:

Write of size 96 at addr 0000000000000010 by task syz-executor.0/22376
==================================================================
BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
PGD 80000001dc1a9067 P4D 80000001dc1a9067 PUD 1a32b5067 PMD 0
[...]
Call Trace
memcpy  include/linux/string.h:345 [inline]
tcf_pedit_init+0x7b4/0xa10 net/sched/act_pedit.c:232
tcf_action_init_1+0x59b/0x730  net/sched/act_api.c:920
tcf_action_init+0x1ef/0x320  net/sched/act_api.c:975
tcf_action_add+0xd2/0x270  net/sched/act_api.c:1360
tc_ctl_action+0x267/0x290  net/sched/act_api.c:1412
[...]

The root cause is that we use kmalloc() to allocate mem space for
keys without checking if the ksize is 0. if ksize == 0 then kmalloc()
will return ZERO_SIZE_PTR currently defined as 16, not the NULL pointer.
eventually ZERO_SIZE_PTR is assigned to p->tcfp_keys. The next time you
update the action with ksize not equal to 0, the bug will appear. This is
because  p->tcfp_nkeys == 0, so it will not call kmalloc() to realloc mem
using new ksize and eventually, memcpy() use ZERO_SIZE_PTR as destination
address.

So we can allow memory reallocation even if current p->tcfp_nkeys == 0 and
kfree() supports ZERO_SIZE_PTR as input parameter

Signed-off-by: Di Zhu <zhudi21@huawei.com>
---
 net/sched/act_pedit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index b45304446e13..86514bd49ab6 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -216,7 +216,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	spin_lock_bh(&p->tcf_lock);
 
 	if (ret == ACT_P_CREATED ||
-	    (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys)) {
+	    (p->tcfp_nkeys != parm->nkeys)) {
 		keys = kmalloc(ksize, GFP_ATOMIC);
 		if (!keys) {
 			spin_unlock_bh(&p->tcf_lock);
-- 
2.23.0

Re: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[Davide Caratti]

hello, thanks for the patch!

On Tue, 2021-03-09 at 11:47 +0800, zhudi wrote:
> From: Di Zhu <zhudi21@huawei.com>
> 
> when we use syzkaller to fuzz-test our kernel, one NULL pointer dereference
> BUG happened:
> 
> Write of size 96 at addr 0000000000000010 by task syz-executor.0/22376
> ==================================================================
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> PGD 80000001dc1a9067 P4D 80000001dc1a9067 PUD 1a32b5067 PMD 0
> [...]
> Call Trace
> memcpy  include/linux/string.h:345 [inline]
> tcf_pedit_init+0x7b4/0xa10 net/sched/act_pedit.c:232
> tcf_action_init_1+0x59b/0x730  net/sched/act_api.c:920
> tcf_action_init+0x1ef/0x320  net/sched/act_api.c:975
> tcf_action_add+0xd2/0x270  net/sched/act_api.c:1360
> tc_ctl_action+0x267/0x290  net/sched/act_api.c:1412
> [...]
> 
> The root cause is that we use kmalloc() to allocate mem space for
> keys without checking if the ksize is 0. 

actually Linux does this:

173         parm = nla_data(pattr);
174         if (!parm->nkeys) {
175                 NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed");
176                 return -EINVAL;
177         }
178         ksize = parm->nkeys * sizeof(struct tc_pedit_key);
179         if (nla_len(pattr) < sizeof(*parm) + ksize) {
180                 NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid");
181                 return -EINVAL;
182         }

maybe it's not sufficient? If so, we can add something here. I'd prefer
to disallow inserting pedit actions with p->tcfp_nkeys equal to zero,
because they are going to trigger a WARN(1) in the traffic path (see
tcf_pedit_act() at the bottom).

Then, we can also remove all the tests on the positiveness of tcfp_nkeys
and the one you removed with your patch. WDYT?

thanks,
-- 
davide

答复: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[zhudi (J)]

> 
> hello, thanks for the patch!
> 
> On Tue, 2021-03-09 at 11:47 +0800, zhudi wrote:
> > From: Di Zhu <zhudi21@huawei.com>
> >
> > when we use syzkaller to fuzz-test our kernel, one NULL pointer
> dereference
> > BUG happened:
> >
> > Write of size 96 at addr 0000000000000010 by task syz-executor.0/22376
> >
> ========================================================== ========
> > BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000010
> > PGD 80000001dc1a9067 P4D 80000001dc1a9067 PUD 1a32b5067 PMD 0
> > [...]
> > Call Trace
> > memcpy  include/linux/string.h:345 [inline]
> > tcf_pedit_init+0x7b4/0xa10 net/sched/act_pedit.c:232
> > tcf_action_init_1+0x59b/0x730  net/sched/act_api.c:920
> > tcf_action_init+0x1ef/0x320  net/sched/act_api.c:975
> > tcf_action_add+0xd2/0x270  net/sched/act_api.c:1360
> > tc_ctl_action+0x267/0x290  net/sched/act_api.c:1412
> > [...]
> >
> > The root cause is that we use kmalloc() to allocate mem space for
> > keys without checking if the ksize is 0.
> 
> actually Linux does this:
> 
> 173         parm = nla_data(pattr);
> 174         if (!parm->nkeys) {
> 175                 NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be
> passed");
> 176                 return -EINVAL;
> 177         }
> 178         ksize = parm->nkeys * sizeof(struct tc_pedit_key);
> 179         if (nla_len(pattr) < sizeof(*parm) + ksize) {
> 180                 NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of
> TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid");
> 181                 return -EINVAL;
> 182         }
> 
> maybe it's not sufficient? If so, we can add something here. I'd prefer
> to disallow inserting pedit actions with p->tcfp_nkeys equal to zero,
> because they are going to trigger a WARN(1) in the traffic path (see
> tcf_pedit_act() at the bottom).

Yes, you are right.  I didn't notice your code submission(commit-id is f67169fef8dbcc1a) in 2019 
and the kernel we tested is a bit old. Normally,  your code submission can avoid this bug.

> 
> Then, we can also remove all the tests on the positiveness of tcfp_nkeys
> and the one you removed with your patch. WDYT?

Yes,  remove tests on the positiveness of tcfp_nkeys in this case can also make code more robust,
In particular,  at some abnormal situations. Should we do it now?

 I will retest with your code merged,  thanks.

> 
> thanks,
> --
> davide

Re: 答复: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[Davide Caratti]

On Tue, 2021-03-09 at 11:24 +0000, zhudi (J) wrote:
> > 
> > hello, thanks for the patch!
> > 
> > On Tue, 2021-03-09 at 11:47 +0800, zhudi wrote:
> > > From: Di Zhu <zhudi21@huawei.com>
> > > 
> > > when we use syzkaller to fuzz-test our kernel, one NULL pointer
> > dereference
> > > BUG happened:
> > > 
> > > Write of size 96 at addr 0000000000000010 by task syz-
> > > executor.0/22376
> > > 
> > ========================================================== ========
> > > BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000010

[...]

> > > 
> > > The root cause is that we use kmalloc() to allocate mem space for
> > > keys without checking if the ksize is 0.
> > 
> > actually Linux does this:
> > 
> > 173         parm = nla_data(pattr);
> > 174         if (!parm->nkeys) {
> > 175                 NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys
> > to be
> > passed");
> > 176                 return -EINVAL;
> > 177         }
> > 178         ksize = parm->nkeys * sizeof(struct tc_pedit_key);
> > 179         if (nla_len(pattr) < sizeof(*parm) + ksize) {
> > 180                 NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of
> > TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid");
> > 181                 return -EINVAL;
> > 182         }
> > 
> > maybe it's not sufficient? If so, we can add something here. I'd
> > prefer
> > to disallow inserting pedit actions with p->tcfp_nkeys equal to
> > zero,
> > because they are going to trigger a WARN(1) in the traffic path (see
> > tcf_pedit_act() at the bottom).
> 
> Yes, you are right.  I didn't notice your code submission(commit-id is
> f67169fef8dbcc1a) in 2019 
> and the kernel we tested is a bit old. Normally,  your code submission
> can avoid this bug.

well... :) 

that commit protected us against cases where param->nkeys was 0.
However, when parm->nkeys is equal to 536870912, the value of ksize
computed at line 178 becomes equal to 0.

The test at line 179 might help bailing out to error when the product
parm->nkeys * sizeof(struct tc_pedit_key) does an integer overflow, but 
I'm quite sure that negative or zero values of ksize are "unwanted" here
and probably it's still possible to craft a netlink request where parm-
>nkeys is 536870912 and nla_len(pattr) is bigger than sizeof(*parm).

Then, tcf_pedit_keys_ex_parse() might still help us detecting a bad
message, but to stay safe:

> > Then, we can also remove all the tests on the positiveness of
> > tcfp_nkeys
> > and the one you removed with your patch. WDYT?
> 
> Yes,  remove tests on the positiveness of tcfp_nkeys in this case can
> also make code more robust,
> In particular,  at some abnormal situations. Should we do it now?

I think it's correct to use an unsigned value for ksize, and protect
against the integer overflow anyway. If you want to re-submit a patch
for that, I will be happy to pass it through tdc.py selftest :)


>  I will retest with your code merged,  thanks.

either ways ok, just let me know.

thanks!
-- 
davide

Re: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit_init

[Cong Wang]

On Tue, Mar 9, 2021 at 3:24 AM zhudi (J) <zhudi21@huawei.com> wrote:
>
> Yes, you are right.  I didn't notice your code submission(commit-id is f67169fef8dbcc1a) in 2019
> and the kernel we tested is a bit old. Normally,  your code submission can avoid this bug.

Please do not submit patches for the latest kernel unless you test it.

Thanks.