linux-kernel - Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE

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 for Android: free password hash cracker in your pocket

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

Date:	Sun, 19 Apr 2009 14:12:07 -0700
From:	Roland Dreier <rdreier@...co.com>
To:	"Robert P. J. Day" <rpjday@...shcourse.ca>
Cc:	Hitoshi Mitake <h.mitake@...il.com>, Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars

 > from arch/x86/Kconfig:
 > 	...
 >         select HAVE_READQ
 >         select HAVE_WRITEQ
 > 	...
 > 
 > yet there are no such defined Kconfig vars anywhere.  thoughts?

git blame shows that this came in from 2c5643b1 ("x86: provide
readq()/writeq() on 32-bit too").  And that commit looks very dubious
indeed to me -- it defines readq() and writeq() in a way that is not
atomic and probably won't generate single 64-bit bus cycles.

Now, many drivers do "#ifndef readq <define my own implementation> #endif"
but exactly what is required is very hardware-dependent, and accessing
32-bit halves in the wrong order may lead to very subtle bugs.  For
example, the changelog for e23a59e1 ("niu: Fix readq implementation when
architecture does not provide one.") says:

    In particular one of the issues is whether the top 32-bits
    or the bottom 32-bits of the 64-bit register should be read
    first.  There could be side effects, and in fact that is
    exactly the problem here.

By coincidence, the 32-bit x86 implementation is actually OK for niu,
but I didn't audit every similar driver, and I don't think any
implementation of readq()/writeq() that generates multiple bus cycles is
suitable in general -- it doesn't meet the requirements of the API.

So I would strongly suggest reverting 2c5643b1 since as far as I can
tell it just sets a trap for subtle bugs that only show up on 32-bit
x86 -- any portable driver still needs to provide readq()/writeq() for
other 32-bit architectures, so it doesn't really help anyone.

 - R.
--
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/