Re: memory leak in tcindex_set_parms

[Eric Dumazet <eric.dumazet@gmail.com>]

On 2/4/20 2:26 PM, Cong Wang wrote:
> On Tue, Feb 4, 2020 at 1:22 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 4 Feb 2020 10:03:16 -0800
>>
>>>
>>>
>>> On 2/4/20 9:58 AM, syzbot wrote:
>>>> Hello,
>>>>
>>>> syzbot found the following crash on:
>>>>
>>>> HEAD commit:    322bf2d3 Merge branch 'for-5.6' of git://git.kernel.org/pu..
>>>> git tree:       upstream
>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=1111f8e6e00000
>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=8d0490614a000a37
>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=f0bbb2287b8993d4fa74
>>>> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
>>>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17db90f6e00000
>>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13a94511e00000
>>>>
>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>> Reported-by: syzbot+f0bbb2287b8993d4fa74@syzkaller.appspotmail.com
>>>>
>>>>
>>>
>>> Might have been fixed already ?
>>>
>>> commit 599be01ee567b61f4471ee8078870847d0a11e8e    net_sched: fix an OOB access in cls_tcindex
>>
>> My reaction was actually that this bug is caused by this commit :)
> 
> I think it is neither of the cases, I will test the following change:
> 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 09b7dc5fe7e0..2495b15ca78c 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -454,6 +454,7 @@ tcindex_set_parms(struct net *net, struct
> tcf_proto *tp, unsigned long base,
>         oldp = p;
>         r->res = cr;
>         tcf_exts_change(&r->exts, &e);
> +       tcf_exts_destroy(&e);
> 

The only thing the repro fires for me on latest net tree is an UBSAN warning about a silly ->shift

I have not tried it on the old kernel.

[  170.331009] UBSAN: Undefined behaviour in net/sched/cls_tcindex.c:239:29
[  170.337729] shift exponent 19375 is too large for 32-bit type 'int'


diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 0323aee03de7efbb99c7943be078765c74dfdf2e..42436ae61b7a64a7244a9df03dc397e5aa103a86 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -339,9 +339,13 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
        if (tb[TCA_TCINDEX_MASK])
                cp->mask = nla_get_u16(tb[TCA_TCINDEX_MASK]);
 
-       if (tb[TCA_TCINDEX_SHIFT])
+       if (tb[TCA_TCINDEX_SHIFT]) {
                cp->shift = nla_get_u32(tb[TCA_TCINDEX_SHIFT]);
-
+               if (cp->shift > 16) {
+                       err = -EINVAL;
+                       goto errout;
+               }
+       }
        if (!cp->hash) {
                /* Hash not specified, use perfect hash if the upper limit
                 * of the hashing index is below the threshold.