linux-kernel - Re: [PATCH] arm64/cpuinfo: Drop boot_cpu

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] [day] [month] [year] [list]

Message-ID: <20200504155013.GG73375@C02TD0UTHF1T.local>
Date:   Mon, 4 May 2020 16:50:13 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Mark Brown <broonie@...nel.org>,
        Suzuki Poulose <suzuki.poulose@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64/cpuinfo: Drop boot_cpu_data

On Mon, May 04, 2020 at 08:23:08PM +0530, Anshuman Khandual wrote:
> 
> 
> On 05/04/2020 06:13 PM, Mark Rutland wrote:
> > On Mon, May 04, 2020 at 06:00:00PM +0530, Anshuman Khandual wrote:
> >> A global boot_cpu_data is not really required. Lets drop this.
> > 
> > I don't think it's true that this isn't required today.
> > 
> > One reason that we have both boot_cpu_data and a cpu_data variable for
> > CPU0 is that CPU0 itself can be hotplugged out then back in, and this
> > allows us to detect if CPU0's features have changed (e.g. due to FW
> > failing to configure it appropriately, or real physical hotplug
> > occurring).
> 
> Understood. After hotplug, CPU0 will come back via secondary_start_kernel()
> where it's current register values will be checked against earlier captured
> values i.e boot_cpu_data.
> 
> But wondering why should CPU0 be treated like any other secondary CPU. IOW
> in case the fresh boot CPU register values dont match with boot_cpu_data,
> should not the online process just be declined ? AFAICS, current approach
> will let the kernel run with taint in case of a mismatch.

I don't follow. When CPU0 is hotplguged back in it'll follow the
secondary boot path, so it can be rejected as with any other secondary
CPU.

If I'm missing a case, could you please point that out more
specifically?

> > So NAK to the patch as it stands. If we're certain we capture all of
> > those details even without boot_cpu_data, then we should make other
> > changes to make that clear (e.g. removing it as an argument to
> > update_cpu_features()).
> 
> There might not be another way, unless we can override CPU0's cpu_data
> variable when the boot CPU comes back in after vetting against existing
> values. Is there any particular reason to store the very first boot CPU0
> info for ever ?

The reason is so that we can log the values for comparison. Otherwise
we'll have to choose some arbitrary CPU's value in order to do so.

> Passing on CPU0's cpu_data variable in update_cpu_features() for secondary
> CPUs during boot still make sense. It helps in finalizing register values.
> Re-entering CPU0's test against boot_cpu_data seems different.

I think that practically this means we should leave this as-is. If we
need to keep it around for CPU, then we may as well keep it around and
use it consitently for all secondary CPUs.

I'd prefer to leave this as-is given it's simple to reason about.

Thanks,
Mark.