| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1205359220.6906.9.camel@w-sridhar2.beaverton.ibm.com>
Date: Wed, 12 Mar 2008 15:00:20 -0700
From: Sridhar Samudrala <sri@...ibm.com>
To: Vlad Yasevich <vladislav.yasevich@...com>
Cc: lksctp-developers <lksctp-developers@...ts.sourceforge.net>,
Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH] [SCTP]: Fix a race between module load and protosw
access
Vlad,
See my comments inline.
Thanks
Sridhar
On Wed, 2008-03-12 at 10:16 -0400, Vlad Yasevich wrote:
> [Can folks please review this. It solves the crashes I and others ]
> [have been seeing, but I want to make sure nothing else got goofed.]
>
> There is a race is SCTP between the loading of the module
> and the access by the socket layer to the protocol functions.
> In particular, a list of addresses that SCTP maintains is
> not initialized prior to the registration with the protosw.
> Thus it is possible for a user application to gain access
> to SCTP functions before everything has been initialized.
> The problem shows up as odd crashes during connection
> initialization when we try to access the SCTP address list.
>
> The solution is to re-factor how we do registration and
> initialize the lists prior to registering with the protosw.
> Care must be taken since the address list initialization
> depends on some other pieces of SCTP initialization. Also
> the clean-up in case of failure now also needs to be refactored.
>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@...com>
> ---
> include/net/sctp/sctp.h | 12 +++--
> net/sctp/ipv6.c | 32 ++++++++-----
> net/sctp/protocol.c | 117 ++++++++++++++++++++++++++++++----------------
> 3 files changed, 104 insertions(+), 57 deletions(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 57df27f..53aa5e1 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -380,15 +380,19 @@ static inline int sctp_sysctl_jiffies_ms(ctl_table *table, int __user *name, int
>
> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>
> -int sctp_v6_init(void);
> -void sctp_v6_exit(void);
> +int sctp_v6_pf_init(void);
> +void sctp_v6_pf_exit(void);
> +int sctp_v6_protosw_init(void);
> +void sctp_v6_protosw_exit(void);
> int sctp_v6_add_protocol(void);
> void sctp_v6_del_protocol(void);
>
> #else /* #ifdef defined(CONFIG_IPV6) */
>
> -static inline int sctp_v6_init(void) { return 0; }
> -static inline void sctp_v6_exit(void) { return; }
> +static inline int sctp_v6_pf_init(void) { return 0; }
> +static inline void sctp_v6_pf_exit(void) { return; }
> +static inline int sctp_v6_protosw_init(void) { return 0; }
> +static inline void sctp_v6_protosw_exit(void) { return; }
> static inline int sctp_v6_add_protocol(void) { return 0; }
> static inline void sctp_v6_del_protocol(void) { return; }
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 9aa0733..c7e56db 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -1014,15 +1014,24 @@ static struct sctp_pf sctp_pf_inet6 = {
> };
>
> /* Initialize IPv6 support and register with socket layer. */
> -int sctp_v6_init(void)
> +void sctp_v6_pf_init(void)
> {
> - int rc;
> -
> /* Register the SCTP specific PF_INET6 functions. */
> sctp_register_pf(&sctp_pf_inet6, PF_INET6);
>
> /* Register the SCTP specific AF_INET6 functions. */
> sctp_register_af(&sctp_af_inet6);
> +}
> +
> +void sctp_v6_pf_exit(void)
> +{
> + list_del(&sctp_ipv6_specific.list);
> +}
Shouldn't this be sctp_af_inet6? I think Neil's patch removed
sctp_ipv6_specific.
> +
> +/* Initialize IPv6 support and register with socket layer. */
> +int sctp_v6_protosw_init(void)
> +{
> + int rc;
>
> rc = proto_register(&sctpv6_prot, 1);
> if (rc)
> @@ -1035,6 +1044,14 @@ int sctp_v6_init(void)
> return 0;
> }
>
> +void sctp_v6_protosw_exit(void)
> +{
> + inet6_unregister_protosw(&sctpv6_seqpacket_protosw);
> + inet6_unregister_protosw(&sctpv6_stream_protosw);
> + proto_unregister(&sctpv6_prot);
> +}
> +
> +
> /* Register with inet6 layer. */
> int sctp_v6_add_protocol(void)
> {
> @@ -1047,15 +1064,6 @@ int sctp_v6_add_protocol(void)
> return 0;
> }
>
> -/* IPv6 specific exit support. */
> -void sctp_v6_exit(void)
> -{
> - inet6_unregister_protosw(&sctpv6_seqpacket_protosw);
> - inet6_unregister_protosw(&sctpv6_stream_protosw);
> - proto_unregister(&sctpv6_prot);
> - list_del(&sctp_af_inet6.list);
> -}
> -
> /* Unregister with inet6 layer. */
> void sctp_v6_del_protocol(void)
> {
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index ad0a406..62f271c 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -992,6 +992,54 @@ static void cleanup_sctp_mibs(void)
> free_percpu(sctp_statistics[1]);
> }
>
> +static void sctp_v4_pf_init(void)
> +{
> + /* Initialize the SCTP specific PF functions. */
> + sctp_register_pf(&sctp_pf_inet, PF_INET);
> + sctp_register_af(&sctp_ipv4_specific);
> +}
> +
> +static void sctp_v4_pf_exit(void)
> +{
> + list_del(&sctp_ipv4_specific.list);
> +}
Same here. should be sctp_af_inet. May be you are working with
a older kernel.
> +
> +static int sctp_v4_protosw_init(void)
> +{
> + int rc;
> +
> + rc = proto_register(&sctp_prot, 1);
> + if (rc)
> + return rc;
> +
> + /* Register SCTP(UDP and TCP style) with socket layer. */
> + inet_register_protosw(&sctp_seqpacket_protosw);
> + inet_register_protosw(&sctp_stream_protosw);
return missing here.
> +}
> +
> +static void sctp_v4_protosw_exit(void)
> +{
> + inet_unregister_protosw(&sctp_stream_protosw);
> + inet_unregister_protosw(&sctp_seqpacket_protosw);
> + proto_unregister(&sctp_prot);
> +}
> +
> +static int sctp_v4_add_protocol(void)
> +{
> + /* Register notifier for inet address additions/deletions. */
> + register_inetaddr_notifier(&sctp_inetaddr_notifier);
> +
> + /* Register SCTP with inet layer. */
> + if (inet_add_protocol(&sctp_protocol, IPPROTO_SCTP) < 0)
> + return -EAGAIN;
missing return
> +}
> +
> +static void sctp_v4_del_protocol(void)
> +{
> + inet_del_protocol(&sctp_protocol, IPPROTO_SCTP);
> + unregister_inetaddr_notifier(&sctp_inetaddr_notifier);
> +}
> +
> /* Initialize the universe into something sensible. */
> SCTP_STATIC __init int sctp_init(void)
> {
> @@ -1035,8 +1083,6 @@ SCTP_STATIC __init int sctp_init(void)
> /* Initialize object count debugging. */
> sctp_dbg_objcnt_init();
>
> - /* Initialize the SCTP specific PF functions. */
> - sctp_register_pf(&sctp_pf_inet, PF_INET);
> /*
> * 14. Suggested SCTP Protocol Parameter Values
> */
> @@ -1194,17 +1240,20 @@ SCTP_STATIC __init int sctp_init(void)
> sctp_sysctl_register();
>
> INIT_LIST_HEAD(&sctp_address_families);
> - sctp_register_af(&sctp_af_inet);
> + sctp_v4_pf_init();
> + sctp_v6_pf_init();
> +
> + /* Initialize the local address list. */
> + INIT_LIST_HEAD(&sctp_local_addr_list);
> + spin_lock_init(&sctp_local_addr_lock);
> + sctp_get_local_addr_list();
>
> - status = proto_register(&sctp_prot, 1);
> + status = sctp_v4_protosw_init();
> +
> if (status)
> goto err_proto_register;
You should change the goto label to match the new call.
>
> - /* Register SCTP(UDP and TCP style) with socket layer. */
> - inet_register_protosw(&sctp_seqpacket_protosw);
> - inet_register_protosw(&sctp_stream_protosw);
> -
> - status = sctp_v6_init();
> + status = sctp_v6_protosw_init();
> if (status)
> goto err_v6_init;
same here.
>
> @@ -1215,19 +1264,9 @@ SCTP_STATIC __init int sctp_init(void)
> goto err_ctl_sock_init;
> }
>
> - /* Initialize the local address list. */
> - INIT_LIST_HEAD(&sctp_local_addr_list);
> - spin_lock_init(&sctp_local_addr_lock);
> - sctp_get_local_addr_list();
> -
> - /* Register notifier for inet address additions/deletions. */
> - register_inetaddr_notifier(&sctp_inetaddr_notifier);
> -
> - /* Register SCTP with inet layer. */
> - if (inet_add_protocol(&sctp_protocol, IPPROTO_SCTP) < 0) {
> - status = -EAGAIN;
> + status = sctp_v4_add_protocol();
> + if (status)
> goto err_add_protocol;
> - }
>
> /* Register SCTP with inet6 layer. */
> status = sctp_v6_add_protocol();
> @@ -1238,18 +1277,18 @@ SCTP_STATIC __init int sctp_init(void)
> out:
> return status;
> err_v6_add_protocol:
> - inet_del_protocol(&sctp_protocol, IPPROTO_SCTP);
> - unregister_inetaddr_notifier(&sctp_inetaddr_notifier);
> + sctp_v6_del_protocol();
> err_add_protocol:
> - sctp_free_local_addr_list();
> + sctp_v4_del_protocol();
> sock_release(sctp_ctl_socket);
> err_ctl_sock_init:
> - sctp_v6_exit();
> + sctp_v6_protosw_exit();
> err_v6_init:
> - inet_unregister_protosw(&sctp_stream_protosw);
> - inet_unregister_protosw(&sctp_seqpacket_protosw);
> - proto_unregister(&sctp_prot);
> + sctp_v4_protosw_exit();
> err_proto_register:
> + sctp_free_local_addr_list();
> + sctp_v4_pf_exit();
> + sctp_v6_pf_exit();
> sctp_sysctl_unregister();
> list_del(&sctp_af_inet.list);
> free_pages((unsigned long)sctp_port_hashtable,
> @@ -1282,23 +1321,21 @@ SCTP_STATIC __exit void sctp_exit(void)
>
> /* Unregister with inet6/inet layers. */
> sctp_v6_del_protocol();
> - inet_del_protocol(&sctp_protocol, IPPROTO_SCTP);
> -
> - /* Unregister notifier for inet address additions/deletions. */
> - unregister_inetaddr_notifier(&sctp_inetaddr_notifier);
> -
> - /* Free the local address list. */
> - sctp_free_local_addr_list();
> + sctp_v4_del_protocol();
>
> /* Free the control endpoint. */
> sock_release(sctp_ctl_socket);
>
> - /* Cleanup v6 initializations. */
> - sctp_v6_exit();
> + /* Free protosw registrations */
> + sctp_v6_protosw_exit();
> + sctp_v4_protosw_exit();
> +
> + /* Free the local address list. */
> + sctp_free_local_addr_list();
>
> /* Unregister with socket layer. */
> - inet_unregister_protosw(&sctp_stream_protosw);
> - inet_unregister_protosw(&sctp_seqpacket_protosw);
> + sctp_v4_pf_exit();
> + sctp_v6_pf_exit();
To be symmetric, you could do v6 before v4 just like the
other cleanups.
>
> sctp_sysctl_unregister();
> list_del(&sctp_af_inet.list);
> @@ -1317,8 +1354,6 @@ SCTP_STATIC __exit void sctp_exit(void)
>
> kmem_cache_destroy(sctp_chunk_cachep);
> kmem_cache_destroy(sctp_bucket_cachep);
> -
> - proto_unregister(&sctp_prot);
> }
>
> module_init(sctp_init);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists