Re: [PATCH v7 04/17] nvmet: Implement CCR logpage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvmet: uninitialized SGL tail leak in
  nvmet_execute_get_log_page_ccr()
- [Medium] nvmet: premature CCR cleanup before SGL copy in
  nvmet_execute_get_log_page_ccr()
--

commit bd4c242dbfa9b91e0bb3eb6e57be8d11c6a19d19
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvmet: Implement CCR logpage

This commit implements the Cross-Controller Reset (CCR) log page (LID 0x1e)
for the NVMe target and advertises it in the Supported Log Pages log.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 3df9db454db9c..25c320752d2a7 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c

[ ... ]

> @@ -617,6 +618,46 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +static void nvmet_execute_get_log_page_ccr(struct nvmet_req *req)
> +{
> +	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvmet_ccr *ccr;
> +	struct nvme_ccr_log *log;
> +	int index = 0;
> +	u16 status;
> +
> +	log = kzalloc_obj(*log);
> +	if (!log) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	mutex_lock(&ctrl->lock);
> +	list_for_each_entry(ccr, &ctrl->ccr_list, entry) {
> +		u8 flags = NVME_CCR_FLAGS_VALIDATED | NVME_CCR_FLAGS_INITIATED;
> +		u8 ccr_status = ccr->ctrl ? NVME_CCR_STATUS_IN_PROGRESS :
> +					NVME_CCR_STATUS_SUCCESS;
> +
> +		log->entries[index].icid = cpu_to_le16(ccr->icid);
> +		log->entries[index].ciu = ccr->ciu;
> +		log->entries[index].acid = cpu_to_le16(0xffff);
> +		log->entries[index].ccrs = ccr_status;
> +		log->entries[index].ccrf = flags;
> +		index++;
> +	}
> +
> +	/* Cleanup completed CCRs if requested */
> +	if (req->cmd->get_log_page.lsp & 0x1)
> +		nvmet_ctrl_cleanup_ccrs(ctrl, false);
> +	mutex_unlock(&ctrl->lock);

[Severity: Medium]
Can nvmet_ctrl_cleanup_ccrs() discard completed CCR entries when the log
page fails to copy to the host?

nvmet_execute_get_log_page() only checks that req->transfer_len matches the
command's NUMDL/NUMDU length.  If a host requests fewer than sizeof(*log)
bytes (for example, reading the 8-byte header) with LSP bit 0 set,
nvmet_ctrl_cleanup_ccrs() frees all completed entries from ctrl->ccr_list
before nvmet_copy_to_sgl() fails with NVME_SC_SGL_INVALID_DATA, so the host
permanently loses those completion records.

> +
> +	log->ne = cpu_to_le16(index);
> +	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));

[Severity: High]
Does this leak uninitialized target kernel memory when the host requests
more than sizeof(*log) bytes?

Transports allocate req->sg for req->transfer_len bytes via sgl_alloc()
with GFP_KERNEL (without __GFP_ZERO).  Because nvmet_execute_get_log_page()
only checks that req->transfer_len matches NUMDL/NUMDU and
nvmet_execute_get_log_page_ccr() neither rejects req->transfer_len !=
sizeof(*log) (like nvmet_execute_get_log_page_rmi() and
nvmet_execute_get_log_page_smart()) nor zeroes trailing SGL bytes,
requesting more than 4096 bytes copies sizeof(*log) at offset 0 and returns
the remaining uninitialized scatterlist pages to the host.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=4