netdev - Re: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in tcf_pedit

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]

Date:   Tue, 09 Mar 2021 09:53:37 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     zhudi <zhudi21@...wei.com>, jhs@...atatu.com,
        xiyou.wangcong@...il.com
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        rose.chen@...wei.com
Subject: Re: [PATCH] net/sched: act_pedit: fix a NULL pointer deref in
 tcf_pedit_init

hello, thanks for the patch!

On Tue, 2021-03-09 at 11:47 +0800, zhudi wrote:
> From: Di Zhu <zhudi21@...wei.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