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

Sony John-N
2014-09-09T06:27:04Z

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

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?

Tested the patch and its working fine.