[PATCH 0/4] net: caif: fix 2 memory leaks

[PATCH 0/4] net: caif: fix 2 memory leaks

[Pavel Skripkin]

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

-- 
2.31.1

[PATCH 1/4] net: caif: added cfserl_release function

[Pavel Skripkin]

Added cfserl_release() function.

Cc: stable@vger.kernel.org
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 include/net/caif/cfserl.h | 1 +
 net/caif/cfserl.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/net/caif/cfserl.h b/include/net/caif/cfserl.h
index 14a55e03bb3c..67cce8757175 100644
--- a/include/net/caif/cfserl.h
+++ b/include/net/caif/cfserl.h
@@ -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
diff --git a/net/caif/cfserl.c b/net/caif/cfserl.c
index e11725a4bb0e..40cd57ad0a0f 100644
--- a/net/caif/cfserl.c
+++ b/net/caif/cfserl.c
@@ -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);
-- 
2.31.1

[PATCH 2/4] net: caif: add proper error handling

[Pavel Skripkin]

caif_enroll_dev() can fail in some cases. Ingnoring
these cases can lead to memory leak due to not assigning
link_support pointer to anywhere.

Fixes: 7c18d2205ea7 ("caif: Restructure how link caif link layer enroll")
Cc: stable@vger.kernel.org
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 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(-)

diff --git a/include/net/caif/caif_dev.h b/include/net/caif/caif_dev.h
index 48ecca8530ff..b655d8666f55 100644
--- a/include/net/caif/caif_dev.h
+++ b/include/net/caif/caif_dev.h
@@ -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 *,
diff --git a/include/net/caif/cfcnfg.h b/include/net/caif/cfcnfg.h
index 2aa5e91d8457..8819ff4db35a 100644
--- a/include/net/caif/cfcnfg.h
+++ b/include/net/caif/cfcnfg.h
@@ -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,
diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index c10e5a55758d..fffbe41440b3 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -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);
 
diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index 399239a14420..cac30e676ac9 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -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);
 
-- 
2.31.1

[PATCH 3/4] net: caif: fix memory leak in caif_device_notify

[Pavel Skripkin]

In 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

Fixes: 7c18d2205ea7 ("caif: Restructure how link caif link layer enroll")
Cc: stable@vger.kernel.org
Reported-and-tested-by: syzbot+7ec324747ce876a29db6@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 net/caif/caif_dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index fffbe41440b3..440139706130 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -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;
 
-- 
2.31.1

Re: [PATCH 3/4] net: caif: fix memory leak in caif_device_notify

[Pavel Skripkin]

On Thu,  3 Jun 2021 19:39:11 +0300
Pavel Skripkin <paskripkin@gmail.com> wrote:

> In 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
> 
> Fixes: 7c18d2205ea7 ("caif: Restructure how link caif link layer
> enroll") Cc: stable@vger.kernel.org
> Reported-and-tested-by:
> syzbot+7ec324747ce876a29db6@syzkaller.appspotmail.com Signed-off-by:
> Pavel Skripkin <paskripkin@gmail.com> ---
>  net/caif/caif_dev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
> index fffbe41440b3..440139706130 100644
> --- a/net/caif/caif_dev.c
> +++ b/net/caif/caif_dev.c
> @@ -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;
>  

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

[Pavel Skripkin]

In 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.

Fixes: 7ad65bf68d70 ("caif: Add support for CAIF over CDC NCM USB interface")
Cc: stable@vger.kernel.org
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 net/caif/caif_usb.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/caif/caif_usb.c b/net/caif/caif_usb.c
index a0116b9503d9..b02e1292f7f1 100644
--- a/net/caif/caif_usb.c
+++ b/net/caif/caif_usb.c
@@ -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 = {
-- 
2.31.1

Re: [PATCH 0/4] net: caif: fix 2 memory leaks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

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 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.
> 
> [...]

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