Re: [PATCH net] net: ping6: Fix possible leaked pernet namespace in pingv6_init()

[Chen Zhongjin]

Hi,

On 2022/11/4 0:58, Kuniyuki Iwashima wrote:
> From:   Chen Zhongjin <chenzhongjin@xxxxxxxxxx>
> Date:   Thu, 3 Nov 2022 17:03:45 +0800
> > When IPv6 module initializing in pingv6_init(), inet6_register_protosw()
> > is possible to fail but returns without any error cleanup.
> The change itself looks sane, but how does it fail ?
> It seems inet6_register_protosw() never fails for pingv6_protosw.
> Am I missing something ?

Thanks for reminding! I only injected error return value for functions 
but didn't notice the inner logic.

Rechecked and find you are right that inet6_register_protosw() is safe 
for this case.

Sorry for bothering, please reject this. Will check carefully next time.


Best,

Chen

> ---8<---
> static struct inet_protosw pingv6_protosw = {
> 	.type =      SOCK_DGRAM,         <-- .type < SOCK_MAX
> 	.protocol =  IPPROTO_ICMPV6,
> 	.prot =      &pingv6_prot,
> 	.ops =       &inet6_sockraw_ops,
> 	.flags =     INET_PROTOSW_REUSE,  <-- always makes `answer` NULL
> };
>
> int inet6_register_protosw(struct inet_protosw *p)
> {
> 	struct list_head *lh;
> 	struct inet_protosw *answer;
> 	struct list_head *last_perm;
> 	int protocol = p->protocol;
> 	int ret;
>
> 	spin_lock_bh(&inetsw6_lock);
>
> 	ret = -EINVAL;
> 	if (p->type >= SOCK_MAX)
> 		goto out_illegal;
>
> 	/* If we are trying to override a permanent protocol, bail. */
> 	answer = NULL;
> 	ret = -EPERM;
> 	last_perm = &inetsw6[p->type];
> 	list_for_each(lh, &inetsw6[p->type]) {
> 		answer = list_entry(lh, struct inet_protosw, list);
>
> 		/* Check only the non-wild match. */
> 		if (INET_PROTOSW_PERMANENT & answer->flags) {
> 			if (protocol == answer->protocol)
> 				break;
> 			last_perm = lh;
> 		}
>
> 		answer = NULL;
> 	}
> 	if (answer)
> 		goto out_permanent;
> ...
> 	list_add_rcu(&p->list, last_perm);
> 	ret = 0;
> out:
> 	spin_unlock_bh(&inetsw6_lock);
> 	return ret;
>
> out_permanent:
> 	pr_err("Attempt to override permanent protocol %d\n", protocol);
> 	goto out;
>
> out_illegal:
> 	pr_err("Ignoring attempt to register invalid socket type %d\n",
> 	       p->type);
> 	goto out;
> }
> ---8<---
>
> > This leaves wild ops in namespace list and when another module tries to
> > add or delete pernet namespace it triggers page fault.
> > Although IPv6 cannot be unloaded now, this error should still be handled
> > to avoid kernel panic during IPv6 initialization.
> >
> > BUG: unable to handle page fault for address: fffffbfff80bab69
> > CPU: 0 PID: 434 Comm: modprobe
> > RIP: 0010:unregister_pernet_operations+0xc9/0x450
> > Call Trace:
> >   <TASK>
> >   unregister_pernet_subsys+0x31/0x3e
> >   nf_tables_module_exit+0x44/0x6a [nf_tables]
> >   __do_sys_delete_module.constprop.0+0x34f/0x5b0
> >   ...
> >
> > Fix it by adding error handling in pingv6_init(), and add a helper
> I'm wondering this could be another place.
>
>
> > function pingv6_ops_unset to avoid duplicate code.
> >
> > Fixes: d862e5461423 ("net: ipv6: Implement /proc/net/icmp6.")
> > Signed-off-by: Chen Zhongjin <chenzhongjin@xxxxxxxxxx>
> > ---
> >   net/ipv6/ping.c | 30 ++++++++++++++++++++++--------
> >   1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> > index 86c26e48d065..5df688dd5208 100644
> > --- a/net/ipv6/ping.c
> > +++ b/net/ipv6/ping.c
> > @@ -277,10 +277,21 @@ static struct pernet_operations ping_v6_net_ops = {
> >   };
> >   #endif
> >   
> > +static void pingv6_ops_unset(void)
> > +{
> > +	pingv6_ops.ipv6_recv_error = dummy_ipv6_recv_error;
> > +	pingv6_ops.ip6_datagram_recv_common_ctl = dummy_ip6_datagram_recv_ctl;
> > +	pingv6_ops.ip6_datagram_recv_specific_ctl = dummy_ip6_datagram_recv_ctl;
> > +	pingv6_ops.icmpv6_err_convert = dummy_icmpv6_err_convert;
> > +	pingv6_ops.ipv6_icmp_error = dummy_ipv6_icmp_error;
> > +	pingv6_ops.ipv6_chk_addr = dummy_ipv6_chk_addr;
> > +}
> > +
> >   int __init pingv6_init(void)
> >   {
> > +	int ret;
> >   #ifdef CONFIG_PROC_FS
> > -	int ret = register_pernet_subsys(&ping_v6_net_ops);
> > +	ret = register_pernet_subsys(&ping_v6_net_ops);
> >   	if (ret)
> >   		return ret;
> >   #endif
> > @@ -291,7 +302,15 @@ int __init pingv6_init(void)
> >   	pingv6_ops.icmpv6_err_convert = icmpv6_err_convert;
> >   	pingv6_ops.ipv6_icmp_error = ipv6_icmp_error;
> >   	pingv6_ops.ipv6_chk_addr = ipv6_chk_addr;
> > -	return inet6_register_protosw(&pingv6_protosw);
> > +
> > +	ret = inet6_register_protosw(&pingv6_protosw);
> > +	if (ret) {
> > +		pingv6_ops_unset();
> > +#ifdef CONFIG_PROC_FS
> > +		unregister_pernet_subsys(&ping_v6_net_ops);
> > +#endif
> > +	}
> > +	return ret;
> >   }
> >   
> >   /* This never gets called because it's not possible to unload the ipv6 module,
> > @@ -299,12 +318,7 @@ int __init pingv6_init(void)
> >    */
> >   void pingv6_exit(void)
> >   {
> > -	pingv6_ops.ipv6_recv_error = dummy_ipv6_recv_error;
> > -	pingv6_ops.ip6_datagram_recv_common_ctl = dummy_ip6_datagram_recv_ctl;
> > -	pingv6_ops.ip6_datagram_recv_specific_ctl = dummy_ip6_datagram_recv_ctl;
> > -	pingv6_ops.icmpv6_err_convert = dummy_icmpv6_err_convert;
> > -	pingv6_ops.ipv6_icmp_error = dummy_ipv6_icmp_error;
> > -	pingv6_ops.ipv6_chk_addr = dummy_ipv6_chk_addr;
> > +	pingv6_ops_unset();
> >   #ifdef CONFIG_PROC_FS
> >   	unregister_pernet_subsys(&ping_v6_net_ops);
> >   #endif
> > --
> > 2.17.1