[net] net/flow: fix fc->percpu NULL pointer dereference

Now we will force to do garbage collection if any policy removed in
xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini()
first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini()
-> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer
dereference when check percpu_empty. The code path looks like:

flow_cache_fini()
  - fc->percpu = NULL
xfrm_policy_fini()
  - xfrm_policy_flush()
    - xfrm_garbage_collect()
      - flow_cache_flush()
        - flow_cache_percpu_empty()
	  - fcp = per_cpu_ptr(fc->percpu, cpu)

To reproduce, just add ipsec in netns and then remove the netns.

Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/core/flow.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Hi Steffen,

BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
But If we put the check in flow_cache_percpu_empty(), we can prevent
other functions set fc->percpu to NULL, although not much possible : )

So I'm not quite sure whether we should put the check in
flow_cache_percpu_empty() or in xfrm_policy_flush().

Do you have any suggestion?

Thanks
Hangbin

2017-06-09 16:13 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> Now we will force to do garbage collection if any policy removed in
> xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini()
> first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini()
> -> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer
> dereference when check percpu_empty. The code path looks like:
>
> flow_cache_fini()
>   - fc->percpu = NULL
> xfrm_policy_fini()
>   - xfrm_policy_flush()
>     - xfrm_garbage_collect()
>       - flow_cache_flush()
>         - flow_cache_percpu_empty()
>           - fcp = per_cpu_ptr(fc->percpu, cpu)
>
> To reproduce, just add ipsec in netns and then remove the netns.
>
> Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/core/flow.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/flow.c b/net/core/flow.c
> index f7f5d19..321fc53 100644
> --- a/net/core/flow.c
> +++ b/net/core/flow.c
> @@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu)
>         struct flow_cache_percpu *fcp;
>         unsigned int i;
>
> -       fcp = per_cpu_ptr(fc->percpu, cpu);
> -       for (i = 0; i < flow_cache_hash_size(fc); i++)
> -               if (!hlist_empty(&fcp->hash_table[i]))
> -                       return 0;
> +       if (fc->percpu) {
> +               fcp = per_cpu_ptr(fc->percpu, cpu);
> +               for (i = 0; i < flow_cache_hash_size(fc); i++)
> +                       if (!hlist_empty(&fcp->hash_table[i]))
> +                               return 0;
> +       }
> +
>         return 1;
>  }
>
> --
> 2.5.5
>

On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
> Hi Steffen,
> 
> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
> But If we put the check in flow_cache_percpu_empty(), we can prevent
> other functions set fc->percpu to NULL, although not much possible : )
> 
> So I'm not quite sure whether we should put the check in
> flow_cache_percpu_empty() or in xfrm_policy_flush().

Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?

On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
>> Hi Steffen,
>>
>> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
>> But If we put the check in flow_cache_percpu_empty(), we can prevent
>> other functions set fc->percpu to NULL, although not much possible : )
>>
>> So I'm not quite sure whether we should put the check in
>> flow_cache_percpu_empty() or in xfrm_policy_flush().
>
> Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?
>
That would be a better fix. seems safe as what flow_cache_fini does
is only to free fcp->hash_table and stop timer, I didn't see  it has
any dependence on xfrm_policy stuff.

2017-06-09 16:43 GMT+08:00 Xin Long <lucien.xin@gmail.com>:
> On Fri, Jun 9, 2017 at 4:32 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
>> On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
>>> Hi Steffen,
>>>
>>> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
>>> But If we put the check in flow_cache_percpu_empty(), we can prevent
>>> other functions set fc->percpu to NULL, although not much possible : )
>>>
>>> So I'm not quite sure whether we should put the check in
>>> flow_cache_percpu_empty() or in xfrm_policy_flush().
>>
>> Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?

Yes, that would be easy fix. I have been thinking about that.  But if we change
 the order in xfrm_net_exit(), do we also need to change the order in
 xfrm_net_init()? That would change a lot.

If no need, that would be good.

>>
> That would be a better fix. seems safe as what flow_cache_fini does
> is only to free fcp->hash_table and stop timer, I didn't see  it has
> any dependence on xfrm_policy stuff.

I'm not familiar about this part. So not sure about the influence if we free
the flow cache after xfrm_policy_fini(). I need do some test first.

I would also be appreciate if you or some one could make sure if it doesn't
influence anything.

Thanks
Hangbin