netdev - Re: [PATCH net-next v3 1/3] can: mscan: fix improper return if dlc < 8 in start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4B46239E.2000403@hartkopp.net>
Date:	Thu, 07 Jan 2010 19:10:38 +0100
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Wolfgang Grandegger <wg@...ndegger.com>
CC:	Netdev@...r.kernel.org, Socketcan-core@...ts.berlios.de,
	Linuxppc-dev@...ts.ozlabs.org, Devicetree-discuss@...ts.ozlabs.org,
	Wolfram Sang <w.sang@...gutronix.de>,
	Wolfgang Grandegger <wg@...x.de>
Subject: Re: [PATCH net-next v3 1/3] can: mscan: fix improper return if dlc
 < 8 in start_xmit function

Wolfgang Grandegger wrote:

> The start_xmit function of the MSCAN Driver did return improperly if
> the CAN dlc check failed (skb not freed and invalid return code). This
> patch adds a proper check of the frame lenght and data size and returns
> now correctly. 

> @@ -177,8 +177,13 @@ static netdev_tx_t mscan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	int i, rtr, buf_id;
>  	u32 can_id;
>  
> -	if (frame->can_dlc > 8)
> -		return -EINVAL;
> +	if (skb->len != sizeof(*frame) || frame->can_dlc > 8) {
> +		dev_err(dev->dev.parent,
> +			"Dropping non-conform packet: len %u, can_dlc %u\n",
> +			skb->len, frame->can_dlc);
> +		kfree_skb(skb);
> +		return NETDEV_TX_OK;
> +	}
>  

Hi Wolfgang,

i would suggest to remove the dev_err() which may flood the kernel log and add

   dev->stats.tx_dropped++;

instead.

As discussed with DaveM on netdev-ML this 'silent' handling seems to be the
most appropriate approach to deal with invalid skbs.

We should update the other CAN drivers in a similar way, if this is ok for you.

Best regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html