Re: Dangerous code in cpumask_of_cpu?

[Johannes Weiner <hannes@saeurebad.de>]

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)