net: caif: fix 2 memory leaks
Subsystems (3)
cover net: caif: fix 2 memory leaks
This patch series fix 2 memory leaks in caif
interface.
Syzbot reported memory leak in cfserl_create(). The problem was in cfcnfg_add_phy_layer() function. This function accepts struct cflayer *link_support and assign it to corresponting structures, but it can fail in some cases.
These cases must be handled to prevent leaking allocated struct cflayer *link_support pointer, because if error accured before assigning link_support pointer to somewhere, this pointer must be freed.
Fail log:
[ 49.051872][ T7010] caif:cfcnfg_add_phy_layer(): Too many CAIF Link Layers (max 6) [ 49.110236][ T7042] caif:cfcnfg_add_phy_layer(): Too many CAIF Link Layers (max 6) [ 49.134936][ T7045] caif:cfcnfg_add_phy_layer(): Too many CAIF Link Layers (max 6) [ 49.163083][ T7043] caif:cfcnfg_add_phy_layer(): Too many CAIF Link Layers (max 6) [ 55.248950][ T6994] kmemleak: 4 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
int cfcnfg_add_phy_layer(..., struct cflayer *link_support, ...) {
...
/* CAIF protocol allow maximum 6 link-layers */
for (i = 0; i < 7; i++) {
phyid = (dev->ifindex + i) & 0x7;
if (phyid == 0)
continue;
if (cfcnfg_get_phyinfo_rcu(cnfg, phyid) == NULL)
goto got_phyid;
}
pr_warn("Too many CAIF Link Layers (max 6)\n");
goto out;
...
if (link_support != NULL) {
link_support->id = phyid;
layer_set_dn(frml, link_support);
layer_set_up(link_support, frml);
layer_set_dn(link_support, phy_layer);
layer_set_up(phy_layer, link_support);
}...
}
As you can see, if cfcnfg_add_phy_layer fails before layer_set_*, link_support becomes leaked.
So, in this series, I made cfcnfg_add_phy_layer() return an int and added error handling code to prevent leaking link_support pointer in caif_device_notify() and cfusbl_device_notify() functions.
NOTE: this series was tested by syzbot
https://syzkaller.appspot.com/bug?id=62bc71b5fa73349e2e6b6280eca9c9615ddeb585)
Pavel Skripkin (4):
net: caif: added cfserl_release function
net: caif: add proper error handling
net: caif: fix memory leak in caif_device_notify
net: caif: fix memory leak in cfusbl_device_notify
include/net/caif/caif_dev.h | 2 +- include/net/caif/cfcnfg.h | 2 +- include/net/caif/cfserl.h | 1 + net/caif/caif_dev.c | 13 +++++++++---- net/caif/caif_usb.c | 14 +++++++++++++- net/caif/cfcnfg.c | 16 +++++++++++----- net/caif/cfserl.c | 5 +++++ 7 files changed, 41 insertions(+), 12 deletions(-)
Signature · 1 line
2.31.1
PATCH 1/4 net: caif: added cfserl_release function
Added cfserl_release() function.
--- include/net/caif/cfserl.h | 1 + net/caif/cfserl.c | 5 +++++ 2 files changed, 6 insertions(+)
include/net/caif/cfserl.h +1 -0
@@ -9,4 +9,5 @@#include <net/caif/caif_layer.h> struct cflayer *cfserl_create (int instance,bool use_stx); +void cfserl_release (struct cflayer *layer);#endif
net/caif/cfserl.c +5 -0
@@ -31,6 +31,11 @@ static int cfserl_transmit(struct cflayer *layr, struct cfpkt *pkt);static void cfserl_ctrlcmd (struct cflayer *layr,enum caif_ctrlcmd ctrl,int phyid); +void cfserl_release (struct cflayer *layer) +{ +kfree (layer); +} +struct cflayer *cfserl_create (int instance,bool use_stx) {struct cfserl *this =kzalloc (sizeof (struct cfserl ),GFP_ATOMIC );
Signature · 1 line
2.31.1
PATCH 2/4 net: caif: add proper error handling
In reply to Pavel Skripkincaif_enroll_dev() can fail in some cases. Ingnoring these cases can lead to memory leak due to not assigning link_support pointer to anywhere.
--- include/net/caif/caif_dev.h | 2 +- include/net/caif/cfcnfg.h | 2 +- net/caif/caif_dev.c | 8 +++++--- net/caif/cfcnfg.c | 16 +++++++++++----- 4 files changed, 18 insertions(+), 10 deletions(-)
include/net/caif/caif_dev.h +1 -1
@@ -119,7 +119,7 @@ void caif_free_client(struct cflayer *adap_layer);* The link_support layer is used to add any Link Layer specific * framing. */ -void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev, +int caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev, struct cflayer *link_support, int head_room, struct cflayer **layer, int (**rcv_func)( struct sk_buff *, struct net_device *,
include/net/caif/cfcnfg.h +1 -1
@@ -62,7 +62,7 @@ void cfcnfg_remove(struct cfcnfg *cfg);* @fcs: Specify if checksum is used in CAIF Framing Layer. * @head_room: Head space needed by link specific protocol. */ -void +int cfcnfg_add_phy_layer(struct cfcnfg *cnfg, struct net_device *dev, struct cflayer *phy_layer, enum cfcnfg_phy_preference pref,
net/caif/caif_dev.c +5 -3
@@ -308,7 +308,7 @@ static void dev_flowctrl(struct net_device *dev, int on)caifd_put (caifd); } -void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev, +int caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev, struct cflayer *link_support, int head_room, struct cflayer **layer, int (**rcv_func)(struct sk_buff *, struct net_device *,
@@ -319,11 +319,12 @@ void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev,enum cfcnfg_phy_preference pref;struct cfcnfg *cfg =get_cfcnfg (dev_net (dev));struct caif_device_entry_list *caifdevs; +int res; caifdevs =caif_device_list (dev_net (dev)); caifd =caif_device_alloc (dev);if (!caifd) -return ; +return -ENOMEM ; *layer = &caifd->layer;spin_lock_init (&caifd->flow_lock);
@@ -344,7 +345,7 @@ void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev,strlcpy (caifd->layer.name, dev->name,sizeof (caifd->layer.name)); caifd->layer.transmit = transmit; - cfcnfg_add_phy_layer(cfg, + res = cfcnfg_add_phy_layer(cfg, dev, &caifd->layer, pref,
@@ -354,6 +355,7 @@ void caif_enroll_dev(struct net_device *dev, struct caif_dev_common *caifdev,mutex_unlock (&caifdevs->lock);if (rcv_func) *rcv_func = receive; +return res; }EXPORT_SYMBOL (caif_enroll_dev);
net/caif/cfcnfg.c +11 -5
@@ -450,7 +450,7 @@ cfcnfg_linkup_rsp(struct cflayer *layer, u8 channel_id, enum cfctrl_srv serv,rcu_read_unlock (); } -void +int cfcnfg_add_phy_layer(struct cfcnfg *cnfg, struct net_device *dev, struct cflayer *phy_layer, enum cfcnfg_phy_preference pref,
@@ -459,7 +459,7 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, {struct cflayer *frml;struct cfcnfg_phyinfo *phyinfo =NULL ; -int i; +int i, res =0 ;u8 phyid;mutex_lock (&cnfg->lock);
@@ -473,12 +473,15 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, goto got_phyid; }pr_warn ("Too many CAIF Link Layers (max 6)\n" ); + res = -EEXIST ; goto out; got_phyid: phyinfo =kzalloc (sizeof (struct cfcnfg_phyinfo ),GFP_ATOMIC ); -if (!phyinfo) +if (!phyinfo) { + res = -ENOMEM ; goto out_err; + } phy_layer->id = phyid; phyinfo->pref = pref;
@@ -492,8 +495,10 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, frml =cffrml_create (phyid, fcs); -if (!frml) +if (!frml) { + res = -ENOMEM ; goto out_err; + } phyinfo->frm_layer = frml;layer_set_up (frml, cnfg->mux);
@@ -511,11 +516,12 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg,list_add_rcu (&phyinfo->node, &cnfg->phys); out:mutex_unlock (&cnfg->lock); -return ; +return res; out_err:kfree (phyinfo);mutex_unlock (&cnfg->lock); +return res; }EXPORT_SYMBOL (cfcnfg_add_phy_layer);
Signature · 1 line
2.31.1
PATCH 3/4 net: caif: fix memory leak in caif_device_notify
In reply to Pavel SkripkinIn case of caif_enroll_dev() fail, allocated link_support won't be assigned to the corresponding structure. So simply free allocated pointer in case of error
--- net/caif/caif_dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
net/caif/caif_dev.c +4 -1
@@ -370,6 +370,7 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what,struct cflayer *layer, *link_support;int head_room =0 ;struct caif_device_entry_list *caifdevs; +int res; cfg =get_cfcnfg (dev_net (dev)); caifdevs =caif_device_list (dev_net (dev));
@@ -395,8 +396,10 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what,break ; } } -caif_enroll_dev (dev, caifdev, link_support, head_room, + res =caif_enroll_dev (dev, caifdev, link_support, head_room, &layer,NULL ); +if (res) +cfserl_release (link_support); caifdev->flowctrl = dev_flowctrl;break ;
Signature · 1 line
2.31.1
Re: [PATCH 3/4] net: caif: fix memory leak in caif_device_notify
Quoted 28 lines · On Thu, 3 Jun 2021 19:39:11 +0300 Pavel Skripkin <[email protected]> wrote:
In case of
caif_enroll_dev()fail, allocatedlink_supportwon't be assigned to the corresponding structure. So simply free allocated pointer in case of errorFixes 7c18d2205ea7 ("caif: Restructure how link caif link layerenroll") Cc: [email protected] Reported-and-tested-by:
[email protected] Signed-off-by:
Pavel Skripkin <[email protected]> ---net/caif/caif_dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)net/caif/caif_dev.c +1
-0@@ -370,6 +370,7 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, struct cflayer *layer, *link_support;int head_room =0 ;struct caif_device_entry_list *caifdevs; +int res; cfg =get_cfcnfg (dev_net (dev)); caifdevs =caif_device_list (dev_net (dev));@@ -395,8 +396,10 @@ static int caif_device_notify(struct notifier_block *me, unsigned long what, break;
net/caif/caif_dev.c static int caif_device_notify(struct · line 405
Quoted 10 lines
net/caif/caif_dev.c +3
-1} } -caif_enroll_dev (dev, caifdev, link_support, head_room, + res =caif_enroll_dev (dev, caifdev, link_support, head_room, &layer,NULL ); +if (res) +cfserl_release (link_support); caifdev->flowctrl = dev_flowctrl;break ;
One thing Im wondering about is should I return this error from caif_device_notify()? I look forward to hearing your perspective on this question and patch series :)
With regards,
Pavel Skripkin
PATCH 4/4 net: caif: fix memory leak in cfusbl_device_notify
In reply to Pavel SkripkinIn case of caif_enroll_dev() fail, allocated link_support won't be assigned to the corresponding structure. So simply free allocated pointer in case of error.
--- net/caif/caif_usb.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
net/caif/caif_usb.c +13 -1
@@ -115,6 +115,11 @@ static struct cflayer *cfusbl_create(int phyid, u8 ethaddr[ETH_ALEN],return (struct cflayer *) this; } +static void cfusbl_release (struct cflayer *layer) +{ +kfree (layer); +} +static struct packet_type caif_usb_type__read_mostly = { .type =cpu_to_be16 (ETH_P_802_EX1 ), };
@@ -127,6 +132,7 @@ static int cfusbl_device_notify(struct notifier_block *me, unsigned long what,struct cflayer *layer, *link_support;struct usbnet *usbnet;struct usb_device *usbdev; +int res;/* Check whether we have a NCM device, and find its VID/PID. */ if (!(dev->dev.parent && dev->dev.parent->driver &&
@@ -169,8 +175,11 @@ static int cfusbl_device_notify(struct notifier_block *me, unsigned long what,if (dev->num_tx_queues >1 )pr_warn ("USB device uses more than one tx queue\n" ); -caif_enroll_dev (dev, &common, link_support,CFUSB_MAX_HEADLEN , + res =caif_enroll_dev (dev, &common, link_support,CFUSB_MAX_HEADLEN , &layer, &caif_usb_type.func); +if (res) + goto err; +if (!pack_added)dev_add_pack (&caif_usb_type); pack_added = true;
@@ -178,6 +187,9 @@ static int cfusbl_device_notify(struct notifier_block *me, unsigned long what,strlcpy (layer->name, dev->name,sizeof (layer->name));return 0 ; +err: +cfusbl_release (link_support); +return res; }static struct notifier_block caif_device_notifier = {
Signature · 1 line
2.31.1
This series was applied to netdev/net.git (refs/heads/master):
Hello:
This series was applied to netdev/net.git (refs/heads/master):
Quoted 10 lines · On Thu, 3 Jun 2021 19:37:27 +0300 you wrote:
This patch series fix 2 memory leaks in caif interface.
Syzbot reported memory leak in
cfserl_create(). The problem was incfcnfg_add_phy_layer()function. This function accepts struct cflayer*link_supportand assign it to corresponting structures, but it can fail in some cases.[...]
Here is the summary with links:
- [1/4] net: caif: added cfserl_release function
https://git.kernel.org/netdev/net/c/bce130e7f392 - [2/4] net: caif: add proper error handling https://git.kernel.org/netdev/net/c/a2805dca5107 - [3/4] net: caif: fix memory leak in caif_device_notify https://git.kernel.org/netdev/net/c/b53558a950a8 - [4/4] net: caif: fix memory leak in cfusbl_device_notify https://git.kernel.org/netdev/net/c/7f5d86669fa4
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html