be2iscsi: adding functionality to change network settings using iscsiadm

SCSI Mike Christie 2 participants 2 messages Sep 5, 2014 – Sep 9, 2014 1 message not in the archive

Subsystems (2)

SCSI Emulex 10Gbps iSCSI - OneConnect driver

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20140904102734.GA21504@mwanda

Mike Christie

author

2014-09-05 03:10 UTC #1 lore raw

Quoted 32 lines · Dan Carpenter

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          }

Quoted 9 lines

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?

drivers/scsi/be2iscsi/be_mgmt.c +8 -5

index 8478506..681d4e8 100644
@@ -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);
 }

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Mike Christie
Sent: Friday, September 05, 2014 8:40 AM
To: Dan Carpenter
Cc: [email protected]; Kees Cook; Jayamohan Kallickal
Subject: Re: [SCSI] be2iscsi: adding functionality to change network settings using iscsiadm

Quoted 35 lines · Dan Carpenter

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.

Quoted 8 lines

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?

Tested the patch and its working fine.