Re: [PATCH v2] nvmet: preserve ANA log page copy errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmet: skipped partial ANA descriptor copy in nvmet_execute_get_log_page_ana()
--

commit d3533edc412b03d8943fd7010d72c42247b5d211
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

nvmet: preserve ANA log page copy errors

This commit preserves descriptor-copy and allocation errors in
nvmet_execute_get_log_page_ana() instead of overwriting status when copying
the ANA log page header, and skips copying ANA group descriptors when their
length exceeds the remaining transfer length.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 26028be8fe4d8..b0bcfbc94b930 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -542,17 +542,23 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	u32 grpid;
>  	u16 ngrps = 0;
>  	u16 status;
> +	u16 header_status;
>  
>  	status = NVME_SC_INTERNAL;
>  	desc = kmalloc_flex(*desc, nsids, NVMET_MAX_NAMESPACES);
>  	if (!desc)
>  		goto out;
>  
> +	status = NVME_SC_SUCCESS;
> +
>  	down_read(&nvmet_ana_sem);
>  	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
>  		if (!nvmet_ana_group_enabled[grpid])
>  			continue;
>  		len = nvmet_format_ana_group(req, grpid, desc);
> +		if (offset > req->transfer_len || len > req->transfer_len - offset)
> +			break;
> +
>  		status = nvmet_copy_to_sgl(req, offset, desc, len);
>  		if (status)
>  			break;
> @@ -572,7 +578,9 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	kfree(desc);
>  
>  	/* copy the header last once we know the number of groups */
> -	status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
> +	header_status = nvmet_copy_to_sgl(req, 0, &hdr, sizeof(hdr));
> +	if (!status)
> +		status = header_status;

[Severity: High]
As reported by Sashiko on v1 [1], in nvmet_execute_get_log_page_ana() in
drivers/nvme/target/admin-cmd.c, when a host issues a Get Log Page command
for NVME_LOG_ANA with req->transfer_len ending partway through an enabled ANA
group descriptor (offset < req->transfer_len < offset + len), does breaking
out of the loop before calling nvmet_copy_to_sgl() leave the trailing
[offset, req->transfer_len) portion of req->sg unwritten?

In drivers/nvme/target/core.c, nvmet_copy_to_sgl() wraps
sg_pcopy_from_buffer():

u16 nvmet_copy_to_sgl(struct nvmet_req *req, off_t off, const void *buf,
		size_t len)
{
	if (sg_pcopy_from_buffer(req->sg, req->sg_cnt, buf, len, off) != len) {
		req->error_loc = offsetof(struct nvme_common_command, dptr);
		return NVME_SC_SGL_INVALID_DATA | NVME_STATUS_DNR;
	}
	return 0;
}

Previously, when offset < req->transfer_len and offset + len >
req->transfer_len, sg_pcopy_from_buffer() still copied the first
req->transfer_len - offset bytes of desc to fill req->sg up to
req->transfer_len before returning a short count, and the final header copy
at offset 0 reset status to 0.

Because nvmet transports allocate req->sg with capacity req->transfer_len
without zeroing it and transmit the full req->transfer_len on completion,
skipping nvmet_copy_to_sgl() when len > req->transfer_len - offset omits the
partial descriptor bytes that fit in the buffer and exposes req->transfer_len
- offset bytes of uninitialized kernel memory to the host.

Also, since req->sg has total capacity req->transfer_len,
sg_pcopy_from_buffer() only returns less than len when off + len exceeds
req->transfer_len. With the pre-check ensuring offset + len <=
req->transfer_len, can nvmet_copy_to_sgl(req, offset, desc, len) ever return
a non-zero status here?

[1] https://lore.kernel.org/r/20261031083505.4767-1-sreekuttan2156239@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031093420.3017-1-sreekuttan2156239@gmail.com?part=1