linux-kernel - Re: [PATCH v3] sched/numa: fix unsafe get_task_struct() in task_numa

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]

Message-ID: <20141022090954.GF12706@worktop.programming.kicks-ass.net>
Date:	Wed, 22 Oct 2014 11:09:54 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Kirill Tkhai <tkhai@...dex.ru>,
	Kirill Tkhai <ktkhai@...allels.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
	Vladimir Davydov <vdavydov@...allels.com>, cl@...ux.com
Subject: Re: [PATCH v3] sched/numa: fix unsafe get_task_struct() in
 task_numa_assign()

On Tue, Oct 21, 2014 at 09:03:35PM +0200, Oleg Nesterov wrote:
> > I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel
> > thing
> 
> I won't really insist, but let me try to explain why I dislike it in
> this particular case.
> 
> - It is not clear who else (except task_numa_compare) will need it.
>   And it looks at bit strange to make task_struct SLAB_DESTROY_BY_RCU
>   just to read a word in task_numa_compare().
> 
> - In some sense, the usage of SDBR looks simply "wrong" in this case.
>   IOW, I agree that probe_kernel_read() is ugly, but in this case
>   SDBR acts exactly the same way as probe_kernel_read().
> 
>   SDBR does not make the object rcu-safe, it only protects the object
>   type plus ensures that this memory can't go away. It was designed
>   for the case when you read the stable members initialized in ctor
>   (usually a lock) and verify/lock the object.
> 
>   But in this case we can not detect that the object is still alive
>   without the additional trick, so when you read ->sighand or ->flags,
>   the fact that this memory is still "struct task_struct" doesn't help
>   and doesn't matter at all. Only the subsequent "task == rq->curr"
>   check proves that yes, this is task_struct.
> 
>   OTOH, (afaics) we only need probe_kernel_read() if CONFIG_DEBUG_SLAB.
>   And in fact I think that "read the valid but potentially freed kernel
>   pointer" deserves another helper, it can have more users. For example,
>   please look at get_freepointer_safe().
> 
>   What if we add get_kernel(x, ptr) macro to factor out the IS_ENABLED()
>   of ifdef hack? Or inline function... This way the new xxx() helper we
>   discussed won't look that bad.
> 
> But again, I agree that this subjective, I won't really argue.

So I worry about cache aliasing (not an issue on x86), so by touching
'random' pages that might be freed and reissued to back userspace, we
could be accessing the one page through multiple virtual mappings which
therefore result in aliases.

SDBR avoids this issue by guaranteeing the page is not reissued for
another purpose.

I'm not sure I can convince myself SLUB is correct here. How do we avoid
cache aliasing.
--
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/