Re: [SCSI] be2iscsi: adding functionality to change network settings using iscsiadm

[Mike Christie <michaelc@cs.wisc.edu>]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

On 09/04/2014 05:27 AM, Dan Carpenter wrote:
> Hello Mike Christie,
> 
> The patch 0e43895ec1f4: "[SCSI] be2iscsi: adding functionality to
> change network settings using iscsiadm" from Apr 3, 2012, leads to
> the following static checker warning:
> 
> 	drivers/scsi/be2iscsi/be_mgmt.c:945 mgmt_static_ip_modify()
> 	error: 'ip_param->len' from user is not capped properly
> 
> drivers/scsi/be2iscsi/be_mgmt.c
>    940          req->ip_params.ip_record.ip_addr.size_of_structure =
>    941                  sizeof(struct be_ip_addr_subnet_format);
>    942          req->ip_params.ip_record.ip_addr.ip_type = ip_type;
>    943  
>    944          if (ip_action == IP_ACTION_ADD) {
>    945                  memcpy(req->ip_params.ip_record.ip_addr.addr, ip_param->value,
>    946                         ip_param->len);
>    947  
>    948                  if (subnet_param)
>    949                          memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
>    950                                 subnet_param->value, subnet_param->len);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    951          } else {
>    952                  memcpy(req->ip_params.ip_record.ip_addr.addr,
>    953                         if_info->ip_addr.addr, ip_param->len);
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    954  
>    955                  memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
>    956                         if_info->ip_addr.subnet_mask, ip_param->len);
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    957          }
> 
> These memcpy()s can overflow.  It seems root only but it makes the
> static checker complain.
> 
> One call tree is:
> 
> beiscsi_set_static_ip() <- gets iface_ip.
>   -> mgmt_set_ip()
>      -> mgmt_static_ip_modify()
> 

Jay, I made the attached patch to fix these issues plus one more I
found. I am still waiting on getting systems at work. Could you have
your people test it?


[-- Attachment #2: be2iscsi-check-ip-buf-size.patch --]
[-- Type: text/x-patch, Size: 1495 bytes --]

diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
index 8478506..681d4e8 100644
--- a/drivers/scsi/be2iscsi/be_mgmt.c
+++ b/drivers/scsi/be2iscsi/be_mgmt.c
@@ -943,17 +943,20 @@ mgmt_static_ip_modify(struct beiscsi_hba *phba,
 
 	if (ip_action == IP_ACTION_ADD) {
 		memcpy(req->ip_params.ip_record.ip_addr.addr, ip_param->value,
-		       ip_param->len);
+		       sizeof(req->ip_params.ip_record.ip_addr.addr));
 
 		if (subnet_param)
 			memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
-			       subnet_param->value, subnet_param->len);
+			       subnet_param->value,
+			       sizeof(req->ip_params.ip_record.ip_addr.subnet_mask));
 	} else {
 		memcpy(req->ip_params.ip_record.ip_addr.addr,
-		       if_info->ip_addr.addr, ip_param->len);
+		       if_info->ip_addr.addr,
+		       sizeof(req->ip_params.ip_record.ip_addr.addr));
 
 		memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
-		       if_info->ip_addr.subnet_mask, ip_param->len);
+		       if_info->ip_addr.subnet_mask,
+		       sizeof(req->ip_params.ip_record.ip_addr.subnet_mask));
 	}
 
 	rc = mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
@@ -981,7 +984,7 @@ static int mgmt_modify_gateway(struct beiscsi_hba *phba, uint8_t *gt_addr,
 	req->action = gtway_action;
 	req->ip_addr.ip_type = BE2_IPV4;
 
-	memcpy(req->ip_addr.addr, gt_addr, param_len);
+	memcpy(req->ip_addr.addr, gt_addr, sizeof(req->ip_addr.addr));
 
 	return mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
 }