linux-kernel - Re: [PATCH v3 01/21] x86/virt/tdx: Detect SEAM

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: <e50706db-e625-8b91-2e5c-a59cda6478f1@intel.com>
Date:   Wed, 27 Apr 2022 07:22:03 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     seanjc@...gle.com, pbonzini@...hat.com, len.brown@...el.com,
        tony.luck@...el.com, rafael.j.wysocki@...el.com,
        reinette.chatre@...el.com, dan.j.williams@...el.com,
        peterz@...radead.org, ak@...ux.intel.com,
        kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com,
        isaku.yamahata@...el.com
Subject: Re: [PATCH v3 01/21] x86/virt/tdx: Detect SEAM

On 4/26/22 16:49, Kai Huang wrote:
> On Tue, 2022-04-26 at 16:28 -0700, Dave Hansen wrote:
>> What about a dependency?  Isn't this dead code without CONFIG_KVM=y/m?
> 
> Conceptually, KVM is one user of the TDX module, so it doesn't seem correct to
> make CONFIG_INTEL_TDX_HOST depend on CONFIG_KVM.  But so far KVM is the only
> user of TDX, so in practice the code is dead w/o KVM.
> 
> What's your opinion?

You're stuck in some really weird fantasy world.  Sure, we can dream up
more than one user of the TDX module.  But, in the real world, there's
only one.  Plus, code can have multiple dependencies!

	depends on FOO || BAR

This TDX cruft is dead code in today's real-world kernel without KVM.
You should add a dependency.

>>>>> +static bool __seamrr_enabled(void)
>>>>> +{
>>>>> +	return (seamrr_mask & SEAMRR_ENABLED_BITS) == SEAMRR_ENABLED_BITS;
>>>>> +}
>>>>
>>>> But there's no case where seamrr_mask is non-zero and where
>>>> _seamrr_enabled().  Why bother checking the SEAMRR_ENABLED_BITS?
>>>
>>> seamrr_mask will only be non-zero when SEAMRR is enabled by BIOS, otherwise it
>>> is 0.  It will also be cleared when BIOS mis-configuration is detected on any
>>> AP.  SEAMRR_ENABLED_BITS is used to check whether SEAMRR is enabled.
>>
>> The point is that this could be:
>>
>> 	return !!seamrr_mask;
> 
> The definition of this SEAMRR_MASK MSR defines "ENABLED" and "LOCKED" bits. 
> Explicitly checking the two bits, instead of !!seamrr_mask roles out other
> incorrect configurations.  For instance, we should not treat SEAMRR being
> enabled if we only have "ENABLED" bit set or "LOCKED" bit set.

You're confusing two different things:
 * The state of the variable
 * The actual correct hardware state

The *VARIABLE* can't be non-zero and also denote that SEAMRR is enabled.
 Does this *CODE* ever set ENABLED or LOCKED without each other?

>>>>> +static void detect_seam_ap(struct cpuinfo_x86 *c)
>>>>> +{
>>>>> +	u64 base, mask;
>>>>> +
>>>>> +	/*
>>>>> +	 * Don't bother to detect this AP if SEAMRR is not
>>>>> +	 * enabled after earlier detections.
>>>>> +	 */
>>>>> +	if (!__seamrr_enabled())
>>>>> +		return;
>>>>> +
>>>>> +	rdmsrl(MSR_IA32_SEAMRR_PHYS_BASE, base);
>>>>> +	rdmsrl(MSR_IA32_SEAMRR_PHYS_MASK, mask);
>>>>> +
>>>>
>>>> This is the place for a comment about why the values have to be equal.
>>>
>>> I'll add below:
>>>
>>> /* BIOS must configure SEAMRR consistently across all cores */
>>
>> What happens if the BIOS doesn't do this?  What actually breaks?  In
>> other words, do we *NEED* error checking here?
> 
> AFAICT the spec doesn't explicitly mention what will happen if BIOS doesn't
> configure them consistently among cores.  But for safety I think it's better to
> detect.

Safety?  Safety of what?

>>>>> +void tdx_detect_cpu(struct cpuinfo_x86 *c)
>>>>> +{
>>>>> +	detect_seam(c);
>>>>> +}
>>>>
>>>> The extra function looks a bit silly here now.  Maybe this gets filled
>>>> out later, but it's goofy-looking here.
>>>
>>> Thomas suggested to put all TDX detection related in one function call, so I
>>> added tdx_detect_cpu().  I'll move this to the next patch when detecting TDX
>>> KeyIDs.
>>
>> That's fine, or just add a comment or a changelog sentence about this
>> being filled out later.
> 
> There's already one sentence in the changelog:
> 
> "......Add a function to detect all TDX preliminaries (SEAMRR, TDX private
> KeyIDs) for a given cpu when it is brought up.  As the first step, detect the
> validity of SEAMRR."
> 
> Does this look good to you?

No, that doesn't provide enough context.

There are two single-line wrapper functions.  One calls the other.  That
looks entirely silly in this patch.  You need to explain the silliness,
explicitly.