linux-kernel - Re: linux-next: Tree for June 5

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:	Fri, 6 Jun 2008 16:36:04 +0200
From:	"Vegard Nossum" <vegard.nossum@...il.com>
To:	"Mike Travis" <travis@....com>, "Ingo Molnar" <mingo@...e.hu>
Cc:	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Stephen Rothwell" <sfr@...b.auug.org.au>,
	linux-next@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: linux-next: Tree for June 5

On Fri, Jun 6, 2008 at 4:20 PM, Mike Travis <travis@....com> wrote:
> Vegard Nossum wrote:
>> On Fri, Jun 6, 2008 at 3:50 PM, Vegard Nossum <vegard.nossum@...il.com> wrote:
>>> On Fri, Jun 6, 2008 at 3:33 PM, Mike Travis <travis@....com> wrote:
>>>> Vegard Nossum wrote:
>>>>> I reproced it with gc 4.1.2. I think the error is somewhere in kernel/sched.c.
>>>>>
>>>>> static int __build_sched_domains(const cpumask_t *cpu_map,
>>>>>                                  struct sched_domain_attr *attr)
>>>>> {
>>>>> ...
>>>>>         for (i = 0; i < MAX_NUMNODES; i++) {
>>>>> ...
>>>>>                 sg = kmalloc_node(sizeof(struct sched_group), GFP_KERNEL, i);
>>>>> ...
>>>>>
>>>>> This code is calling into the allocator with a spurious value of i,
>>>>> which causes SLAB to use an index (of 4 in my case) that is out of
>>>>> bounds for its nodelist array (at least it hasn't been initialized).
>>>>>

...

>> The error is of course that the node masks for nodes > nr_node_ids are
>> not valid. While this function ignores that:
>>
>> cpumask_t *_node_to_cpumask_ptr(int node)
>> {
>>         if (node_to_cpumask_map == NULL) {
>>                 printk(KERN_WARNING
>>                         "_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n",
>>                         node);
>>                 dump_stack();
>>                 return &cpu_online_map;
>>         }
>>         return &node_to_cpumask_map[node];
>> }
>> EXPORT_SYMBOL(_node_to_cpumask_ptr);
>>
>> Notice the return statement. It needs to check if node < nr_node_ids.
>>

...

>
> Thanks, yes I had that some after thought.  It should check the node
> index if CONFIG_DEBUG_PER_CPU_MAPS is enabled.  One gotcha is that
> nr_node_ids is intialized to MAX_NUMNODES until setup_node_to_cpumask_map()
> sets it to the correct value.  So uses before that should be caught by
> the earlier check.

I think it should always check the node index. The code in
kernel/sched.c (see above) calls node_to_cpumask(i) on nodes 0 < i <
MAX_NUMNODES and it WILL use invalid pointers. Or should
kernel/sched.c be changed to use nr_node_ids instead of MAX_NUMNODES?
I believe there are more places that do this than just sched.c.

I have attached two patches. The sched one fixes Andrew's boot
problem. The x86 one is untested, but I believe it is better to BUG
than silently corrupt some arbitrary memory. (Then the callers can be
found easily and fixed at least.)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

View attachment "0001-sched-don-t-call-node_to_cpumask-on-nodes-nr_no.patch" of type "text/x-patch" (1830 bytes)

View attachment "0001-x86-don-t-return-invalid-pointers-from-node_to_cpum.patch" of type "text/x-patch" (966 bytes)