Commit e0310182 authored by David S. Miller's avatar David S. Miller
Browse files

Merge branch 'caif-fixes' 



Pavel Skripkin says:

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

Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 4189777c 7f5d8666

include/net/caif/caif_dev.h

+1 −1
Original line number Diff line number Diff line
@@ -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
Original line number Diff line number Diff line
@@ -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,

include/net/caif/cfserl.h

+1 −0
Original line number Diff line number Diff line
@@ -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/caif_dev.c

+9 −4
Original line number Diff line number Diff line
@@ -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);
@@ -368,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));
@@ -393,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;

net/caif/caif_usb.c

+13 −1
Original line number Diff line number Diff line
@@ -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 = {