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  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:   Fri, 26 Mar 2021 11:30:26 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Kees Cook <keescook@...omium.org>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] media: siano: Fix multiple out-of-bounds warnings
 in smscore_load_firmware_family2()

Hi all,

Friendly ping: who can take this, please?

Thanks
--
Gustavo

On 3/10/21 20:19, Gustavo A. R. Silva wrote:
> Rename struct sms_msg_data4 to sms_msg_data5 and increase the size of
> its msg_data array from 4 to 5 elements. Notice that at some point
> the 5th element of msg_data is being accessed in function
> smscore_load_firmware_family2():
> 
> 1006                 trigger_msg->msg_data[4] = 4; /* Task ID */
> 
> Also, there is no need for the object _trigger_msg_ of type struct
> sms_msg_data *, when _msg_ can be used, directly. Notice that msg_data
> in struct sms_msg_data is a one-element array, which causes multiple
> out-of-bounds warnings when accessing beyond its first element
> in function smscore_load_firmware_family2():
> 
>  992                 struct sms_msg_data *trigger_msg =                                                  
>  993                         (struct sms_msg_data *) msg;                                                
>  994                                                                                                     
>  995                 pr_debug("sending MSG_SMS_SWDOWNLOAD_TRIGGER_REQ\n");                               
>  996                 SMS_INIT_MSG(&msg->x_msg_header,                                                    
>  997                                 MSG_SMS_SWDOWNLOAD_TRIGGER_REQ,                                     
>  998                                 sizeof(struct sms_msg_hdr) +                                        
>  999                                 sizeof(u32) * 5);                                                   
> 1000                                                                                                     
> 1001                 trigger_msg->msg_data[0] = firmware->start_address;                                 
> 1002                                         /* Entry point */                                           
> 1003                 trigger_msg->msg_data[1] = 6; /* Priority */                                        
> 1004                 trigger_msg->msg_data[2] = 0x200; /* Stack size */                                  
> 1005                 trigger_msg->msg_data[3] = 0; /* Parameter */                                       
> 1006                 trigger_msg->msg_data[4] = 4; /* Task ID */ 
> 
> even when enough dynamic memory is allocated for _msg_:
> 
>  929         /* PAGE_SIZE buffer shall be enough and dma aligned */
>  930         msg = kmalloc(PAGE_SIZE, GFP_KERNEL | coredev->gfp_buf_flags);
> 
> but as _msg_ is casted to (struct sms_msg_data *):
> 
>  992                 struct sms_msg_data *trigger_msg =
>  993                         (struct sms_msg_data *) msg;
> 
> the out-of-bounds warnings are actually valid and should be addressed.
> 
> Fix this by declaring object _msg_ of type struct sms_msg_data5 *,
> which contains a 5-elements array, instead of just 4. And use
> _msg_ directly, instead of creating object trigger_msg.
> 
> This helps with the ongoing efforts to enable -Warray-bounds by fixing
> the following warnings:
> 
>   CC [M]  drivers/media/common/siano/smscoreapi.o
> drivers/media/common/siano/smscoreapi.c: In function ‘smscore_load_firmware_family2’:
> drivers/media/common/siano/smscoreapi.c:1003:24: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>  1003 |   trigger_msg->msg_data[1] = 6; /* Priority */
>       |   ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
>   619 |  u32 msg_data[1];
>       |      ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1004:24: warning: array subscript 2 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>  1004 |   trigger_msg->msg_data[2] = 0x200; /* Stack size */
>       |   ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
>   619 |  u32 msg_data[1];
>       |      ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1005:24: warning: array subscript 3 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>  1005 |   trigger_msg->msg_data[3] = 0; /* Parameter */
>       |   ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
>   619 |  u32 msg_data[1];
>       |      ^~~~~~~~
> drivers/media/common/siano/smscoreapi.c:1006:24: warning: array subscript 4 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>  1006 |   trigger_msg->msg_data[4] = 4; /* Task ID */
>       |   ~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from drivers/media/common/siano/smscoreapi.c:12:
> drivers/media/common/siano/smscoreapi.h:619:6: note: while referencing ‘msg_data’
>   619 |  u32 msg_data[1];
>       |      ^~~~~~~~
> 
> Fixes: 018b0c6f8acb ("[media] siano: make load firmware logic to work with newer firmwares")
> Co-developed-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  drivers/media/common/siano/smscoreapi.c | 22 +++++++++-------------
>  drivers/media/common/siano/smscoreapi.h |  4 ++--
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
> index 410cc3ac6f94..bceaf91faa15 100644
> --- a/drivers/media/common/siano/smscoreapi.c
> +++ b/drivers/media/common/siano/smscoreapi.c
> @@ -908,7 +908,7 @@ static int smscore_load_firmware_family2(struct smscore_device_t *coredev,
>  					 void *buffer, size_t size)
>  {
>  	struct sms_firmware *firmware = (struct sms_firmware *) buffer;
> -	struct sms_msg_data4 *msg;
> +	struct sms_msg_data5 *msg;
>  	u32 mem_address,  calc_checksum = 0;
>  	u32 i, *ptr;
>  	u8 *payload = firmware->payload;
> @@ -989,24 +989,20 @@ static int smscore_load_firmware_family2(struct smscore_device_t *coredev,
>  		goto exit_fw_download;
>  
>  	if (coredev->mode == DEVICE_MODE_NONE) {
> -		struct sms_msg_data *trigger_msg =
> -			(struct sms_msg_data *) msg;
> -
>  		pr_debug("sending MSG_SMS_SWDOWNLOAD_TRIGGER_REQ\n");
>  		SMS_INIT_MSG(&msg->x_msg_header,
>  				MSG_SMS_SWDOWNLOAD_TRIGGER_REQ,
> -				sizeof(struct sms_msg_hdr) +
> -				sizeof(u32) * 5);
> +				sizeof(*msg));
>  
> -		trigger_msg->msg_data[0] = firmware->start_address;
> +		msg->msg_data[0] = firmware->start_address;
>  					/* Entry point */
> -		trigger_msg->msg_data[1] = 6; /* Priority */
> -		trigger_msg->msg_data[2] = 0x200; /* Stack size */
> -		trigger_msg->msg_data[3] = 0; /* Parameter */
> -		trigger_msg->msg_data[4] = 4; /* Task ID */
> +		msg->msg_data[1] = 6; /* Priority */
> +		msg->msg_data[2] = 0x200; /* Stack size */
> +		msg->msg_data[3] = 0; /* Parameter */
> +		msg->msg_data[4] = 4; /* Task ID */
>  
> -		rc = smscore_sendrequest_and_wait(coredev, trigger_msg,
> -					trigger_msg->x_msg_header.msg_length,
> +		rc = smscore_sendrequest_and_wait(coredev, msg,
> +					msg->x_msg_header.msg_length,
>  					&coredev->trigger_done);
>  	} else {
>  		SMS_INIT_MSG(&msg->x_msg_header, MSG_SW_RELOAD_EXEC_REQ,
> diff --git a/drivers/media/common/siano/smscoreapi.h b/drivers/media/common/siano/smscoreapi.h
> index 4a6b9f4c44ac..f8789ee0d554 100644
> --- a/drivers/media/common/siano/smscoreapi.h
> +++ b/drivers/media/common/siano/smscoreapi.h
> @@ -624,9 +624,9 @@ struct sms_msg_data2 {
>  	u32 msg_data[2];
>  };
>  
> -struct sms_msg_data4 {
> +struct sms_msg_data5 {
>  	struct sms_msg_hdr x_msg_header;
> -	u32 msg_data[4];
> +	u32 msg_data[5];
>  };
>  
>  struct sms_data_download {
> 

Powered by blists - more mailing lists