[PATCH 01/12] iscsi_sysfs: Fix NULL pointer deference in iscsi_sysfs_read_iface

[Wenchao Hao]

Check if t is valid before accessing it.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---
usr/iscsi_sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/usr/iscsi_sysfs.c b/usr/iscsi_sysfs.c
index 540adfd..abefde2 100644
--- a/usr/iscsi_sysfs.c
+++ b/usr/iscsi_sysfs.c
@@ -856,7 +856,7 @@ static int iscsi_sysfs_read_iface(struct iface_rec *iface, int host_no,
}
}

- if (session && t->template->use_boot_info)
+ if (session && t && t->template->use_boot_info)
iscsi_sysfs_read_boot(iface, session);

if (!iface_kern_id)
--
2.27.0

[Wenchao Hao]

Recently, we use Coverity to analysis the open-iscsi package.
Several issues should be resolved to make Coverity happy.

Wenchao Hao (12):
iscsi_sysfs: Fix NULL pointer deference in iscsi_sysfs_read_iface
iscsi-iname: Verify open() return value before calling read()
iscsiuio: Fix invalid parameter when call fstat()
open-iscsi: Fix invalid pointer deference in find_initiator()
open-iscsi: Fix NULL pointer dereference in mgmt_ipc_read_req()
iscsi_net_util: Fix NULL pointer dereference in find_vlan_dev()
open-iscsi: Clean user_param list when process exit
fwparam_ppc: Fix NULL pointer dereference in find_devtree()
sysfs: Verify parameter of sysfs_device_get()
fwparam_ppc: Fix illegal memory access in fwparam_ppc.c
iscsiuio: Remove unused macro IFNAMSIZ defined in iscsid_ipc.c
fwparam_ppc: Fix memory leak in fwparam_ppc.c

iscsiuio/src/unix/iscsid_ipc.c | 3 --
iscsiuio/src/unix/libs/bnx2x.c | 7 ++++
iscsiuio/src/unix/libs/qedi.c | 7 ++++
usr/idbm.c | 14 ++++++++
usr/idbm.h | 1 +
usr/iscsi_net_util.c | 6 ++++
usr/iscsi_sysfs.c | 2 +-
usr/iscsiadm.c | 4 +++
usr/iscsistart.c | 5 +++
usr/mgmt_ipc.c | 5 ++-
usr/sysfs.c | 6 ++--
utils/fwparam_ibft/fwparam_ppc.c | 57 +++++++++++++++++++++++++-------
utils/iscsi-iname.c | 6 ++--
13 files changed, 102 insertions(+), 21 deletions(-)

--
2.27.0

[Wenchao Hao]

System call open() might return -1 if an error occurred which
should be taken into consideration.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

utils/iscsi-iname.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/utils/iscsi-iname.c b/utils/iscsi-iname.c
index 0f587e1..834352e 100644
--- a/utils/iscsi-iname.c
+++ b/utils/iscsi-iname.c
@@ -96,7 +96,8 @@ main(int argc, char *argv[])
* uniqueness properties
*/

- if ((fd = open(RANDOM_NUM_GENERATOR, O_RDONLY))) {
+ fd = open(RANDOM_NUM_GENERATOR, O_RDONLY);
+ if (fd != -1) {
e = read(fd, &entropy, 16);
if (e >= 1)
MD5Update(&context, (md5byte *)entropy, e);
@@ -141,7 +142,8 @@ main(int argc, char *argv[])
* good as any other).
*/

- if ((fd = open(RANDOM_NUM_GENERATOR, O_RDONLY))) {
+ fd = open(RANDOM_NUM_GENERATOR, O_RDONLY);
+ if (fd != -1) {
if (read(fd, entropy, 1) == 1)
bytes = &digest[(entropy[0] % (sizeof(digest) - 6))];
close(fd);
--
2.27.0

[Wenchao Hao]

In sysfs_device_get() we should firstly check whether
devpath pointer is NULL before accessing it

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

usr/sysfs.c | 6 ++++--

1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/usr/sysfs.c b/usr/sysfs.c
index 8d37c69..7f26572 100644
--- a/usr/sysfs.c
+++ b/usr/sysfs.c
@@ -168,9 +168,11 @@ struct sysfs_device *sysfs_device_get(const char *devpath)
int len;
char *pos;

+ if (!devpath)
+ return NULL;
+
/* we handle only these devpathes */
- if (devpath != NULL &&
- strncmp(devpath, "/devices/", 9) != 0 &&
+ if (strncmp(devpath, "/devices/", 9) != 0 &&
strncmp(devpath, "/subsystem/", 11) != 0 &&
strncmp(devpath, "/module/", 8) != 0 &&
strncmp(devpath, "/bus/", 5) != 0 &&
--
2.27.0

[Wenchao Hao]

if_nameindex() returns NULL on error, if it returns NULL, we
just print the error info and return NULL.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

usr/iscsi_net_util.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/usr/iscsi_net_util.c b/usr/iscsi_net_util.c
index c38456f..10e6fa6 100644
--- a/usr/iscsi_net_util.c
+++ b/usr/iscsi_net_util.c
@@ -206,6 +206,12 @@ static char *find_vlan_dev(char *netdev, int vlan_id) {
}

ifni = if_nameindex();
+ if (!ifni) {
+ log_error("Failed to find netdev:%s", strerror(errno));
+ close(sockfd);
+ return NULL;
+ }
+
for (i = 0; ifni[i].if_index && ifni[i].if_name; i++) {
strlcpy(vlan_hwaddr.ifr_name, ifni[i].if_name, IFNAMSIZ);
ioctl(sockfd, SIOCGIFHWADDR, &vlan_hwaddr);
--
2.27.0

[Wenchao Hao]

If strdup() returns NULL, find_devtree() just return NULL to
indicate this function execute failed.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

utils/fwparam_ibft/fwparam_ppc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/utils/fwparam_ibft/fwparam_ppc.c b/utils/fwparam_ibft/fwparam_ppc.c
index b5eaa00..25d4532 100644
--- a/utils/fwparam_ibft/fwparam_ppc.c
+++ b/utils/fwparam_ibft/fwparam_ppc.c
@@ -76,6 +76,9 @@ static char *find_devtree(const char *filename)
* /chosen.
*/

+ if (!devtree)
+ return NULL;
+
chop_at = strstr(devtree, "/chosen");
if (!chop_at)
chop_at = strstr(devtree, "/aliases");
--
2.27.0

[Wenchao Hao]

Both in iscsiadm and iscsistart, a list is initialized to management
struct iscsi_param which allocated by idbm_alloc_user_param().
While both of iscsiadm and iscsistart did not free these iscsi_param
when process exit, this patch add a free flow when process exit.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

usr/idbm.c | 14 ++++++++++++++
usr/idbm.h | 1 +
usr/iscsiadm.c | 4 ++++
usr/iscsistart.c | 5 +++++
4 files changed, 24 insertions(+)

diff --git a/usr/idbm.c b/usr/idbm.c
index 42c2699..f8b50f1 100644
--- a/usr/idbm.c
+++ b/usr/idbm.c
@@ -2903,6 +2903,20 @@ free_param:
return NULL;
}

+void idbm_free_user_param(struct user_param *param)
+{
+ if (!param)
+ return;
+
+ if (param->name)
+ free(param->name);
+
+ if (param->value)
+ free(param->value);
+
+ free(param);
+}
+
int idbm_node_set_rec_from_param(struct list_head *params, node_rec_t *rec,
int verify)
{
diff --git a/usr/idbm.h b/usr/idbm.h
index 46cd82a..7496f1d 100644
--- a/usr/idbm.h
+++ b/usr/idbm.h
@@ -153,6 +153,7 @@ extern int idbm_node_set_rec_from_param(struct list_head *params,
extern int idbm_node_set_param(void *data, node_rec_t *rec);
extern int idbm_discovery_set_param(void *data, discovery_rec_t *rec);
struct user_param *idbm_alloc_user_param(char *name, char *value);
+void idbm_free_user_param(struct user_param *param);
extern void idbm_node_setup_defaults(node_rec_t *rec);
extern struct node_rec *idbm_find_rec_in_list(struct list_head *rec_list,
char *targetname, char *addr,
diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
index 667f83c..ea1643b 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -4075,6 +4075,10 @@ free_ifaces:
list_del(&iface->list);
free(iface);
}
+ list_for_each_entry(param, &params, list) {
+ list_del(&param->list);
+ idbm_free_user_param(param);
+ }
free_transports();
sysfs_cleanup();
return rc;
diff --git a/usr/iscsistart.c b/usr/iscsistart.c
index ee810f7..73991b3 100644
--- a/usr/iscsistart.c
+++ b/usr/iscsistart.c
@@ -347,6 +347,7 @@ int main(int argc, char *argv[])
struct boot_context *context, boot_context;
struct sigaction sa_old;
struct sigaction sa_new;
+ struct user_param *param;
int control_fd, mgmt_ipc_fd, err;
pid_t pid;

@@ -541,6 +542,10 @@ int main(int argc, char *argv[])
mgmt_ipc_close(mgmt_ipc_fd);
free_initiator();
sysfs_cleanup();
+ list_for_each_entry(param, &user_params, list) {
+ list_del(&param->list);
+ idbm_free_user_param(param);
+ }

log_debug(1, "iscsi child done");
return 0;
--
2.27.0

[Wenchao Hao]

If malloc() returns NULL on fail, we should return -ENOMEM to
avoid NULL pointer dereference.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

usr/mgmt_ipc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/usr/mgmt_ipc.c b/usr/mgmt_ipc.c
index c292161..054378e 100644
--- a/usr/mgmt_ipc.c
+++ b/usr/mgmt_ipc.c
@@ -453,8 +453,11 @@ mgmt_ipc_read_req(queue_task_t *qtask)
/* Remember the allocated pointer in the
* qtask - it will be freed by write_rsp.
* Note: we allocate one byte in excess
- * so we can append a NUL byte. */
+ * so we can append a NULL byte. */
qtask->payload = malloc(req->payload_len + 1);
+ if (!qtask->payload)
+ return -ENOMEM;
+
rc = mgmt_ipc_read_data(qtask->mgmt_ipc_fd,
qtask->payload,
req->payload_len);
--
2.27.0

[Wenchao Hao]

If calloc() returns NULL just return ENOMEM;
if strdup() returns NULL we should free dev and return errno.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>

---
utils/fwparam_ibft/fwparam_ppc.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/utils/fwparam_ibft/fwparam_ppc.c b/utils/fwparam_ibft/fwparam_ppc.c
index 429d45c..b5eaa00 100644
--- a/utils/fwparam_ibft/fwparam_ppc.c
+++ b/utils/fwparam_ibft/fwparam_ppc.c
@@ -332,9 +332,16 @@ static int find_initiator(const char *fpath, const struct stat *sb, int tflag,
"/aliases/iscsi-disk"))) {

if (dev_count < OFWDEV_MAX) {
- ofwdevs[dev_count++] = dev =
- calloc(sizeof(struct ofw_dev), 1);
+ dev = calloc(sizeof(struct ofw_dev), 1);
+ if (!dev)
+ return -ENOMEM;
+
dev->prop_path = strdup(fpath + devtree_offset);
+ if (!dev->prop_path) {
+ free(dev);
+ return errno;
+ }
+ ofwdevs[dev_count++] = dev;
}
}
return 0;
--
2.27.0

[Wenchao Hao]

IFNAMSIZ is not used in this file. IFNAMSIZ is a macro defined in
net/if.h. Maybe this this macro is defined here to give it a self
defined value rather than system pre-defined at beginning, while
it seems the code reference the macro in this file is removed, so
here we can remove it.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>

Signed-off-by: Wu Bo <wub...@huawei.com>
---

iscsiuio/src/unix/iscsid_ipc.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/iscsiuio/src/unix/iscsid_ipc.c b/iscsiuio/src/unix/iscsid_ipc.c
index 2acac48..ea03d37 100644
--- a/iscsiuio/src/unix/iscsid_ipc.c
+++ b/iscsiuio/src/unix/iscsid_ipc.c
@@ -54,9 +54,6 @@

#define PFX "iscsi_ipc "

-/* TODO fix me */
-#define IFNAMSIZ 15
-
#include "nic.h"
#include "nic_utils.h"
#include "nic_vlan.h"
--
2.27.0

[Wenchao Hao]

calloc() is called because loop_devs() to allocate memory
which stored in array ofwdev. These memory should be freed
at the end.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

utils/fwparam_ibft/fwparam_ppc.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/utils/fwparam_ibft/fwparam_ppc.c b/utils/fwparam_ibft/fwparam_ppc.c
index 6a45b8c..da9d76e 100644
--- a/utils/fwparam_ibft/fwparam_ppc.c
+++ b/utils/fwparam_ibft/fwparam_ppc.c
@@ -446,6 +446,7 @@ int fwparam_ppc_boot_info(struct boot_context *context)
char filename[FILENAMESZ];
int error;
char *devtree;
+ int i;

/*
* For powerpc, our operations are fundamentally to locate
@@ -511,6 +512,10 @@ free_bootpath_val:

free_devtree:
free(devtree);
+ for (i = 0; i < dev_count; i++)
+ if (ofwdevs[i])
+ free(ofwdevs[i]);
+
return error;
}

@@ -525,6 +530,7 @@ int fwparam_ppc_get_targets(struct list_head *list)
struct boot_context *context;
int error;
char *devtree;
+ int i;

/*
* For powerpc, our operations are fundamentally to locate
@@ -592,5 +598,9 @@ free_bootpath_val:

free_devtree:
free(devtree);
+ for (i = 0; i < dev_count; i++)
+ if (ofwdevs[i])
+ free(ofwdevs[i]);
+
return error;
}
--
2.27.0

[Wenchao Hao]

bootpath_val is allocated in find_file() and referenced if find_file()
excute successfully, while it was freed in find_file() and
dereferenced after find_file() if find_file() returns 1.

This patch remove free(bootpath_val) in find_file() and free
bootpath_val when it is used done.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

utils/fwparam_ibft/fwparam_ppc.c | 33 ++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/utils/fwparam_ibft/fwparam_ppc.c b/utils/fwparam_ibft/fwparam_ppc.c
index 25d4532..6a45b8c 100644
--- a/utils/fwparam_ibft/fwparam_ppc.c
+++ b/utils/fwparam_ibft/fwparam_ppc.c
@@ -292,15 +292,17 @@ static int find_file(const char *filename)
fprintf(stderr, "%s: Could not open %s: %s (%d)\n",
__func__, filename, strerror(errno), errno);
free(bootpath_val);
+ bootpath_val = NULL;
return -1;
}

bytes_read = read(fd, bootpath_val, bootpath_stat.st_size);
close(fd);
- free(bootpath_val);
if (bytes_read != bootpath_stat.st_size) {
- fprintf(stderr, "%s: Could not open %s: %s (%d)\n",
+ fprintf(stderr, "%s: Failed to read %s: %s (%d)\n",
__func__, filename, strerror(EIO), EIO);
+ free(bootpath_val);
+ bootpath_val = NULL;
return -1;
}

@@ -381,6 +383,8 @@ static int loop_devs(const char *devtree)
if (!error)
error = locate_mac(devtree, ofwdevs[i]);

+ free(bootpath_val);
+ bootpath_val = NULL;
}
}
return error;
@@ -468,9 +472,10 @@ int fwparam_ppc_boot_info(struct boot_context *context)
if (error)
goto free_devtree;

- if (find_file(filename) < 1)
+ if (find_file(filename) < 1) {
error = ISCSI_ERR_NO_OBJS_FOUND;
- else {
+ goto free_devtree;
+ } else {
if (debug)
printf("%s:\n%s\n\n", filename, bootpath_val);
/*
@@ -480,12 +485,12 @@ int fwparam_ppc_boot_info(struct boot_context *context)

if (!strstr(bootpath_val, "iscsi")) {
error = ISCSI_ERR_INVAL;
- goto free_devtree;
+ goto free_bootpath_val;
}
ofwdevs[0] = calloc(1, sizeof(struct ofw_dev));
if (!ofwdevs[0]) {
error = ISCSI_ERR_NOMEM;
- goto free_devtree;
+ goto free_bootpath_val;
}

error = parse_params(bootpath_val, ofwdevs[0]);
@@ -500,6 +505,10 @@ int fwparam_ppc_boot_info(struct boot_context *context)
free(ofwdevs[0]);
}

+free_bootpath_val:
+ free(bootpath_val);
+ bootpath_val = NULL;
+
free_devtree:
free(devtree);
return error;
@@ -542,9 +551,10 @@ int fwparam_ppc_get_targets(struct list_head *list)
if (error)
goto free_devtree;

- if (find_file(filename) < 1)
+ if (find_file(filename) < 1) {
error = ISCSI_ERR_NO_OBJS_FOUND;
- else {
+ goto free_devtree;
+ } else {
if (debug)
printf("%s:\n%s\n\n", filename, bootpath_val);
/*
@@ -554,12 +564,12 @@ int fwparam_ppc_get_targets(struct list_head *list)

if (!strstr(bootpath_val, "iscsi")) {
error = ISCSI_ERR_INVAL;
- goto free_devtree;
+ goto free_bootpath_val;
}
ofwdevs[0] = calloc(1, sizeof(struct ofw_dev));
if (!ofwdevs[0]) {
error = ISCSI_ERR_NOMEM;
- goto free_devtree;
+ goto free_bootpath_val;
}

error = parse_params(bootpath_val, ofwdevs[0]);
@@ -576,6 +586,9 @@ int fwparam_ppc_get_targets(struct list_head *list)
}
free(ofwdevs[0]);
}
+free_bootpath_val:
+ free(bootpath_val);
+ bootpath_val = NULL;

free_devtree:
free(devtree);
--
2.27.0

[Ulrich Windl]

>>> Wenchao Hao <haowe...@huawei.com> schrieb am 07.12.2020 um 02:54 in Nachricht
<20201207015410.4...@huawei.com>:

> If malloc() returns NULL on fail, we should return -ENOMEM to
> avoid NULL pointer dereference.
>
> Signed-off-by: Wenchao Hao <haowe...@huawei.com>
> Signed-off-by: Zhiqiang Liu <liuzhi...@huawei.com>
> Signed-off-by: Wu Bo <wub...@huawei.com>
> ---
> usr/mgmt_ipc.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/usr/mgmt_ipc.c b/usr/mgmt_ipc.c
> index c292161..054378e 100644
> --- a/usr/mgmt_ipc.c
> +++ b/usr/mgmt_ipc.c
> @@ -453,8 +453,11 @@ mgmt_ipc_read_req(queue_task_t *qtask)
> /* Remember the allocated pointer in the
> * qtask - it will be freed by write_rsp.
> * Note: we allocate one byte in excess
> - * so we can append a NUL byte. */
> + * so we can append a NULL byte. */

Nitpick: "NUL" is a well-defined ACSII character, while NULL is a well-defined C pointer. Thus I'd keep NUL.

> qtask->payload = malloc(req->payload_len + 1);
> + if (!qtask->payload)
> + return -ENOMEM;
> +
> rc = mgmt_ipc_read_data(qtask->mgmt_ipc_fd,
> qtask->payload,
> req->payload_len);
> --
> 2.27.0
>

> --
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to open-iscsi+...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/open-iscsi/20201207015410.48488-6-haowencha
> o%40huawei.com.

[Lee Duncan]

Reviewed-by: Lee Duncan <ldu...@suse.com>

[Lee Duncan]

Reviewed-by: Lee Duncan <ldu...@suse.com>

[Lee Duncan]

I have reviewed a couple of these, but you need to submit them as pull
requests to github.com/open-iscsi/open-iscsi.

The "reviewed-by" tag I replied to a couple of them really isn't needed,
as I'm the one that will be merging them (or not) on github (sans Chris,
my co-maintainer, who sometimes does that).

On 12/6/20 5:53 PM, Wenchao Hao wrote:

[Wenchao Hao]

On 2020/12/12 0:32, Lee Duncan wrote:
> I have reviewed a couple of these, but you need to submit them as pull
> requests to github.com/open-iscsi/open-iscsi.
>
> The "reviewed-by" tag I replied to a couple of them really isn't needed,
> as I'm the one that will be merging them (or not) on github (sans Chris,
> my co-maintainer, who sometimes does that).

I submitted a pull request to github already.