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: <aBcxjsdC4tsIgIf2@gmail.com>
Date: Sun, 4 May 2025 11:21:18 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Ashish Kalra <Ashish.Kalra@....com>
Cc: tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
	x86@...nel.org, bp@...en8.de, thomas.lendacky@....com,
	hpa@...or.com, michael.roth@....com, nikunj@....com,
	seanjc@...gle.com, ardb@...nel.org, stable@...r.kernel.org,
	linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
	linux-coco@...ts.linux.dev
Subject: Re: [PATCH v4] x86/sev: Fix making shared pages private during kdump


* Ashish Kalra <Ashish.Kalra@....com> wrote:

>  
> -			if (addr <= ghcb && ghcb <= addr + size) {
> +			/* Handle the case of a huge page containing the GHCB page */
> +			if (addr <= ghcb && ghcb < addr + size) {
>  				skipped_addr = true;
>  				break;
>  			}
> @@ -1131,9 +1132,8 @@ static void shutdown_all_aps(void)
>  void snp_kexec_finish(void)
>  {
>  	struct sev_es_runtime_data *data;
> +	unsigned long size, mask, ghcb;
>  	unsigned int level, cpu;
> -	unsigned long size;
> -	struct ghcb *ghcb;

So this patch just morphs the type of 'ghcb' from a typed pointer to 
unsigned long, while most 'ghcb' uses in coco/ are typed pointers?

That's just sloppy and fragile. Please just keep 'ghcb' a typed 
pointer, and introduce *another* variable for the virtual address to 
the hugepage.

>  	pte_t *pte;
>  
>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> @@ -1157,11 +1157,14 @@ void snp_kexec_finish(void)
>  
>  	for_each_possible_cpu(cpu) {
>  		data = per_cpu(runtime_data, cpu);
> -		ghcb = &data->ghcb_page;
> -		pte = lookup_address((unsigned long)ghcb, &level);
> +		ghcb = (unsigned long)&data->ghcb_page;

If 'ghcb' has the proper type then this ugly forced type-cast goes 
away.

> +		pte = lookup_address(ghcb, &level);
>  		size = page_level_size(level);
> +		mask = page_level_mask(level);
> +		/* Handle the case of a huge page containing the GHCB page */
> +		ghcb &= mask;

This too calls for using a separate variable for this, because after 
this masking 'ghcb' is very much *not* the location of a GHCB page 
anymore...

>  		set_pte_enc(pte, level, (void *)ghcb);
> -		snp_set_memory_private((unsigned long)ghcb, (size / PAGE_SIZE));
> +		snp_set_memory_private(ghcb, (size / PAGE_SIZE));

Do we know whether this is safe? Could the huge page around the GHCB 
page contain anything else? What is the structure of this memory area, 
is it all dedicated to the GHCB, or could it contain random other data?

Thanks,

	Ingo

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ