| 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 |
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(<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); >
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(<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); >
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(<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);