[PATCHv3 net 0/3] ipv6: sr: fix errors during unregister

[PATCHv3 net 0/3] ipv6: sr: fix errors during unregister

[Hangbin Liu]

Fix some errors in seg6 unregister path, like missing unregister functions,
incorrect unregister order, etc.

v3:
1) fix unreachable code when CONFIG_IPV6_SEG6_LWTUNNEL=n and
   CONFIG_IPV6_SEG6_HMAC=n (Sabrina Dubroca)
2) separate the patch into 3 small patches since they fix different
   commits (Simon Horman)

v2: define CONFIG_IPV6_SEG6_LWTUNNEL for label out_unregister_genl (Sabrina Dubroca)

Hangbin Liu (3):
  ipv6: sr: add missing seg6_local_exit
  ipv6: sr: fix incorrect unregister order
  ipv6: sr: fix invalid unregister error path

 net/ipv6/seg6.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCHv3 net 1/3] ipv6: sr: add missing seg6_local_exit

[Hangbin Liu]

Currently, we only call seg6_local_exit() in seg6_init() if
seg6_local_init() failed. But forgot to call it in seg6_exit().

Fixes: d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/seg6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index 35508abd76f4..5423f1f2aa62 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -564,6 +564,7 @@ void seg6_exit(void)
 	seg6_hmac_exit();
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
+	seg6_local_exit();
 	seg6_iptunnel_exit();
 #endif
 	unregister_pernet_subsys(&ip6_segments_ops);
-- 
2.43.0

[PATCHv3 net 2/3] ipv6: sr: fix incorrect unregister order

[Hangbin Liu]

Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and
null-ptr-deref") changed the register order in seg6_init(). But the
unregister order in seg6_exit() is not updated.

Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/seg6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index 5423f1f2aa62..c4ef96c8fdac 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -567,6 +567,6 @@ void seg6_exit(void)
 	seg6_local_exit();
 	seg6_iptunnel_exit();
 #endif
-	unregister_pernet_subsys(&ip6_segments_ops);
 	genl_unregister_family(&seg6_genl_family);
+	unregister_pernet_subsys(&ip6_segments_ops);
 }
-- 
2.43.0

[PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

[Hangbin Liu]

The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL
is not defined. In that case if seg6_hmac_init() fails, the
genl_unregister_family() isn't called.

This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control
lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible
use-after-free and null-ptr-deref") replaced unregister_pernet_subsys()
with genl_unregister_family() in this error path.

Fixes: 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")
Reported-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/seg6.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index c4ef96c8fdac..a31521e270f7 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -551,6 +551,8 @@ int __init seg6_init(void)
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 out_unregister_genl:
+#endif
+#if IS_ENABLED(CONFIG_IPV6_SEG6_LWTUNNEL) || IS_ENABLED(CONFIG_IPV6_SEG6_HMAC)
 	genl_unregister_family(&seg6_genl_family);
 #endif
 out_unregister_pernet:
-- 
2.43.0

Re: [PATCHv3 net 1/3] ipv6: sr: add missing seg6_local_exit

[Sabrina Dubroca]

2024-05-09, 21:18:10 +0800, Hangbin Liu wrote:
> Currently, we only call seg6_local_exit() in seg6_init() if
> seg6_local_init() failed. But forgot to call it in seg6_exit().
> 
> Fixes: d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCHv3 net 2/3] ipv6: sr: fix incorrect unregister order

[Sabrina Dubroca]

2024-05-09, 21:18:11 +0800, Hangbin Liu wrote:
> Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and
> null-ptr-deref") changed the register order in seg6_init(). But the
> unregister order in seg6_exit() is not updated.
> 
> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

[Sabrina Dubroca]

2024-05-09, 21:18:12 +0800, Hangbin Liu wrote:
> The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL
> is not defined. In that case if seg6_hmac_init() fails, the
> genl_unregister_family() isn't called.
> 
> This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control
> lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible
> use-after-free and null-ptr-deref") replaced unregister_pernet_subsys()
> with genl_unregister_family() in this error path.
> 
> Fixes: 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

seg6_hmac_init_algo also returns without cleaning up the previous
allocations if one fails, so it's going to leak all that memory and
the crypto tfms.

-- 
Sabrina

Re: [PATCHv3 net 1/3] ipv6: sr: add missing seg6_local_exit

[David Ahern]

On 5/9/24 7:18 AM, Hangbin Liu wrote:
> Currently, we only call seg6_local_exit() in seg6_init() if
> seg6_local_init() failed. But forgot to call it in seg6_exit().
> 
> Fixes: d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/seg6.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCHv3 net 2/3] ipv6: sr: fix incorrect unregister order

[David Ahern]

On 5/9/24 7:18 AM, Hangbin Liu wrote:
> Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and
> null-ptr-deref") changed the register order in seg6_init(). But the
> unregister order in seg6_exit() is not updated.
> 
> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/seg6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

[David Ahern]

On 5/9/24 7:18 AM, Hangbin Liu wrote:
> The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL
> is not defined. In that case if seg6_hmac_init() fails, the
> genl_unregister_family() isn't called.
> 
> This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control
> lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible
> use-after-free and null-ptr-deref") replaced unregister_pernet_subsys()
> with genl_unregister_family() in this error path.
> 
> Fixes: 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/seg6.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> index c4ef96c8fdac..a31521e270f7 100644
> --- a/net/ipv6/seg6.c
> +++ b/net/ipv6/seg6.c
> @@ -551,6 +551,8 @@ int __init seg6_init(void)
>  #endif
>  #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
>  out_unregister_genl:
> +#endif
> +#if IS_ENABLED(CONFIG_IPV6_SEG6_LWTUNNEL) || IS_ENABLED(CONFIG_IPV6_SEG6_HMAC)
>  	genl_unregister_family(&seg6_genl_family);
>  #endif
>  out_unregister_pernet:

a good example of why ifdef's create problems. It would have been
simpler if all of those init functions were defined for both cases and
this function does not need the '#if' spaghetti.

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCHv3 net 0/3] ipv6: sr: fix errors during unregister

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  9 May 2024 21:18:09 +0800 you wrote:
> Fix some errors in seg6 unregister path, like missing unregister functions,
> incorrect unregister order, etc.
> 
> v3:
> 1) fix unreachable code when CONFIG_IPV6_SEG6_LWTUNNEL=n and
>    CONFIG_IPV6_SEG6_HMAC=n (Sabrina Dubroca)
> 2) separate the patch into 3 small patches since they fix different
>    commits (Simon Horman)
> 
> [...]

Here is the summary with links:
  - [PATCHv3,net,1/3] ipv6: sr: add missing seg6_local_exit
    https://git.kernel.org/netdev/net/c/3321687e3213
  - [PATCHv3,net,2/3] ipv6: sr: fix incorrect unregister order
    https://git.kernel.org/netdev/net/c/6e370a771d29
  - [PATCHv3,net,3/3] ipv6: sr: fix invalid unregister error path
    https://git.kernel.org/netdev/net/c/160e9d275218

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

[Hangbin Liu]

On Fri, May 10, 2024 at 07:52:36PM -0600, David Ahern wrote:
> a good example of why ifdef's create problems. It would have been
> simpler if all of those init functions were defined for both cases and
> this function does not need the '#if' spaghetti.

Yes, I will post a restruct patch for net-next.

Thanks
Hangbin