linux-kernel - Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

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]

Date:	Mon, 20 Nov 2006 01:34:40 -0500
From:	Chris Snook <csnook@...hat.com>
To:	Alan <alan@...rguk.ukuu.org.uk>
CC:	Jay Cliburn <jacliburn@...lsouth.net>, jeff@...zik.org,
	shemminger@...l.org, romieu@...zoreil.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

Alan wrote:
> Would be nice if it used atl_ not at_ so its less likely to cause
> namespace clashes.

Some of this code looked like Attansic may have meant to share it 
between drivers for atl1/atl2/atf1/atf2, but seeing as I can't find any 
code for those, I'll convert it all to atl1_ and let Attansic generalize 
the code if they ever decide they want to submit drivers.

> You have various macros for swaps that are pretty ugly - we have
> cpu_to and le/be_to_cpu functions for most swapping cases and these are
> generally optimised assembler (eg bswap on x86)
> 
> AT_DESC_USED/UNUSED would be better as inline functions but thats not a
> serious concern.
> 
> Be careful with :1 bitfields when working with hardware - the compiler
> has more than one choice about how to pack them.

Lacking a spec, I'm not entirely sure what the original intent was, so 
we're stuck with testing.  Is there a specific disambiguation technique 
you recommend?

> The irq enable/disable use for locking on vlan appears unsafe. PCI
> interrupt delivery is asynchronous which means you can get this happen
> 
> 
> 	card sends PCI interrupt
> 	We call irq_disable
> 	We take lock
> 	We poke bits
> 	We drop lock
> 
> 	PCI interrupt arrives
> 
> 
> This really does happen, typically its nasty to debug as well because you
> usually only get it on PIII boards on the one in n-zillion times a
> message collides and is retransmitted on the APIC bus.

Nice catch.  I admit the VLAN code is not so well audited or tested. 
Fortunately, the chip only seems to be on Asus M2V motherboards, at 
least for now, but I want to audit all of the locking code at some point.

> skb->len is unsigned so <= 0 can be == 0. More importantly the subtraction
> before the test will wrap and is completely unsafe (see at_xmit_frame)

Thanks!

	-- Chris
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/