linux-kernel - Re: Dangerous code in cpumask_of

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:	Tue, 08 Jul 2008 10:54:38 +0200
From:	Johannes Weiner <hannes@...urebad.de>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Mike Travis <travis@....com>, linux-kernel@...r.kernel.org,
	"H. Anvin" <hpa@...or.com>, Christoph Lameter <clameter@....com>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: Dangerous code in cpumask_of_cpu?

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:

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)
--
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/