[PATCHv2 net] ipv6: sr: fix invalid unregister error path

[PATCHv2 net] 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.

At the same time, add seg6_local_exit() and fix the genl unregister order
in seg6_exit().

Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
Reported-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: define label out_unregister_genl in CONFIG_IPV6_SEG6_LWTUNNEL(Sabrina Dubroca)
---
 net/ipv6/seg6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index 35508abd76f4..6a80d93399ce 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -551,8 +551,8 @@ int __init seg6_init(void)
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 out_unregister_genl:
-	genl_unregister_family(&seg6_genl_family);
 #endif
+	genl_unregister_family(&seg6_genl_family);
 out_unregister_pernet:
 	unregister_pernet_subsys(&ip6_segments_ops);
 	goto out;
@@ -564,8 +564,9 @@ 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);
 	genl_unregister_family(&seg6_genl_family);
+	unregister_pernet_subsys(&ip6_segments_ops);
 }
-- 
2.43.0

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Sabrina Dubroca]

2024-05-08, 10:55:02 +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.
> 
> At the same time, add seg6_local_exit() and fix the genl unregister order
> in seg6_exit().
> 
> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v2: define label out_unregister_genl in CONFIG_IPV6_SEG6_LWTUNNEL(Sabrina Dubroca)
> ---
>  net/ipv6/seg6.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> index 35508abd76f4..6a80d93399ce 100644
> --- a/net/ipv6/seg6.c
> +++ b/net/ipv6/seg6.c
> @@ -551,8 +551,8 @@ int __init seg6_init(void)
>  #endif
>  #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
>  out_unregister_genl:
> -	genl_unregister_family(&seg6_genl_family);
>  #endif
> +	genl_unregister_family(&seg6_genl_family);

Sorry, I didn't notice when you answered my comment yesterday, but
this will create unreachable code after return when
CONFIG_IPV6_SEG6_LWTUNNEL=n and CONFIG_IPV6_SEG6_HMAC=n:

out:
	return err;
	genl_unregister_family(&seg6_genl_family);
out_unregister_pernet:
	unregister_pernet_subsys(&ip6_segments_ops);
	goto out;


(stragely, gcc doesn't complain about it, I thought it would)


The only solution I can think of if we want to avoid it is ugly:

 #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:
 	unregister_pernet_subsys(&ip6_segments_ops);
 	goto out;

(on top of v2)

For all other cases your patch looks correct.

-- 
Sabrina

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Hangbin Liu]

On Wed, May 08, 2024 at 11:33:18AM +0200, Sabrina Dubroca wrote:
> > ---
> > v2: define label out_unregister_genl in CONFIG_IPV6_SEG6_LWTUNNEL(Sabrina Dubroca)
> > ---
> >  net/ipv6/seg6.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> > index 35508abd76f4..6a80d93399ce 100644
> > --- a/net/ipv6/seg6.c
> > +++ b/net/ipv6/seg6.c
> > @@ -551,8 +551,8 @@ int __init seg6_init(void)
> >  #endif
> >  #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
> >  out_unregister_genl:
> > -	genl_unregister_family(&seg6_genl_family);
> >  #endif
> > +	genl_unregister_family(&seg6_genl_family);
> 
> Sorry, I didn't notice when you answered my comment yesterday, but
> this will create unreachable code after return when
> CONFIG_IPV6_SEG6_LWTUNNEL=n and CONFIG_IPV6_SEG6_HMAC=n:

Oh.. Didn't notice this...

> 
> out:
> 	return err;
> 	genl_unregister_family(&seg6_genl_family);
> out_unregister_pernet:
> 	unregister_pernet_subsys(&ip6_segments_ops);
> 	goto out;
> 
> 
> (stragely, gcc doesn't complain about it, I thought it would)

Yes, I also complied the patch with not complain, so I just posted it.

> 
> 
> The only solution I can think of if we want to avoid it is ugly:
> 
>  #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:
>  	unregister_pernet_subsys(&ip6_segments_ops);
>  	goto out;
> 
> (on top of v2)
> 
> For all other cases your patch looks correct.

Thanks, I will check if there are any other workaround. If not, I will do
like what you said.

Hangbin

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Simon Horman]

On Wed, May 08, 2024 at 10:55:02AM +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.
> 
> At the same time, add seg6_local_exit() and fix the genl unregister order
> in seg6_exit().

It seems that this fixes two, or perhaps three different problems.
Perhaps we should consider two or three patches?

Also, could you explain the implications of changing the unregister order
in the patch description: it should describe why a change is made.

> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")

I agree that the current manifestation of the first problem
was introduced. But didn't a very similar problem exist before then?
I suspect the fixes tag should refer to an earlier commit.

> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

I think these bugs are pretty good examples of why not
to sprinkle #ifdef inside of functions - it makes the
logic hard to reason with.

So while I agree that a minimal fix, along the lines of this patch, is
suitable for 'net'. Could we consider, as a follow-up, refactoring the code
to remove this #ifdef spaghetti? F.e. by providing dummy implementations
of seg6_iptunnel_init()/seg6_iptunnel_exit() and so on.

-- 
pw-bot: changes-requested

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[kovalev]

Hi,

08.05.2024 12:40, Simon Horman wrote:
> On Wed, May 08, 2024 at 10:55:02AM +0800, Hangbin Liu wrote:

>> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> 
> I agree that the current manifestation of the first problem
> was introduced. But didn't a very similar problem exist before then?
> I suspect the fixes tag should refer to an earlier commit.
> 
Indeed, the invalid unregister error path was introduced by commit 
46738b1317e1 ("ipv6: sr: add option to control lwtunnel support"),
and the mentioned 5559cea2d5aa commit replaced one function with another 
in this error path.

-- 
Regards,
Vasiliy Kovalev

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Hangbin Liu]

On Wed, May 08, 2024 at 03:04:36PM +0300, kovalev@altlinux.org wrote:
> Hi,
> 
> 08.05.2024 12:40, Simon Horman wrote:
> > On Wed, May 08, 2024 at 10:55:02AM +0800, Hangbin Liu wrote:
> 
> > > Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> > 
> > I agree that the current manifestation of the first problem
> > was introduced. But didn't a very similar problem exist before then?
> > I suspect the fixes tag should refer to an earlier commit.
> > 
> Indeed, the invalid unregister error path was introduced by commit
> 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support"),
> and the mentioned 5559cea2d5aa commit replaced one function with another in
> this error path.

Thanks for your checking :)

Hangbin

Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Hangbin Liu]

On Wed, May 08, 2024 at 10:40:53AM +0100, Simon Horman wrote:
> On Wed, May 08, 2024 at 10:55:02AM +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.
> > 
> > At the same time, add seg6_local_exit() and fix the genl unregister order
> > in seg6_exit().
> 
> It seems that this fixes two, or perhaps three different problems.
> Perhaps we should consider two or three patches?

Yeah..
> 
> Also, could you explain the implications of changing the unregister order
> in the patch description: it should describe why a change is made.

Sure, I will.

> 
> > Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
> 
> I agree that the current manifestation of the first problem
> was introduced. But didn't a very similar problem exist before then?
> I suspect the fixes tag should refer to an earlier commit.

Yes, I will check previous commits.

> 
> > Reported-by: Guillaume Nault <gnault@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> I think these bugs are pretty good examples of why not
> to sprinkle #ifdef inside of functions - it makes the
> logic hard to reason with.
> 
> So while I agree that a minimal fix, along the lines of this patch, is
> suitable for 'net'. Could we consider, as a follow-up, refactoring the code
> to remove this #ifdef spaghetti? F.e. by providing dummy implementations
> of seg6_iptunnel_init()/seg6_iptunnel_exit() and so on.

Makes sense.

Thanks
Hangbin