From: Di Zhu <redacted>
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 <redacted>
---
net/sched/act_pedit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
hello, thanks for the patch!
On Tue, 2021-03-09 at 11:47 +0800, zhudi wrote:
From: Di Zhu <redacted>
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
BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
quoted
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.
BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
[...]
quoted
quoted
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:
quoted
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
From: Cong Wang <hidden> Date: 2021-03-09 18:01:24
On Tue, Mar 9, 2021 at 3:24 AM zhudi (J) [off-list ref] 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.