Re: [PATCH v3 2/3] scsi: target: rd: Release protection space on allocation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] target: UAF in rd_build_prot_space() error path during concurrent configfs writes
--

commit d01a55deaeecc1f16dadcdc415bd1b576984c0da
Author: Sanan Hasanov <sanan.hasanov@ucf.edu>

scsi: target: rd: Release protection space on allocation failure

When rd_allocate_sgl_table() fails, rd_build_prot_space() returns the error
but leaves the partially populated sg_prot_array in place. This commit adds
a call to rd_release_prot_space() before returning the error to prevent
leaking the previous allocation.

> diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> index 97c63b12266fb..b73bdd733928e 100644
> --- a/drivers/target/target_core_rd.c
> +++ b/drivers/target/target_core_rd.c
> @@ -263,8 +263,10 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length, int block
>  	rd_dev->sg_prot_count = sg_tables;
>  
>  	rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);
> -	if (rc)
> +	if (rc) {
> +		rd_release_prot_space(rd_dev);

[Severity: High]
Could this introduce a use-after-free if triggered by concurrent configfs
writes?

Looking at pi_prot_type_store() in drivers/target/target_core_configfs.c,
there appears to be no locking before checking old_prot and calling
dev->transport->init_prot(dev):

	int old_prot = da->pi_prot_type, ret;
	struct se_device *dev = da->da_dev;
	...
	da->pi_prot_type = flag;

	if (flag && !old_prot) {
		ret = dev->transport->init_prot(dev);

If multiple threads write to pi_prot_type concurrently, they can both enter
rd_build_prot_space() and concurrently overwrite the global
rd_dev->sg_prot_array pointer:

	rd_dev->sg_prot_array = sg_table;
	rd_dev->sg_prot_count = sg_tables;

	rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);

If one thread's allocation fails, it executes this new error path which
reads rd_dev->sg_prot_array and frees it. This could free the active
scatterlist array that is currently being populated by the other racing
thread, causing a use-after-free of the scatterlist table and pages.

>  		return rc;
> +	}
>  
>  	pr_debug("CORE_RD[%u] - Built Ramdisk Device ID: %u prot space of"
>  		 " %u pages in %u tables\n", rd_dev->rd_host->rd_host_id,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008235411.56641-1-sanan.hasanou@gmail.com?part=2