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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ