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]

Message-ID: <8e5a5a7cdf69b951b1d1077f0eaa4766cde4fbf1.camel@redhat.com>
Date:   Tue, 09 Mar 2021 16:47:59 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     "zhudi (J)" <zhudi21@...wei.com>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Chenxiang (EulerOS)" <rose.chen@...wei.com>
Subject: Re: 答复: [PATCH] net/sched:
 act_pedit: fix a NULL pointer deref in tcf_pedit_init

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

[...]

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