Re: [PATCH v1 2/4] smartpqi: add support for CCISS_BIG_PASSTHRU ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbounded `kmalloc` of user-controlled size prevents large transfers and triggers page allocation warnings.
- [High] Missing 32-bit compat ioctl translation for `CCISS_BIG_PASSTHRU`.
--

--- Patch [2]: [PATCH v1 2/4] smartpqi: add support for CCISS_BIG_PASSTHRU ioctl ---
commit 89c5d7119f80d6cae47c460c85b511017c1cd263
Author: David Strahan <David.Strahan@microchip.com>

smartpqi: add support for CCISS_BIG_PASSTHRU ioctl
    
This commit adds pqi_big_passthru_ioctl() to handle CCISS_BIG_PASSTHRU ioctl
requests. The new ioctl uses BIG_IOCTL_Command_struct which stores the buffer
size as a 32-bit integer, allowing larger transfers than the previous 64KB
limit.

> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 3a75b9fbedf45..473f1c9e47380 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c

[ ... ]

> @@ -6935,6 +6935,144 @@ static int pqi_passthru_ioctl(struct pqi_ctrl_info *ctrl_info, void __user *arg)
>  	return rc;
>  }
>  
> +static int pqi_big_passthru_ioctl(struct pqi_ctrl_info *ctrl_info, void __user *arg)
> +{

[ ... ]

> +	if (iocommand.buf_size > 0) {
> +		kernel_buffer = kmalloc(iocommand.buf_size, GFP_KERNEL);

[Severity: High]
Will this trigger page allocation warnings for large transfers?

Since iocommand.buf_size is a user-controlled 32-bit integer passed directly
here, a large allocation without __GFP_NOWARN could trigger a page allocation
warning if it exceeds MAX_PAGE_ORDER. This might panic systems that have 
panic_on_warn set.

> +		if (!kernel_buffer)
> +			return -ENOMEM;

[ ... ]

> +	if (iocommand.buf_size > 0) {
> +		put_unaligned_le32(iocommand.buf_size, &request.buffer_length);
> +
> +		rc = pqi_map_single(ctrl_info->pci_dev,
> +			&request.sg_descriptors[0], kernel_buffer,
> +			iocommand.buf_size, DMA_BIDIRECTIONAL);

[Severity: High]
Does this prevent the driver from handling the large chunked transfers this
ioctl was designed for?

Since the entire buffer is mapped to a single scatter-gather descriptor here
(sg_descriptors[0]), it seems to require physically contiguous memory from
the earlier kmalloc() call, rather than utilizing scatter-gather arrays for
large payloads. Memory fragmentation could cause large transfer requests to fail.

> +		if (rc)
> +			goto out;

[ ... ]

> @@ -6958,6 +7096,12 @@ static int pqi_ioctl(struct scsi_device *sdev, unsigned int cmd,
>  	case CCISS_PASSTHRU:
>  		rc = pqi_passthru_ioctl(ctrl_info, arg);
>  		break;
> +	case CCISS_BIG_PASSTHRU:
> +		rc = pqi_big_passthru_ioctl(ctrl_info, arg);
> +		break;
> +	case CCISS_BIG_PASSTHRU_SUPPORTED:
> +		rc = 0;
> +		break;
>  	default:
>  		rc = -EINVAL;

[Severity: High]
Is a compat_ioctl handler needed for 32-bit applications?

BIG_IOCTL_Command_struct contains a trailing pointer (BYTE __user *buf), so
its size varies between 32-bit and 64-bit architectures (88 vs 96 bytes).
Because the _IOWR macro embeds the structure size in the ioctl command
number, a 32-bit application will emit a different command number
(e.g., CCISS_BIG_PASSTHRU32) than the 64-bit kernel expects.

Since there is no compat_ioctl callback for the smartpqi driver, will the
32-bit ioctl fall through to this default -EINVAL case?

Interestingly, since CCISS_BIG_PASSTHRU_SUPPORTED uses the _IO macro, it
is size-independent, so 32-bit utilities might successfully probe for
support but fail when actually issuing the transfer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722220401.6357-1-david.strahan@microchip.com?part=2