Re: [PATCH v3 3/4] crypto: ccp: Fix possible deadlock in SEV init failure path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Failure in `rmp_mark_pages_firmware()` causes `__snp_alloc_firmware_pages()` to leak memory and return NULL, leading to a cascade invalid pointer dereference.
- [High] Bounce buffers and firmware-owned pages are permanently leaked when legacy command buffer mapping or preparation fails.
- [High] Command buffer active state flags are leaked on error paths, leading to permanent `-EBUSY` exhaustion for all SEV firmware commands.
--

commit 1310cc2bc448794800c5bf6fa91a6d1bce53eafb
Author: Atish Patra <atishp@meta.com>

crypto: ccp: Fix possible deadlock in SEV init failure path
    
__sev_platform_init_handle_init_ex_path() calls
rmp_mark_pages_firmware() with locked=false while the parent
function of init_ex_path already acquired the sev_cmd_mutex.
In the case of an RMPUPDATE failure for any page after the first, the cleanup
path would invoke reclaim pages which would result in a deadlock in
sev_do_cmd.

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index d1e9e0ac63b60..3d4793e8e34b2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1557,7 +1557,7 @@ static int __sev_platform_init_handle_init_ex_path(struct sev_device *sev)
>  		unsigned long npages;
>  
>  		npages = 1UL << get_order(NV_LENGTH);
> -		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, false)) {
> +		if (rmp_mark_pages_firmware(__pa(sev_init_ex_buffer), npages, true)) {
>  			dev_err(sev->dev, "SEV: INIT_EX NV memory page state change failed.\n");
>  			return -ENOMEM;
>  		}

[Severity: High]
This is a pre-existing issue, but does __snp_alloc_firmware_pages() leak memory
and return NULL if rmp_mark_pages_firmware() fails?

drivers/crypto/ccp/sev-dev.c:__snp_alloc_firmware_pages()
		if (rmp_mark_pages_firmware(paddr, npages, locked))
			return NULL;

The function appears to return NULL without freeing the allocated page.

Additionally, in __sev_snp_init_locked(), the result of this function is
passed directly to page_address():

drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()
		void *tio_status = page_address(__snp_alloc_firmware_pages(
			GFP_KERNEL_ACCOUNT | __GFP_ZERO, 0, true));

		if (tio_status) {
			sev_tsm_init_locked(sev, tio_status);

Since page_address() applies pointer arithmetic, passing NULL to it results
in a non-NULL invalid virtual address. This bypasses the subsequent
if (tio_status) check, causing a crash upon dereference in
sev_tsm_init_locked(). Can we add a check for the NULL return value before
calling page_address() and ensure the memory is freed on failure?

[Severity: High]
This isn't a bug introduced by this patch, but are bounce buffers and
firmware-owned pages permanently leaked when legacy command buffer mapping or
preparation fails?

In snp_map_cmd_buf_desc():
	if (rmp_mark_pages_firmware(*desc->paddr_ptr, npages, true)) {
		pr_warn("Error moving pages to firmware-owned state for SEV legacy command.\n");
		return -EFAULT;
	}

The function returns without calling __free_pages() for the allocated bounce
buffer.

Also, in snp_map_cmd_buf_desc_list():
err_unmap:
	for (i--; i >= 0; i--)
		snp_unmap_cmd_buf_desc(&desc_list[i]);

The error unmap loop decrements i before unmapping, skipping the descriptor
that just failed and leaking its buffer.

Furthermore, in snp_prep_cmd_buf():
		if (rmp_mark_pages_firmware(__pa(cmd_buf), 1, true))
			return -EFAULT;

If this fails, the function returns -EFAULT immediately without calling
snp_unmap_cmd_buf_desc_list(). All previously mapped descriptor pages remain
permanently locked in the firmware-owned state, and their bounce buffers are
leaked.

[Severity: High]
This is a pre-existing issue, but could command buffer active state flags leak
on error paths in __sev_do_cmd_locked()?

drivers/crypto/ccp/sev-dev.c:__sev_do_cmd_locked()
		ret = snp_prep_cmd_buf(cmd, cmd_buf, desc_list);
		if (ret) {
			dev_err(sev->dev,
				"SEV: failed to prepare buffer for legacy command 0x%x. Error: %d\n",
				cmd, ret);
			return ret;
		}

If snp_prep_cmd_buf() fails, the function returns immediately without rolling
back the cmd_buf_active and cmd_buf_backup_active lock tracking variables.

After two such failures, both command buffer slots could be permanently
marked as active, leading to all future firmware commands failing with -EBUSY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-sev_snp_fixes-v3-0-24bfd3ae047c@meta.com?part=3