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
| ||
|
Date: Mon, 31 Aug 2020 11:47:58 +0200 From: Eric Dumazet <eric.dumazet@...il.com> To: Tuong Tong Lien <tuong.t.lien@...tech.com.au>, Eric Dumazet <eric.dumazet@...il.com>, "davem@...emloft.net" <davem@...emloft.net>, "jmaloy@...hat.com" <jmaloy@...hat.com>, "maloy@...jonn.com" <maloy@...jonn.com>, "ying.xue@...driver.com" <ying.xue@...driver.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org> Cc: "tipc-discussion@...ts.sourceforge.net" <tipc-discussion@...ts.sourceforge.net> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible On 8/31/20 1:33 AM, Tuong Tong Lien wrote: > Hi Eric, > > Thanks for your comments, please see my answers inline. > >> -----Original Message----- >> From: Eric Dumazet <eric.dumazet@...il.com> >> Sent: Monday, August 31, 2020 3:15 PM >> To: Tuong Tong Lien <tuong.t.lien@...tech.com.au>; davem@...emloft.net; jmaloy@...hat.com; maloy@...jonn.com; >> ying.xue@...driver.com; netdev@...r.kernel.org >> Cc: tipc-discussion@...ts.sourceforge.net >> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible >> >> >> >> On 8/29/20 12:37 PM, Tuong Lien wrote: >>> The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the current >>> CPU for encryption, however the execution can be preemptible since it's >>> actually user-space context, so the 'using smp_processor_id() in >>> preemptible' has been observed. >>> >>> We fix the issue by using the 'get/put_cpu_ptr()' API which consists of >>> a 'preempt_disable()' instead. >>> >>> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication") >> >> Have you forgotten ' Reported-by: syzbot+263f8c0d007dc09b2dda@...kaller.appspotmail.com' ? > Well, really I detected the issue during my testing instead, didn't know if it was reported by syzbot too. > >> >>> Acked-by: Jon Maloy <jmaloy@...hat.com> >>> Signed-off-by: Tuong Lien <tuong.t.lien@...tech.com.au> >>> --- >>> net/tipc/crypto.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c >>> index c38babaa4e57..7c523dc81575 100644 >>> --- a/net/tipc/crypto.c >>> +++ b/net/tipc/crypto.c >>> @@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp) >>> if (aead->cloned) { >>> tipc_aead_put(aead->cloned); >>> } else { >>> - head = *this_cpu_ptr(aead->tfm_entry); >>> + head = *get_cpu_ptr(aead->tfm_entry); >>> + put_cpu_ptr(aead->tfm_entry); >> >> Why is this safe ? >> >> I think that this very unusual construct needs a comment, because this is not obvious. >> >> This really looks like an attempt to silence syzbot to me. > No, this is not to silence syzbot but really safe. > This is because the "aead->tfm_entry" object is "common" between CPUs, there is only its pointer to be the "per_cpu" one. So just trying to lock the process on the current CPU or 'preempt_disable()', taking the per-cpu pointer and dereferencing to the actual "tfm_entry" object... is enough. Later on, that’s fine to play with the actual object without any locking. Why using per cpu pointers, if they all point to a common object ? This makes the code really confusing. Why no lock is required ? This seems hard to believe, given lack of clear explanations anywhere in commit fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication"). If the object can be used without locking, it should be marked const. tipc_aead_tfm_next() has side effects that I really can not understand in SMP world, and presumably with soft interrupts in UP as well.
Powered by blists - more mailing lists