linux-kernel - Re: [PATCH 8/9] x86/virt/tdx: Exclude memory region hole within CMR as TDMR's reserved area

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]

Date: Tue, 18 Jun 2024 12:12:41 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Chao Gao <chao.gao@...el.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <dave.hansen@...el.com>,
	<dan.j.williams@...el.com>, <kirill.shutemov@...ux.intel.com>,
	<rick.p.edgecombe@...el.com>, <peterz@...radead.org>, <tglx@...utronix.de>,
	<bp@...en8.de>, <mingo@...hat.com>, <hpa@...or.com>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <kvm@...r.kernel.org>, <isaku.yamahata@...el.com>,
	<binbin.wu@...ux.intel.com>
Subject: Re: [PATCH 8/9] x86/virt/tdx: Exclude memory region hole within CMR
 as TDMR's reserved area



On 17/06/2024 10:54 pm, Chao Gao wrote:
>> +/* Return whether a given region [start, end) is a sub-region of any CMR */
>> +static bool is_cmr_subregion(struct tdx_sysinfo_cmr_info *cmr_info, u64 start,
>> +			    u64 end)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < cmr_info->num_cmrs; i++) {
>> +		u64 cmr_base = cmr_info->cmr_base[i];
>> +		u64 cmr_size = cmr_info->cmr_size[i];
>> +
>> +		if (start >= cmr_base && end <= (cmr_base + cmr_size))
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> /*
>>   * Go through @tmb_list to find holes between memory areas.  If any of
> 
> The logic here is:
> 1. go through @tmb_list to find holes
> 2. skip a hole if it is in CMRs
> 
> I am wondering if the kernel can traverse CMRs directly to find holes. This
> way, the new is_cmr_subregion() can be removed. And @tmb_list can be dropped
> from a few functions e.g., tdmr_populate_rsvd_holes/areas/areas_all(). So, this
> will simplify those functions a bit.

Traversing CMRs to find reserved areas for a given TDMR sounds good to 
me logically.  The whole construct_tdmrs() assumes all TDX memory blocks 
are fully covered by CMRs anyway.

I'll try this out (validation is a bit tricky because we cannot 
reproduce this issue internally).

> 
>>   * those holes fall within @tdmr, set up a TDMR reserved area to cover
>> @@ -835,7 +932,8 @@ static int tdmr_add_rsvd_area(struct tdmr_info *tdmr, int *p_idx, u64 addr,
>> static int tdmr_populate_rsvd_holes(struct list_head *tmb_list,
>> 				    struct tdmr_info *tdmr,
>> 				    int *rsvd_idx,
>> -				    u16 max_reserved_per_tdmr)
>> +				    u16 max_reserved_per_tdmr,
>> +				    struct tdx_sysinfo_cmr_info *cmr_info)
> 
> Maybe this function can accept a pointer to tdx_sysinfo and remove
> @max_reserved_per_tdmr and @cmr_info because they are both TDX metadata and
> have only one possible combination for a given TDX module. Anyway, I don't have
> a strong opinion on this.

There are pros/cons of the two options.  Passing the @cmr_info and 
@max_reserved_per_tdmr makes this function more clear that it _exactly_ 
requires these two.  Since passing a single @tdx_sysinfo doesn't reduce 
the function arguments a lot (only one argument, and you have to get the 
two inside the function anyway), I would keep the current way unless I 
hear from something different from others.