From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vlad Buslov <vladbu@mellanox.com>
Subject: Re: [PATCH net-next v6 11/11] net: sched: change action API to use array of pointers to actions
Date: Wed, 08 Aug 2018 14:41:08 +0300
Message-ID: <vbfr2j92ii3.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
References: <1530800673-12280-1-git-send-email-vladbu@mellanox.com> <1530800673-12280-12-git-send-email-vladbu@mellanox.com> <CAM_iQpWpxf=yRNadgN+q0rwhd_E0c7pAy7fG+A=asqgk9JRx0Q@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
        David Miller <davem@davemloft.net>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Yevgeny Kliteynik <kliteyn@mellanox.com>
To: Cong Wang <xiyou.wangcong@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-eopbgr50040.outbound.protection.outlook.com ([40.107.5.40]:2493
        "EHLO EUR03-VE1-obe.outbound.protection.outlook.com"
        rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
        id S1726733AbeHHOAk (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 8 Aug 2018 10:00:40 -0400
In-reply-to: <CAM_iQpWpxf=yRNadgN+q0rwhd_E0c7pAy7fG+A=asqgk9JRx0Q@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>


On Tue 07 Aug 2018 at 23:26, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>         attr_size = tcf_action_full_attrs_size(attr_size);
>>
>>         if (event == RTM_GETACTION)
>> -               ret = tcf_get_notify(net, portid, n, &actions, event, extack);
>> +               ret = tcf_get_notify(net, portid, n, actions, event, extack);
>>         else { /* delete */
>> -               ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
>> +               ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
>> +                                    attr_size, extack);
>>                 if (ret)
>>                         goto err;
>>                 return ret;
>>         }
>>  err:
>> -       tcf_action_put_lst(&actions);
>> +       tcf_action_put_many(&actions[acts_deleted]);
>>         return ret;
>
> How does this even work?
>
> You save an index in 'acts_deleted', but you pass &actions[acts_deleted]
> to tcf_action_put_many(), which seems you want to start from
> where it fails, but inside tcf_action_put_many() it starts from 0
> to TCA_ACT_MAX_PRIO, out-of-bound access at least?

Actions array is declared to be TCA_ACT_MAX_PRIO+1 in size, and
initialized to NULL pointers. In loop inside tcf_action_put_many() there
are two checks: One is that index is less than TCA_ACT_MAX_PRIO and
another one that pointer is not NULL. In this case I rely on extra NULL
pointer at the end of actions array to prevent out-of-bound access.