diff mbox series

[v2] appletalk: Correctly check return value of register_snap_client

Message ID 20190307022216.22464-1-yuehaibing@huawei.com
State Not Applicable
Delegated to: David Miller
Headers show
Series [v2] appletalk: Correctly check return value of register_snap_client | expand

Commit Message

YueHaibing March 7, 2019, 2:22 a.m. UTC
From: YueHaibing <yuehaibing@huawei.com>

register_snap_client may return NULL, all the callers
check it, but only print a warning. This will result in
NULL pointer dereference in unregister_snap_client and other
places.

It has always been used like this since v2.6

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v2: fix aarp_dl leak
---
 include/linux/atalk.h |  2 +-
 net/appletalk/aarp.c  | 13 ++++++++++---
 net/appletalk/ddp.c   | 20 ++++++++++++--------
 3 files changed, 23 insertions(+), 12 deletions(-)

Comments

YueHaibing March 9, 2019, 1:34 a.m. UTC | #1
ping.

On 2019/3/7 10:22, Yue Haibing wrote:
> From: YueHaibing <yuehaibing@huawei.com>
> 
> register_snap_client may return NULL, all the callers
> check it, but only print a warning. This will result in
> NULL pointer dereference in unregister_snap_client and other
> places.
> 
> It has always been used like this since v2.6
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> v2: fix aarp_dl leak
> ---
>  include/linux/atalk.h |  2 +-
>  net/appletalk/aarp.c  | 13 ++++++++++---
>  net/appletalk/ddp.c   | 20 ++++++++++++--------
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/atalk.h b/include/linux/atalk.h
> index 5a90f28..0e5265a 100644
> --- a/include/linux/atalk.h
> +++ b/include/linux/atalk.h
> @@ -108,7 +108,7 @@ static __inline__ struct elapaarp *aarp_hdr(struct sk_buff *skb)
>  #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 */
>  
> diff --git a/net/appletalk/aarp.c b/net/appletalk/aarp.c
> index 49a16ce..d11372d 100644
> --- a/net/appletalk/aarp.c
> +++ b/net/appletalk/aarp.c
> @@ -879,15 +879,22 @@ static struct notifier_block aarp_notifier = {
>  
>  static unsigned char aarp_snap_id[] = { 0x00, 0x00, 0x00, 0x80, 0xF3 };
>  
> -void __init aarp_proto_init(void)
> +int __init aarp_proto_init(void)
>  {
> +	int rc;
> +
>  	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");
> +		return -ENOMEM;
> +	}
>  	timer_setup(&aarp_timer, aarp_expire_timeout, 0);
>  	aarp_timer.expires  = jiffies + sysctl_aarp_expiry_time;
>  	add_timer(&aarp_timer);
> -	register_netdevice_notifier(&aarp_notifier);
> +	rc = register_netdevice_notifier(&aarp_notifier);
> +	if (rc)
> +		unregister_snap_client(aarp_dl);
> +	return rc;
>  }
>  
>  /* Remove the AARP entries associated with a device. */
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 795fbc6..709d254 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -1904,9 +1904,6 @@ static unsigned char ddp_snap_id[] = { 0x08, 0x00, 0x07, 0x80, 0x9B };
>  EXPORT_SYMBOL(atrtr_get_dev);
>  EXPORT_SYMBOL(atalk_find_dev_addr);
>  
> -static const char atalk_err_snap[] __initconst =
> -	KERN_CRIT "Unable to register DDP with SNAP.\n";
> -
>  /* Called by proto.c on kernel start up */
>  static int __init atalk_init(void)
>  {
> @@ -1921,17 +1918,22 @@ static int __init atalk_init(void)
>  		goto out_proto;
>  
>  	ddp_dl = register_snap_client(ddp_snap_id, atalk_rcv);
> -	if (!ddp_dl)
> -		printk(atalk_err_snap);
> +	if (!ddp_dl) {
> +		pr_crit("Unable to register DDP with SNAP.\n");
> +		goto out_sock;
> +	}
>  
>  	dev_add_pack(&ltalk_packet_type);
>  	dev_add_pack(&ppptalk_packet_type);
>  
>  	rc = register_netdevice_notifier(&ddp_notifier);
>  	if (rc)
> -		goto out_sock;
> +		goto out_snap;
> +
> +	rc = aarp_proto_init();
> +	if (rc)
> +		goto out_dev;
>  
> -	aarp_proto_init();
>  	rc = atalk_proc_init();
>  	if (rc)
>  		goto out_aarp;
> @@ -1945,11 +1947,13 @@ static int __init atalk_init(void)
>  	atalk_proc_exit();
>  out_aarp:
>  	aarp_cleanup_module();
> +out_dev:
>  	unregister_netdevice_notifier(&ddp_notifier);
> -out_sock:
> +out_snap:
>  	dev_remove_pack(&ppptalk_packet_type);
>  	dev_remove_pack(&ltalk_packet_type);
>  	unregister_snap_client(ddp_dl);
> +out_sock:
>  	sock_unregister(PF_APPLETALK);
>  out_proto:
>  	proto_unregister(&ddp_proto);
>
YueHaibing March 11, 2019, 1:45 a.m. UTC | #2
Hi David,

Is there something need do for this patch? Pls let me know.

I saw the patchwork status labled to  'Not Applicable'

https://patchwork.ozlabs.org/patch/1052624/


On 2019/3/7 10:22, Yue Haibing wrote:
> From: YueHaibing <yuehaibing@huawei.com>
> 
> register_snap_client may return NULL, all the callers
> check it, but only print a warning. This will result in
> NULL pointer dereference in unregister_snap_client and other
> places.
> 
> It has always been used like this since v2.6
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> v2: fix aarp_dl leak
> ---
>  include/linux/atalk.h |  2 +-
>  net/appletalk/aarp.c  | 13 ++++++++++---
>  net/appletalk/ddp.c   | 20 ++++++++++++--------
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/atalk.h b/include/linux/atalk.h
> index 5a90f28..0e5265a 100644
> --- a/include/linux/atalk.h
> +++ b/include/linux/atalk.h
> @@ -108,7 +108,7 @@ static __inline__ struct elapaarp *aarp_hdr(struct sk_buff *skb)
>  #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 */
>  
> diff --git a/net/appletalk/aarp.c b/net/appletalk/aarp.c
> index 49a16ce..d11372d 100644
> --- a/net/appletalk/aarp.c
> +++ b/net/appletalk/aarp.c
> @@ -879,15 +879,22 @@ static struct notifier_block aarp_notifier = {
>  
>  static unsigned char aarp_snap_id[] = { 0x00, 0x00, 0x00, 0x80, 0xF3 };
>  
> -void __init aarp_proto_init(void)
> +int __init aarp_proto_init(void)
>  {
> +	int rc;
> +
>  	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");
> +		return -ENOMEM;
> +	}
>  	timer_setup(&aarp_timer, aarp_expire_timeout, 0);
>  	aarp_timer.expires  = jiffies + sysctl_aarp_expiry_time;
>  	add_timer(&aarp_timer);
> -	register_netdevice_notifier(&aarp_notifier);
> +	rc = register_netdevice_notifier(&aarp_notifier);
> +	if (rc)
> +		unregister_snap_client(aarp_dl);
> +	return rc;
>  }
>  
>  /* Remove the AARP entries associated with a device. */
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 795fbc6..709d254 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -1904,9 +1904,6 @@ static unsigned char ddp_snap_id[] = { 0x08, 0x00, 0x07, 0x80, 0x9B };
>  EXPORT_SYMBOL(atrtr_get_dev);
>  EXPORT_SYMBOL(atalk_find_dev_addr);
>  
> -static const char atalk_err_snap[] __initconst =
> -	KERN_CRIT "Unable to register DDP with SNAP.\n";
> -
>  /* Called by proto.c on kernel start up */
>  static int __init atalk_init(void)
>  {
> @@ -1921,17 +1918,22 @@ static int __init atalk_init(void)
>  		goto out_proto;
>  
>  	ddp_dl = register_snap_client(ddp_snap_id, atalk_rcv);
> -	if (!ddp_dl)
> -		printk(atalk_err_snap);
> +	if (!ddp_dl) {
> +		pr_crit("Unable to register DDP with SNAP.\n");
> +		goto out_sock;
> +	}
>  
>  	dev_add_pack(&ltalk_packet_type);
>  	dev_add_pack(&ppptalk_packet_type);
>  
>  	rc = register_netdevice_notifier(&ddp_notifier);
>  	if (rc)
> -		goto out_sock;
> +		goto out_snap;
> +
> +	rc = aarp_proto_init();
> +	if (rc)
> +		goto out_dev;
>  
> -	aarp_proto_init();
>  	rc = atalk_proc_init();
>  	if (rc)
>  		goto out_aarp;
> @@ -1945,11 +1947,13 @@ static int __init atalk_init(void)
>  	atalk_proc_exit();
>  out_aarp:
>  	aarp_cleanup_module();
> +out_dev:
>  	unregister_netdevice_notifier(&ddp_notifier);
> -out_sock:
> +out_snap:
>  	dev_remove_pack(&ppptalk_packet_type);
>  	dev_remove_pack(&ltalk_packet_type);
>  	unregister_snap_client(ddp_dl);
> +out_sock:
>  	sock_unregister(PF_APPLETALK);
>  out_proto:
>  	proto_unregister(&ddp_proto);
>
diff mbox series

Patch

diff --git a/include/linux/atalk.h b/include/linux/atalk.h
index 5a90f28..0e5265a 100644
--- a/include/linux/atalk.h
+++ b/include/linux/atalk.h
@@ -108,7 +108,7 @@  static __inline__ struct elapaarp *aarp_hdr(struct sk_buff *skb)
 #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 */
 
diff --git a/net/appletalk/aarp.c b/net/appletalk/aarp.c
index 49a16ce..d11372d 100644
--- a/net/appletalk/aarp.c
+++ b/net/appletalk/aarp.c
@@ -879,15 +879,22 @@  static struct notifier_block aarp_notifier = {
 
 static unsigned char aarp_snap_id[] = { 0x00, 0x00, 0x00, 0x80, 0xF3 };
 
-void __init aarp_proto_init(void)
+int __init aarp_proto_init(void)
 {
+	int rc;
+
 	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");
+		return -ENOMEM;
+	}
 	timer_setup(&aarp_timer, aarp_expire_timeout, 0);
 	aarp_timer.expires  = jiffies + sysctl_aarp_expiry_time;
 	add_timer(&aarp_timer);
-	register_netdevice_notifier(&aarp_notifier);
+	rc = register_netdevice_notifier(&aarp_notifier);
+	if (rc)
+		unregister_snap_client(aarp_dl);
+	return rc;
 }
 
 /* Remove the AARP entries associated with a device. */
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 795fbc6..709d254 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1904,9 +1904,6 @@  static unsigned char ddp_snap_id[] = { 0x08, 0x00, 0x07, 0x80, 0x9B };
 EXPORT_SYMBOL(atrtr_get_dev);
 EXPORT_SYMBOL(atalk_find_dev_addr);
 
-static const char atalk_err_snap[] __initconst =
-	KERN_CRIT "Unable to register DDP with SNAP.\n";
-
 /* Called by proto.c on kernel start up */
 static int __init atalk_init(void)
 {
@@ -1921,17 +1918,22 @@  static int __init atalk_init(void)
 		goto out_proto;
 
 	ddp_dl = register_snap_client(ddp_snap_id, atalk_rcv);
-	if (!ddp_dl)
-		printk(atalk_err_snap);
+	if (!ddp_dl) {
+		pr_crit("Unable to register DDP with SNAP.\n");
+		goto out_sock;
+	}
 
 	dev_add_pack(&ltalk_packet_type);
 	dev_add_pack(&ppptalk_packet_type);
 
 	rc = register_netdevice_notifier(&ddp_notifier);
 	if (rc)
-		goto out_sock;
+		goto out_snap;
+
+	rc = aarp_proto_init();
+	if (rc)
+		goto out_dev;
 
-	aarp_proto_init();
 	rc = atalk_proc_init();
 	if (rc)
 		goto out_aarp;
@@ -1945,11 +1947,13 @@  static int __init atalk_init(void)
 	atalk_proc_exit();
 out_aarp:
 	aarp_cleanup_module();
+out_dev:
 	unregister_netdevice_notifier(&ddp_notifier);
-out_sock:
+out_snap:
 	dev_remove_pack(&ppptalk_packet_type);
 	dev_remove_pack(&ltalk_packet_type);
 	unregister_snap_client(ddp_dl);
+out_sock:
 	sock_unregister(PF_APPLETALK);
 out_proto:
 	proto_unregister(&ddp_proto);