| Message ID | 20240508025502.3928296-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2,net] ipv6: sr: fix invalid unregister error path | expand |
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.
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.
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.
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
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
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
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); }
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(-)