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
| ||
|
Date: Wed, 9 Jul 2008 12:22:44 +1000 From: Rusty Russell <rusty@...tcorp.com.au> To: Mike Travis <travis@....com> Cc: Johannes Weiner <hannes@...urebad.de>, linux-kernel@...r.kernel.org, "H. Anvin" <hpa@...or.com>, Christoph Lameter <cl@...ux-foundation.org>, Ingo Molnar <mingo@...e.hu> Subject: Re: Dangerous code in cpumask_of_cpu? On Wednesday 09 July 2008 01:29:34 Mike Travis wrote: > Johannes Weiner wrote: > > Johannes Weiner <hannes@...urebad.de> writes: > >> Hi, > >> > >> Johannes Weiner <hannes@...urebad.de> writes: > >>> Hi, > >>> > >>> Rusty Russell <rusty@...tcorp.com.au> writes: > >>>> Hi Christoph/Mike, > >>>> > >>>> Looked at cpumask_of_cpu as introduced in > >>>> 9f0e8d0400d925c3acd5f4e01dbeb736e4011882 (x86: convert cpumask_of_cpu > >>>> macro to allocated array), and I don't think it's safe: > >>>> > >>>> #define cpumask_of_cpu(cpu) \ > >>>> (*({ \ > >>>> typeof(_unused_cpumask_arg_) m; \ > >>>> if (sizeof(m) == sizeof(unsigned long)) { \ > >>>> m.bits[0] = 1UL<<(cpu); \ > >>>> } else { \ > >>>> cpus_clear(m); \ > >>>> cpu_set((cpu), m); \ > >>>> } \ > >>>> &m; \ > >>>> })) > >>>> > >>>> Referring to &m once out of scope is invalid, and I can't find any > >>>> evidence that it's legal here. In particular, the change > >>>> b53e921ba1cff8453dc9a87a84052fa12d5b30bd (generic: reduce stack > >>>> pressure in sched_affinity) which passes &m to other functions seems > >>>> highly risky. > >>>> > >>>> I'm surprised this hasn't already hit us, but perhaps gcc isn't as > >>>> clever as it could be? > >>> > >>> You don't refer to &m outside scope. Look at the character below the > >>> first e of #define :) > >> > >> Oh, well you do access it outside scope, sorry. Me sleepy. > >> > >> I guess because we dereference it immediately again, the location is not > >> clobbered yet. At least in my test case, gcc assembled it to code that > >> puts the address in eax and derefences it immediately, before eax is > >> reused: > > > > Gee, just ignore this bs. The address is in eax, not the value. > > > >> static int *foo(void) > >> { > >> int x = 42; > >> return &x; > >> } > >> > >> int main(void) > >> { > >> return *foo(); > >> } > > > > However, this code seems to produce valid assembly with -O2. gcc just > > warns and fixes it up. > > > > Hannes > > IIRC, the problem was I needed an lvalue and it seems that the *(&m) was > the way I was able to coerce gcc into producing it. That's not to say > there may be a better way however... ;-) [Btw, I picked up this technique > in the (original) per_cpu() macro.] Yes, but I could do that because it wasn't referring to a temporary variable. > Note the lvalue isn't used for changing the cpumask value, but for sending > it to functions like set_cpus_allowed_ptr() to avoid pushing the 512 bytes > of a 4096 cpus cpumask onto the stack. So it becomes &(*(&m))) ... ;-) > But I thought I checked the assembly for different config options and it > looked ok. Yeah, the problem is that a future gcc will cause horrible and subtle breakage. I think we are going to want a get_cpumask()/put_cpumask() pattern for this. Rusty. -- 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/
Powered by blists - more mailing lists