Messages in this thread Patch in this message | | | From | Johannes Weiner <> | Subject | Re: Dangerous code in cpumask_of_cpu? | Date | Tue, 08 Jul 2008 10:54:38 +0200 |
| |
Hi,
Johannes Weiner <hannes@saeurebad.de> writes:
> Hi, > > Rusty Russell <rusty@rustcorp.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:
static int *foo(void) { int x = 42; return &x; }
int main(void) { return *foo(); }
> But then, this code should probably just evaluate to m without this > obscure *(&m) construct.
This, however is still possible, no?
--- Subject: cpumask: don't dereference an invalidated pointer
m is auto storage, don't use its address outside its scope. Just return m directly instead of that *({type m; &m}) construct.
---
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index c24875b..19802cb 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -232,7 +232,7 @@ extern cpumask_t *cpumask_of_cpu_map; #else #define cpumask_of_cpu(cpu) \ -(*({ \ +({ \ typeof(_unused_cpumask_arg_) m; \ if (sizeof(m) == sizeof(unsigned long)) { \ m.bits[0] = 1UL<<(cpu); \ @@ -240,8 +240,8 @@ extern cpumask_t *cpumask_of_cpu_map; cpus_clear(m); \ cpu_set((cpu), m); \ } \ - &m; \ -})) + m; \ +}) #endif #define CPU_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(NR_CPUS)
| |