netdev - Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

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: <4656072B.40801@garzik.org>
Date:	Thu, 24 May 2007 17:44:11 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
CC:	netdev@...r.kernel.org, mhuth@...sta.com,
	kgdb-bugreport@...ts.sourceforge.net
Subject: Re: [Kgdb-bugreport] [PATCH] 8139too: harden against TX ring overflow

Sergei Shtylyov wrote:
> Jeff Garzik wrote:
> 
>>>> This driver's 4-packet deep TX queue is too sensible to the 
>>>> "careless" callers
>>>> ignoring its state (like netpoll in trapped mode), so add "queue 
>>>> full" check at
>>>> the start of the hard_start_xmit() method (only under #ifndef 
>>>> RTL8139_NDEBUG,
>>>> otherwise the queue will get stuck once dirty pointer gets out of 
>>>> sync); switch
>>>> to using appropriate mnemonics for the return values while at it.
>>>> Also, the out-of-sync dirty pointer check is misplaced in 
>>>> rtl8139_tx_interrupt()
>>>> which causes TX descriptors to be inspected more than once in case 
>>>> the pointer
>>>> really gets out-of-sync (and incrementing the dirty pointer always 
>>>> by 4 is just
>>>> not enough, e.g. KGDBoE managed to stuff 20+ extra buffers into the 
>>>> queue) --
>>>> place it before the loop and limit the loop to only look through 4 
>>>> descriptors
>>>> at most, so that already overwritten descriptors are just not counted.
> 
>>>> Signed-off-by: Sergei Shtylyov <sshtylyov@...mvista.com>
> 
>>>    Jeff, do you have any opinion on this patch?
> 
>> The out-of-sync dirty pointer check is leftover boilerplate, and not 
>> really indicative of anything except for some code to be removed.
> 
>> As for the other stuff, I would say "fix the caller".  We don't need 
>> to hack every driver for cases where netpoll is being dumb.
> 
>    Caller's been fixed now. So, I read that as reject. :-)

Mostly.  You're still welcome to remove the silly boilerplate checks...

And ->hard_start_xmit() return value update(s) were sane.

	Jeff



-
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