Re: [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent updates to `sdp->sector_size` can cause `sd_uninit_command()` to free a payload page to the wrong mempool.
--

commit ba587f7eceae0f0ec53afb6d48daeb8d25456542
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>

scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

sd_set_special_bvec() allocates a special payload page for UNMAP and
WRITE SAME commands.  If scsi_alloc_sgtables() fails afterward in
sd_setup_unmap_cmnd() or sd_setup_write_same{10,16}_cmnd(), the SCSI
midlayer does not call uninit_command() because RQF_DONTPREP is not
set yet, leaking the page.

Call sd_uninit_command() on error, and clear RQF_SPECIAL_PAYLOAD after
freeing the page.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b096ea237f14a..6f05e7683df6d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -958,6 +958,21 @@ static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
>  	return protect;
>  }
>  
> +static void sd_uninit_command(struct scsi_cmnd *cmd)
> +{
> +	struct request *rq = scsi_cmd_to_rq(cmd);
> +	struct scsi_device *sdp = cmd->device;
> +
> +	if (!(rq->rq_flags & RQF_SPECIAL_PAYLOAD))
> +		return;
> +
> +	if (sdp->sector_size > PAGE_SIZE)
> +		mempool_free(rq->special_vec.bv_page, sd_large_page_pool);
> +	else
> +		mempool_free(rq->special_vec.bv_page, sd_page_pool);

[Severity: High]
This is a pre-existing issue, but can checking sdp->sector_size here cause a
page to be freed into the wrong mempool if the sector size changes
concurrently?

In sd_set_special_bvec(), the page is allocated based on sdp->sector_size:

	if (sector_size > PAGE_SIZE)
		page = mempool_alloc(sd_large_page_pool, GFP_ATOMIC);

Concurrently, sd_revalidate_disk() calls sd_read_capacity(), which updates
sdkp->device->sector_size directly before the block layer queue is frozen:

	sdkp->device->sector_size = sector_size;

If sdp->sector_size is modified and crosses the PAGE_SIZE boundary while an
UNMAP or WRITE SAME request is in flight, could sd_uninit_command() read the
new size and free the page to the wrong pool? This might be reachable by a
malicious USB device returning a different sector size upon a media change.
Since sd_large_page_pool uses high-order allocations, freeing an order-0 page
into it could corrupt the page allocator and cause subsequent allocations to
write out of bounds.

> +	rq->rq_flags &= ~RQF_SPECIAL_PAYLOAD;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623100159.4018066-1-yangxiuwei@kylinos.cn?part=3