[PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[Akinobu Mita]

If register_sysctl_table() fails during module initalization,
NULL pointer dereference will happen in the module cleanup.

Cc: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 net/appletalk/sysctl_net_atalk.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: work-fault-inject/net/appletalk/sysctl_net_atalk.c
===================================================================
--- work-fault-inject.orig/net/appletalk/sysctl_net_atalk.c
+++ work-fault-inject/net/appletalk/sysctl_net_atalk.c
@@ -78,5 +78,6 @@ void atalk_register_sysctl(void)
 
 void atalk_unregister_sysctl(void)
 {
-	unregister_sysctl_table(atalk_table_header);
+	if (atalk_table_header)
+		unregister_sysctl_table(atalk_table_header);
 }

Re: [PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[David Rientjes]

On Tue, 24 Oct 2006, Akinobu Mita wrote:

> If register_sysctl_table() fails during module initalization,
> NULL pointer dereference will happen in the module cleanup.
> 

The only way this would happen at atalk_unregister_sysctl is if the 
kmalloc failed on register_sysctl_table during init.  In that case there 
is no need to unregister atalk in the first place since it never came up, 
so this doesn't appear to be the correct fix.  Even if it were possible, 
this check should be done at atalk_exit instead of 
atalk_unregister_sysctl.

		David

Re: [PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[Akinobu Mita]

On Tue, Oct 24, 2006 at 02:38:24AM -0700, David Rientjes wrote:

> The only way this would happen at atalk_unregister_sysctl is if the 
> kmalloc failed on register_sysctl_table during init.  In that case there 
> is no need to unregister atalk in the first place since it never came up, 

Yes. this patch doesn't cause failure if sysctl registration failed.
It aims to avoid that minor possible NULL pointer dereference.

> so this doesn't appear to be the correct fix.  Even if it were possible, 
> this check should be done at atalk_exit instead of 
> atalk_unregister_sysctl.

Are there any difference?
Because atalk_unregister_sysctl() is only called from atalk_exit(). And
atalk_table_header is static variable. So there is no way to know
whether sysclt registration was succeeded or not. Or is it better to
export atalk_table_header for that check from atalk_exit()?

Re: [PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[Alexey Dobriyan]

On Tue, Oct 24, 2006 at 05:53:57PM +0900, Akinobu Mita wrote:
> If register_sysctl_table() fails during module initalization,
> NULL pointer dereference will happen in the module cleanup.

> --- work-fault-inject.orig/net/appletalk/sysctl_net_atalk.c
> +++ work-fault-inject/net/appletalk/sysctl_net_atalk.c
> @@ -78,5 +78,6 @@ void atalk_register_sysctl(void)
>
>  void atalk_unregister_sysctl(void)
>  {
> -	unregister_sysctl_table(atalk_table_header);
> +	if (atalk_table_header)
> +		unregister_sysctl_table(atalk_table_header);

Make sure that module won't load if sysctl table can't be registered,
instead.

Re: [PATCH] appletalk: prevent unregister_sysctl_table() with a NULL argument

[David Rientjes]

On Tue, 24 Oct 2006, Akinobu Mita wrote:

> On Tue, Oct 24, 2006 at 02:38:24AM -0700, David Rientjes wrote:
> 
> > The only way this would happen at atalk_unregister_sysctl is if the 
> > kmalloc failed on register_sysctl_table during init.  In that case there 
> > is no need to unregister atalk in the first place since it never came up, 
> 
> Yes. this patch doesn't cause failure if sysctl registration failed.
> It aims to avoid that minor possible NULL pointer dereference.
> 

That dereference should never be possible.  If sysctl registration fails, 
it should not be left partially initialized so that it would ever need to 
be cleaned up later; it should just fail to register.  So the fix, if 
indeed one is required in this instance that you have witnessed, should be 
an immediate response to an -ENOMEM on register_sysctl_table.  Adding this 
to atalk_unregister_sysctl is incorrect because that function should only 
be entered given the condition that the register was successful, which in 
this case it was not.

		David

[PATCH] appletalk: handle errors during module_init

[Akinobu Mita]

On Tue, Oct 24, 2006 at 02:27:11PM +0400, Alexey Dobriyan wrote:

> Make sure that module won't load if sysctl table can't be registered,
> instead.

I fixed the patch to do so and handle another errors, too.

Subject: [PATCH] appletalk: handle errors during module_init

This patch makes aarp_proto_init() and atalk_register_sysctl()
return error value to catch ENOMEM errors from module init call.
Then it handles several errors in module_init and makes happen fail.

Also unnessesary SYSCTL ifdef in module_cleanup was removed.

Cc: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

 include/linux/atalk.h            |    6 ++---
 net/appletalk/aarp.c             |   12 ++++++----
 net/appletalk/ddp.c              |   45 ++++++++++++++++++++++++++++++++-------
 net/appletalk/sysctl_net_atalk.c |    3 +-
 4 files changed, 49 insertions(+), 17 deletions(-)

Index: work-fault-inject/net/appletalk/sysctl_net_atalk.c
===================================================================
--- work-fault-inject.orig/net/appletalk/sysctl_net_atalk.c
+++ work-fault-inject/net/appletalk/sysctl_net_atalk.c
@@ -71,9 +71,10 @@ static struct ctl_table atalk_root_table
 
 static struct ctl_table_header *atalk_table_header;
 
-void atalk_register_sysctl(void)
+int atalk_register_sysctl(void)
 {
 	atalk_table_header = register_sysctl_table(atalk_root_table, 1);
+	return (atalk_table_header == NULL) ? -ENOMEM : 0;
 }
 
 void atalk_unregister_sysctl(void)
Index: work-fault-inject/net/appletalk/ddp.c
===================================================================
--- work-fault-inject.orig/net/appletalk/ddp.c
+++ work-fault-inject/net/appletalk/ddp.c
@@ -1871,21 +1871,52 @@ static int __init atalk_init(void)
 {
 	int rc = proto_register(&ddp_proto, 0);
 
-	if (rc != 0)
+	if (rc)
 		goto out;
 
-	(void)sock_register(&atalk_family_ops);
+	rc = sock_register(&atalk_family_ops);
+	if (rc)
+		goto out1;
+
 	ddp_dl = register_snap_client(ddp_snap_id, atalk_rcv);
-	if (!ddp_dl)
+	if (!ddp_dl) {
 		printk(atalk_err_snap);
+		rc = -ENOMEM;
+		goto out2;
+	}
 
 	dev_add_pack(&ltalk_packet_type);
 	dev_add_pack(&ppptalk_packet_type);
 
 	register_netdevice_notifier(&ddp_notifier);
-	aarp_proto_init();
-	atalk_proc_init();
-	atalk_register_sysctl();
+
+	rc = aarp_proto_init();
+	if (rc)
+		goto out3;
+
+	rc = atalk_proc_init();
+	if (rc)
+		goto out4;
+
+	rc = atalk_register_sysctl();
+	if (rc)
+		goto out5;
+
+	return 0;
+
+out5:
+	atalk_proc_exit();
+out4:
+	aarp_cleanup_module();	/* General aarp clean-up. */
+out3:
+	unregister_netdevice_notifier(&ddp_notifier);
+	dev_remove_pack(&ltalk_packet_type);
+	dev_remove_pack(&ppptalk_packet_type);
+	unregister_snap_client(ddp_dl);
+out2:
+	sock_unregister(PF_APPLETALK);
+out1:
+	proto_unregister(&ddp_proto);
 out:
 	return rc;
 }
@@ -1902,9 +1933,7 @@ module_init(atalk_init);
  */
 static void __exit atalk_exit(void)
 {
-#ifdef CONFIG_SYSCTL
 	atalk_unregister_sysctl();
-#endif /* CONFIG_SYSCTL */
 	atalk_proc_exit();
 	aarp_cleanup_module();	/* General aarp clean-up. */
 	unregister_netdevice_notifier(&ddp_notifier);
Index: work-fault-inject/include/linux/atalk.h
===================================================================
--- work-fault-inject.orig/include/linux/atalk.h
+++ work-fault-inject/include/linux/atalk.h
@@ -147,7 +147,7 @@ static __inline__ struct elapaarp *aarp_
 #define AARP_RESOLVE_TIME	(10 * HZ)
 
 extern struct datalink_proto *ddp_dl, *aarp_dl;
-extern void aarp_proto_init(void);
+extern int aarp_proto_init(void);
 
 /* Inter module exports */
 
@@ -190,10 +190,10 @@ extern int sysctl_aarp_retransmit_limit;
 extern int sysctl_aarp_resolve_time;
 
 #ifdef CONFIG_SYSCTL
-extern void atalk_register_sysctl(void);
+extern int atalk_register_sysctl(void);
 extern void atalk_unregister_sysctl(void);
 #else
-#define atalk_register_sysctl()		do { } while(0)
+#define atalk_register_sysctl()		({ 0; })
 #define atalk_unregister_sysctl()	do { } while(0)
 #endif
 
Index: work-fault-inject/net/appletalk/aarp.c
===================================================================
--- work-fault-inject.orig/net/appletalk/aarp.c
+++ work-fault-inject/net/appletalk/aarp.c
@@ -858,17 +858,19 @@ static struct notifier_block aarp_notifi
 
 static unsigned char aarp_snap_id[] = { 0x00, 0x00, 0x00, 0x80, 0xF3 };
 
-void __init aarp_proto_init(void)
+int __init aarp_proto_init(void)
 {
 	aarp_dl = register_snap_client(aarp_snap_id, aarp_rcv);
-	if (!aarp_dl)
+	if (!aarp_dl) {
 		printk(KERN_CRIT "Unable to register AARP with SNAP.\n");
-	init_timer(&aarp_timer);
-	aarp_timer.function = aarp_expire_timeout;
-	aarp_timer.data	    = 0;
+		return -ENOMEM;
+	}
+	setup_timer(&aarp_timer, aarp_expire_timeout, 0);
 	aarp_timer.expires  = jiffies + sysctl_aarp_expiry_time;
 	add_timer(&aarp_timer);
 	register_netdevice_notifier(&aarp_notifier);
+
+	return 0;
 }
 
 /* Remove the AARP entries associated with a device. */