linux-kernel - Re: [PATCH v15 02/12] cpu/hotplug: Move idle_thread

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: <82674e57bc06a3e05e581bdf4e1b7413202e1ec7.camel@infradead.org>
Date:   Mon, 20 Mar 2023 08:17:29 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Borislav Petkov <bp@...en8.de>,
        Usama Arif <usama.arif@...edance.com>
Cc:     tglx@...utronix.de, kim.phillips@....com, brgerst@...il.com,
        piotrgorski@...hyos.org, oleksandr@...alenko.name,
        arjan@...ux.intel.com, mingo@...hat.com,
        dave.hansen@...ux.intel.com, hpa@...or.com, x86@...nel.org,
        pbonzini@...hat.com, paulmck@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        rcu@...r.kernel.org, mimoja@...oja.de, hewenliang4@...wei.com,
        thomas.lendacky@....com, seanjc@...gle.com, pmenzel@...gen.mpg.de,
        fam.zheng@...edance.com, punit.agrawal@...edance.com,
        simon.evans@...edance.com, liangma@...ngbit.com,
        gpiccoli@...lia.com
Subject: Re: [PATCH v15 02/12] cpu/hotplug: Move idle_thread_get() to
 <linux/smpboot.h>

On Sun, 2023-03-19 at 17:34 +0100, Borislav Petkov wrote:
> Frankly, I don't think resetting shadow call stack and kasan state
> belongs in a function which returns the idle thread. Even more so if you
> have to add an @unpoison param which is false sometimes and sometimes
> true, depending on where you call the function.
> 
> I think you should have a helper
> 
>         tsk_reset_stacks(struct task_struct *tsk);
> 
> or so which is called where @unpoison == true instead of having a getter
> function do something unrelated too.

Yeah, I see that but my primary concern was that I didn't want callers
to be able to *forget* it, which is what happened the first time.

I don't feel so bad about the getter function actually making the
object *usable* as well as getting it. We don't have to remember to
make a separate function call to unpoison a pointer newly returned from
kmalloc either.

I *think* the 'unpoison' arg is purely an optimisation anyway. Because
those operations are, or at least *could* be, idempotent. It's just
that it's a bit pointless doing them from the finish_cpu() call.

But actually, I think we can have it both ways. There's an early call
to idle_thread_get(cpu) in _cpu_up(), with the comment /* Let it fail
before we try to bring the cpu up */

With an adjustment to the comment, we could do the unpoison and reset
the shadow call stack there — in common code where no architecture can
forget to do it — and still leave idle_thread_get() doing precisely
just the 'get'.

In _cpu_up() the call to idle_thread_get() only happens if the CPU in
question is starting from CPUHP_OFFLINE but I think that's sufficient?

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6c0a92ca6bb5..43e0a77f21e8 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -591,12 +591,6 @@ static int bringup_cpu(unsigned int cpu)
 	struct task_struct *idle = idle_thread_get(cpu);
 	int ret;

-	/*
-	 * Reset stale stack state from the last time this CPU was online.
-	 */
-	scs_task_reset(idle);
-	kasan_unpoison_task_stack(idle);
-
 	/*
 	 * Some architectures have to walk the irq descriptors to
 	 * setup the vector space for the cpu which comes online.
@@ -1383,6 +1377,12 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
 			ret = PTR_ERR(idle);
 			goto out;
 		}
+
+		/*
+		 * Reset stale stack state from the last time this CPU was online.
+		 */
+		scs_task_reset(idle);
+		kasan_unpoison_task_stack(idle);
 	}

 	cpuhp_tasks_frozen = tasks_frozen;

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)