linux-kernel - Re: [PATCH] scsi: myrs: avoid stack overflow warning

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 for Android: free password hash cracker in your pocket

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

Date:   Sat, 3 Nov 2018 09:47:34 +0100
From:   Hannes Reinecke <hare@...e.de>
To:     Arnd Bergmann <arnd@...db.de>, Hannes Reinecke <hare@...nel.org>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>
Cc:     James Bottomley <James.Bottomley@...senPartnership.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: myrs: avoid stack overflow warning

On 11/2/18 4:44 PM, Arnd Bergmann wrote:
> Putting a 1024 byte data structure on the stack is generally a bad idea.
> On 32-bit systems, it also triggers a compile-time warning when building
> with -Og:
> 
> drivers/scsi/myrs.c: In function 'myrs_get_ctlr_info':
> drivers/scsi/myrs.c:212:1: error: the frame size of 1028 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 
> We only really need three members of the structure, so just read them
> manually here instead of copying the entire structure.
> 
> Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>   drivers/scsi/myrs.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
> index 0264a2e2bc19..b8d54ef8cf6d 100644
> --- a/drivers/scsi/myrs.c
> +++ b/drivers/scsi/myrs.c
> @@ -163,9 +163,12 @@ static unsigned char myrs_get_ctlr_info(struct myrs_hba *cs)
>   	dma_addr_t ctlr_info_addr;
>   	union myrs_sgl *sgl;
>   	unsigned char status;
> -	struct myrs_ctlr_info old;
> +	unsigned short ldev_present, ldev_critical, ldev_offline;
> +
> +	ldev_present = cs->ctlr_info->ldev_present;
> +	ldev_critical = cs->ctlr_info->ldev_critical;
> +	ldev_offline = cs->ctlr_info->ldev_offline;
>   
> -	memcpy(&old, cs->ctlr_info, sizeof(struct myrs_ctlr_info));
>   	ctlr_info_addr = dma_map_single(&cs->pdev->dev, cs->ctlr_info,
>   					sizeof(struct myrs_ctlr_info),
>   					DMA_FROM_DEVICE);
> @@ -198,9 +201,9 @@ static unsigned char myrs_get_ctlr_info(struct myrs_hba *cs)
>   		    cs->ctlr_info->rbld_active +
>   		    cs->ctlr_info->exp_active != 0)
>   			cs->needs_update = true;
> -		if (cs->ctlr_info->ldev_present != old.ldev_present ||
> -		    cs->ctlr_info->ldev_critical != old.ldev_critical ||
> -		    cs->ctlr_info->ldev_offline != old.ldev_offline)
> +		if (cs->ctlr_info->ldev_present != ldev_present ||
> +		    cs->ctlr_info->ldev_critical != ldev_critical ||
> +		    cs->ctlr_info->ldev_offline != ldev_offline)
>   			shost_printk(KERN_INFO, cs->host,
>   				     "Logical drive count changes (%d/%d/%d)\n",
>   				     cs->ctlr_info->ldev_critical,
> 
Reviewed-by: Hannes Reinecke <hare@...e.com>

Cheers,

Hannes