netdev - Re: [PATCH net] net/sched: flower: fix infinite loop in fl

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: <6650f0da68982ffa5bb71a773c5a3d588bd972c4.camel@redhat.com>
Date:   Tue, 25 Jun 2019 17:47:36 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Vlad Buslov <vladbu@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Lucas Bates <lucasb@...atatu.com>
Subject: Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()

On Thu, 2019-06-20 at 10:33 -0700, Cong Wang wrote:
> On Thu, Jun 20, 2019 at 5:52 AM Davide Caratti <dcaratti@...hat.com> wrote:
> > hello Cong, thanks for reading.
> > 
> > On Wed, 2019-06-19 at 15:04 -0700, Cong Wang wrote:
> > > On Wed, Jun 19, 2019 at 2:10 PM Davide Caratti <dcaratti@...hat.com> wrote:
> > > > on some CPUs (e.g. i686), tcf_walker.cookie has the same size as the IDR.
> > > > In this situation, the following script:
> > > > 
> > > >  # tc filter add dev eth0 ingress handle 0xffffffff flower action ok
> > > >  # tc filter show dev eth0 ingress
> > > > 
> > > > results in an infinite loop.

[...]

> I am not sure it is better to handle this overflow inside idr_get_next_ul()
> or just let its callers to handle it. According to the comments above
> idr_get_next_ul() it sounds like it is not expected to overflow, so...
> 
> diff --git a/lib/idr.c b/lib/idr.c
> index c34e256d2f01..a38f5e391cec 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -267,6 +267,9 @@ void *idr_get_next_ul(struct idr *idr, unsigned
> long *nextid)
>         if (!slot)
>                 return NULL;
> 
> +       /* overflow */
> +       if (iter.index < id)
> +               return NULL;
>         *nextid = iter.index + base;
>         return rcu_dereference_raw(*slot);
>  }

hello Cong,

I tested the above patch, but I still see the infinite loop on kernel
5.2.0-0.rc5.git0.1.fc31.i686 . 

idr_get_next_ul() returns the entry in the radix tree which is greater or 
equal to '*nextid' (which has the same value as 'id' in the above hunk). 
So, when the radix tree contains one slot with index equal to ULONG_MAX,
whatever can be the value of 'id', the condition in that if() will always 
be false (and the function will keep  returning non-NULL, hence the 
infinite loop).

I also tried this:

if (iter.index == id && id == ULONG_MAX) {
	return NULL;
}

it fixes the infinite loop, but it clearly breaks the function semantic
(and anyway, it's not sufficient to fix my test, at least with cls_flower
it still dumps the entry with id 0xffffffff several times).  I'm for
fixing the callers of idr_get_next_ul(), and in details:

- apply this patch for cls_flower
- change tcf_dump_walker() in act_api.c as follows, and add a TDC testcase
for 'gact'.

index 4e5d2e9ace5d..f34888c8a952 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -228,8 +228,11 @@ static int tcf_dump_walker(struct tcf_idrinfo
*idrinfo, struct sk_buff *skb,
 
        idr_for_each_entry_ul(idr, p, id) {
                index++;
-               if (index < s_i)
+               if (index < s_i) {
+                       if (id == ULONG_MAX)
+                               break;
                        continue;
+               }
 
                if (jiffy_since &&
                    time_after(jiffy_since,


WDYT?

thanks a lot,
-- 
davide