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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200919052352.GD30063@infradead.org>
Date:   Sat, 19 Sep 2020 06:23:52 +0100
From:   Christoph Hellwig <hch@...radead.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Kashyap Desai <kashyap.desai@...adcom.com>,
        Sumit Saxena <sumit.saxena@...adcom.com>,
        Shivasharan S <shivasharan.srikanteshwara@...adcom.com>,
        "James E . J . Bottomley" <jejb@...ux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Christoph Hellwig <hch@...radead.org>,
        anand.lodnoor@...adcom.com, megaraidlinux.pdl@...adcom.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2 2/3] scsi: megaraid_sas: check user-provided offsets

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@...r.kernel.org> # v2.6.15+
> Fixes: c4a3e0a529ab ("[SCSI] MegaRAID SAS RAID: new driver")
> Reviewed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Arnd Bergmann <arnd@...db.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.