[PATCH v2 2/3] scsi: megaraid_sas: check user-provided offsets

[PATCH v2 2/3] scsi: megaraid_sas: check user-provided offsets

[Arnd Bergmann]

It sounds unwise to let user space pass an unchecked 32-bit
offset into a kernel structure in an ioctl. This is an unsigned
variable, so checking the upper bound for the size of the structure
it points into is sufficient to avoid data corruption, but as
the pointer might also be unaligned, it has to be written carefully
as well.

While I stumbled over this problem by reading the code, I did not
continue checking the function for further problems like it.

Cc: <stable@vger.kernel.org> # v2.6.15+
Fixes: c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 861f7140f52e..c3de69f3bee8 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8095,7 +8095,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	int error = 0, i;
 	void *sense = NULL;
 	dma_addr_t sense_handle;
-	unsigned long *sense_ptr;
+	void *sense_ptr;
 	u32 opcode = 0;
 	int ret = DCMD_SUCCESS;
 
@@ -8218,6 +8218,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 	}
 
 	if (ioc->sense_len) {
+		/* make sure the pointer is part of the frame */
+		if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {
+			error = -EINVAL;
+			goto out;
+		}
+
 		sense = dma_alloc_coherent(&instance->pdev->dev, ioc->sense_len,
 					     &sense_handle, GFP_KERNEL);
 		if (!sense) {
@@ -8225,12 +8231,11 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
 			goto out;
 		}
 
-		sense_ptr =
-		(unsigned long *) ((unsigned long)cmd->frame + ioc->sense_off);
+		sense_ptr = (void *)cmd->frame + ioc->sense_off;
 		if (instance->consistent_mask_64bit)
-			*sense_ptr = cpu_to_le64(sense_handle);
+			put_unaligned_le64(sense_handle, sense_ptr);
 		else
-			*sense_ptr = cpu_to_le32(sense_handle);
+			put_unaligned_le32(sense_handle, sense_ptr);
 	}
 
 	/*
-- 
2.27.0

Re: [PATCH v2 2/3] scsi: megaraid_sas: check user-provided offsets

[Christoph Hellwig]

On Fri, Sep 18, 2020 at 02:15:22PM +0200, Arnd Bergmann wrote:
> It sounds unwise to let user space pass an unchecked 32-bit
> offset into a kernel structure in an ioctl. This is an unsigned
> variable, so checking the upper bound for the size of the structure
> it points into is sufficient to avoid data corruption, but as
> the pointer might also be unaligned, it has to be written carefully
> as well.
> 
> While I stumbled over this problem by reading the code, I did not
> continue checking the function for further problems like it.
> 
> Cc: <stable@vger.kernel.org> # v2.6.15+
> Fixes: c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver")
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/scsi/megaraid/megaraid_sas_base.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 861f7140f52e..c3de69f3bee8 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -8095,7 +8095,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
>  	int error = 0, i;
>  	void *sense = NULL;
>  	dma_addr_t sense_handle;
> -	unsigned long *sense_ptr;
> +	void *sense_ptr;
>  	u32 opcode = 0;
>  	int ret = DCMD_SUCCESS;
>  
> @@ -8218,6 +8218,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
>  	}
>  
>  	if (ioc->sense_len) {
> +		/* make sure the pointer is part of the frame */
> +		if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {

Add a line break to avoid the overly long line - also the braces
around the arithmetics aren't actually needed.

Re: [PATCH v2 2/3] scsi: megaraid_sas: check user-provided offsets

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver").

The bot has tested the following trees: v5.8.10, v5.4.66, v4.19.146, v4.14.198, v4.9.236, v4.4.236.

v5.8.10: Build OK!
v5.4.66: Build OK!
v4.19.146: Build OK!
v4.14.198: Failed to apply! Possible dependencies:
    107a60dd71b5 ("scsi: megaraid_sas: Add support for 64bit consistent DMA")
    1b4bed206159 ("scsi: megaraid_sas: Create separate functions for allocating and freeing controller DMA buffers")
    201a810cc188 ("scsi: megaraid_sas: Re-Define enum DCMD_RETURN_STATUS")
    2ce435087902 ("scsi: megaraid_sas: Enhance internal DCMD timeout prints")
    7535f27d1f14 ("scsi: megaraid_sas: Move initialization of instance parameters inside newly created function megasas_init_ctrl_params")
    82add4e1b354 ("scsi: megaraid_sas: Incorrect processing of IOCTL frames for SMP/STP commands")
    e5d65b4b81af ("scsi: megaraid_sas: Move controller memory allocations and DMA mask settings from probe to megasas_init_fw")
    e97e673ca63b ("scsi: megaraid_sas: Retry with reduced queue depth when alloc fails for higher QD")

v4.9.236: Failed to apply! Possible dependencies:
    201a810cc188 ("scsi: megaraid_sas: Re-Define enum DCMD_RETURN_STATUS")
    2493c67e518c ("scsi: megaraid_sas: 128 MSIX Support")
    3e5eadb1a881 ("scsi: megaraid_sas: Enable or Disable Fast path based on the PCI Threshold Bandwidth")
    45b8a35eed7b ("scsi: megaraid_sas: 32 bit descriptor fire cmd optimization")
    45f4f2eb3da3 ("scsi: megaraid_sas: Add new pci device Ids for SAS3.5 Generic Megaraid Controllers")
    82add4e1b354 ("scsi: megaraid_sas: Incorrect processing of IOCTL frames for SMP/STP commands")
    8823abeddbbc ("scsi: megaraid_sas: Fix endianness issues in DCMD handling")
    95c060869e68 ("scsi: megaraid_sas: latest controller OCR capability from FW before sending shutdown DCMD")
    d0fc91d67c59 ("scsi: megaraid_sas: Send SYNCHRONIZE_CACHE for VD to firmware")
    f4fc209326c7 ("scsi: megaraid_sas: change issue_dcmd to return void from int")
    fad119b707f8 ("scsi: megaraid_sas: switch to pci_alloc_irq_vectors")

v4.4.236: Failed to apply! Possible dependencies:
    201a810cc188 ("scsi: megaraid_sas: Re-Define enum DCMD_RETURN_STATUS")
    6d40afbc7d13 ("megaraid_sas: MFI IO timeout handling")
    82add4e1b354 ("scsi: megaraid_sas: Incorrect processing of IOCTL frames for SMP/STP commands")
    8823abeddbbc ("scsi: megaraid_sas: Fix endianness issues in DCMD handling")
    8a01a41d8647 ("megaraid_sas: Make adprecovery variable atomic")
    95c060869e68 ("scsi: megaraid_sas: latest controller OCR capability from FW before sending shutdown DCMD")
    f4fc209326c7 ("scsi: megaraid_sas: change issue_dcmd to return void from int")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha