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

From: Christoph Hellwig <hch@lst.de>
To: Arnd Bergmann <arnd@kernel.org>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>,
	Sumit Saxena <sumit.saxena@broadcom.com>,
	Shivasharan S <shivasharan.srikanteshwara@broadcom.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Arnd Bergmann <arnd@arndb.de>,
	stable@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Anand Lodnoor <anand.lodnoor@broadcom.com>,
	Hannes Reinecke <hare@suse.de>,
	megaraidlinux.pdl@broadcom.com, linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] scsi: megaraid_sas: check user-provided offsets
Date: Tue, 3 Nov 2020 09:55:48 +0100	[thread overview]
Message-ID: <20201103085548.GB14092@lst.de> (raw)
In-Reply-To: <20201030164450.1253641-2-arnd@kernel.org>

On Fri, Oct 30, 2020 at 05:44:20PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 41cd66fc7d81..b1b9a8823c8c 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -8134,7 +8134,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;
>  
> @@ -8257,6 +8257,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;

This still has the overly long line.