netdev - Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <878v7hfzmo.fsf@nemi.mork.no>
Date:	Fri, 25 Jan 2013 09:14:23 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Joe Perches <joe@...ches.com>
Cc:	Oliver Neukum <oneukum@...e.de>, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

Joe Perches <joe@...ches.com> writes:

> On Thu, 2013-01-24 at 20:16 +0100, Bjørn Mork wrote:
>> A device sending 0 length frames as fast as it can has been
>> observed killing the host system due to the resulting memory
>> pressure.
> []
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> []
>> @@ -539,6 +545,22 @@ block:
>>  		break;
>>  	}
>>  
>> +	/* stop rx if packet error rate is high */
>> +	if (++dev->pkt_cnt > 30) {
>> +		dev->pkt_cnt = 0;
>> +		dev->pkt_err = 0;
>> +	} else {
>> +		if (state == rx_cleanup)
>> +			dev->pkt_err++;
>> +		if (dev->pkt_err > 20) {
>> +			set_bit(EVENT_RX_KILL, &dev->flags);
>> +			if (net_ratelimit())
>> +				netif_dbg(dev, rx_err, dev->net,
>> +					  "rx kill: high error rate\n");
>> +			dev->pkt_err = 0;
>> +		}
>> +	}
>
> Maybe use ratelimit() here?
>
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> []
>> @@ -33,6 +33,7 @@ struct usbnet {
>>  	wait_queue_head_t	*wait;
>>  	struct mutex		phy_mutex;
>>  	unsigned char		suspend_count;
>> +	unsigned char		pkt_cnt, pkt_err;
>
> and instead:
>
> 	struct ratelimit_state errors;

Thanks.  I took a look at this, but it seems to be more complex than I
really wanted for keeping the debug noise down here.  The rest of usbnet
does not care much about rate limiting debug messages at all.  I'll get
a message for every 0 length packet for example.

Maybe usbnet should get a private debug ratelimiter all over?

Is the problem that these instances will hide more important net
messages?  Would it help to make the ratelimit call depend on whether
debugging is enabled like ath5k and brcm80211 seems to do?



Bjørn

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