* [PATCH] appletalk: Correctly handle return value of register_snap_client
@ 2019-03-06 7:27 Yue Haibing
2019-03-06 18:17 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Yue Haibing @ 2019-03-06 7:27 UTC (permalink / raw)
To: davem, joe, gregkh; +Cc: linux-kernel, netdev, YueHaibing
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>
---
include/linux/atalk.h | 2 +-
net/appletalk/aarp.c | 10 ++++++----
net/appletalk/ddp.c | 20 ++++++++++++--------
3 files changed, 19 insertions(+), 13 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..e047853 100644
--- a/net/appletalk/aarp.c
+++ b/net/appletalk/aarp.c
@@ -879,15 +879,17 @@ 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)
{
aarp_dl = register_snap_client(aarp_snap_id, aarp_rcv);
- if (!aarp_dl)
- printk(KERN_CRIT "Unable to register AARP with SNAP.\n");
+ if (!aarp_dl) {
+ pr_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);
+ return register_netdevice_notifier(&aarp_notifier);
}
/* 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(<alk_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(<alk_packet_type);
unregister_snap_client(ddp_dl);
+out_sock:
sock_unregister(PF_APPLETALK);
out_proto:
proto_unregister(&ddp_proto);
--
2.7.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] appletalk: Correctly handle return value of register_snap_client
2019-03-06 7:27 [PATCH] appletalk: Correctly handle return value of register_snap_client Yue Haibing
@ 2019-03-06 18:17 ` David Miller
2019-03-07 2:09 ` YueHaibing
0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2019-03-06 18:17 UTC (permalink / raw)
To: yuehaibing; +Cc: joe, gregkh, linux-kernel, netdev
From: Yue Haibing <yuehaibing@huawei.com>
Date: Wed, 6 Mar 2019 15:27:40 +0800
> @@ -879,15 +879,17 @@ 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)
> {
> aarp_dl = register_snap_client(aarp_snap_id, aarp_rcv);
> - if (!aarp_dl)
> - printk(KERN_CRIT "Unable to register AARP with SNAP.\n");
> + if (!aarp_dl) {
> + pr_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);
> + return register_netdevice_notifier(&aarp_notifier);
> }
>
Your error paths in the caller of aarp_proto_init() do not handle the case
where aarp_dl is created by register_netdevice_notifier() fails. You have
to unregister aarp_dl if it is non-NULL.
So your out_dev label path needs to handle aarp_dl if non-NULL.
Probably best is to jump to out_aarp: instead and make aarp_cleanup_module
able to handle partial cleanups.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] appletalk: Correctly handle return value of register_snap_client
2019-03-06 18:17 ` David Miller
@ 2019-03-07 2:09 ` YueHaibing
0 siblings, 0 replies; 3+ messages in thread
From: YueHaibing @ 2019-03-07 2:09 UTC (permalink / raw)
To: David Miller; +Cc: joe, gregkh, linux-kernel, netdev
On 2019/3/7 2:17, David Miller wrote:
> From: Yue Haibing <yuehaibing@huawei.com>
> Date: Wed, 6 Mar 2019 15:27:40 +0800
>
>> @@ -879,15 +879,17 @@ 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)
>> {
>> aarp_dl = register_snap_client(aarp_snap_id, aarp_rcv);
>> - if (!aarp_dl)
>> - printk(KERN_CRIT "Unable to register AARP with SNAP.\n");
>> + if (!aarp_dl) {
>> + pr_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);
>> + return register_netdevice_notifier(&aarp_notifier);
>> }
>>
>
> Your error paths in the caller of aarp_proto_init() do not handle the case
> where aarp_dl is created by register_netdevice_notifier() fails. You have
> to unregister aarp_dl if it is non-NULL.
Thanks, will fix this.
>
> So your out_dev label path needs to handle aarp_dl if non-NULL.
>
> Probably best is to jump to out_aarp: instead and make aarp_cleanup_module
> able to handle partial cleanups.
>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-07 2:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-06 7:27 [PATCH] appletalk: Correctly handle return value of register_snap_client Yue Haibing
2019-03-06 18:17 ` David Miller
2019-03-07 2:09 ` YueHaibing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).