From 3315344dfa4c25006e213afbd8872b5cc5c9a02c Mon Sep 17 00:00:00 2001 From: Dmitry Eremin Date: Tue, 25 Feb 2014 22:44:09 +0400 Subject: [PATCH] LU-4629 lnet: fix issues found by Klocwork Insight tool Null pointer 'cp' that comes from line 2544 may be dereferenced at line 2618. Pointer 'ni' checked for NULL at line 1569 may be passed to function and may be dereferenced there by passing argument 1 to function 'lnet_ni_notify_locked' at line 1621. Null pointer 'best_iface' that comes from line 802 may be dereferenced at line 832. Buffer overflow of string buffer due to non null terminated string. sscanf format specification '%lx' expects type 'unsigned long int *' for 'x', but parameter 4 has a different type 'long int*'. Pointer 'tsc' returned from call to function 'sfw_find_test_case' at line 571 may be NULL and will be dereferenced at line 572. sscanf format specification '%i' expects type 'int *' for 'i', but parameter 3 has a different type 'unsigned int*'. Local variable 'hash' is never used. Signed-off-by: Dmitry Eremin Change-Id: I2a942735cd93906453b92dcc711a956b7adf5615 Reviewed-on: http://review.whamcloud.com/9386 Tested-by: Jenkins Reviewed-by: John L. Hammond Reviewed-by: Isaac Huang Tested-by: Maloo Reviewed-by: Oleg Drokin --- lnet/klnds/o2iblnd/o2iblnd_cb.c | 12 ++++++--- lnet/klnds/socklnd/socklnd.c | 4 +-- lnet/lnet/api-ni.c | 6 ++--- lnet/lnet/router.c | 3 ++- lnet/selftest/conctl.c | 11 +++++--- lnet/selftest/framework.c | 20 +++++++++----- lnet/utils/debug.c | 58 +++++++++++++++++++++-------------------- lnet/utils/portals.c | 4 +-- 8 files changed, 68 insertions(+), 50 deletions(-) diff --git a/lnet/klnds/o2iblnd/o2iblnd_cb.c b/lnet/klnds/o2iblnd/o2iblnd_cb.c index eeef534..f1b580e 100644 --- a/lnet/klnds/o2iblnd/o2iblnd_cb.c +++ b/lnet/klnds/o2iblnd/o2iblnd_cb.c @@ -2615,14 +2615,18 @@ kiblnd_rejected (kib_conn_t *conn, int reason, void *priv, int priv_nob) case IBLND_REJECT_MSG_QUEUE_SIZE: CERROR("%s rejected: incompatible message queue depth %d, %d\n", - libcfs_nid2str(peer->ibp_nid), cp->ibcp_queue_depth, - IBLND_MSG_QUEUE_SIZE(conn->ibc_version)); + libcfs_nid2str(peer->ibp_nid), + cp != NULL ? cp->ibcp_queue_depth : + IBLND_MSG_QUEUE_SIZE(rej->ibr_version), + IBLND_MSG_QUEUE_SIZE(conn->ibc_version)); break; case IBLND_REJECT_RDMA_FRAGS: CERROR("%s rejected: incompatible # of RDMA fragments %d, %d\n", - libcfs_nid2str(peer->ibp_nid), cp->ibcp_max_frags, - IBLND_RDMA_FRAGS(conn->ibc_version)); + libcfs_nid2str(peer->ibp_nid), + cp != NULL ? cp->ibcp_max_frags : + IBLND_RDMA_FRAGS(rej->ibr_version), + IBLND_RDMA_FRAGS(conn->ibc_version)); break; case IBLND_REJECT_NO_RESOURCES: diff --git a/lnet/klnds/socklnd/socklnd.c b/lnet/klnds/socklnd/socklnd.c index ff6be40..9d65d38 100644 --- a/lnet/klnds/socklnd/socklnd.c +++ b/lnet/klnds/socklnd/socklnd.c @@ -829,14 +829,14 @@ ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips) best_npeers = iface->ksni_npeers; } + LASSERT(best_iface != NULL); + best_iface->ksni_npeers++; ip = best_iface->ksni_ipaddr; peer->ksnp_passive_ips[i] = ip; peer->ksnp_n_passive_ips = i+1; } - LASSERT (best_iface != NULL); - /* mark the best matching peer IP used */ j = ksocknal_match_peerip(best_iface, peerips, n_peerips); peerips[j] = 0; diff --git a/lnet/lnet/api-ni.c b/lnet/lnet/api-ni.c index 7ba7a3d..d233ba6 100644 --- a/lnet/lnet/api-ni.c +++ b/lnet/lnet/api-ni.c @@ -218,8 +218,7 @@ lnet_create_remote_nets_table(void) static void lnet_destroy_remote_nets_table(void) { - int i; - cfs_list_t *hash; + int i; if (the_lnet.ln_remote_nets_hash == NULL) return; @@ -228,7 +227,8 @@ lnet_destroy_remote_nets_table(void) LASSERT(cfs_list_empty(&the_lnet.ln_remote_nets_hash[i])); LIBCFS_FREE(the_lnet.ln_remote_nets_hash, - LNET_REMOTE_NETS_HASH_SIZE * sizeof(*hash)); + LNET_REMOTE_NETS_HASH_SIZE * + sizeof(the_lnet.ln_remote_nets_hash[0])); the_lnet.ln_remote_nets_hash = NULL; } diff --git a/lnet/lnet/router.c b/lnet/lnet/router.c index 4da9573..a560db8 100644 --- a/lnet/lnet/router.c +++ b/lnet/lnet/router.c @@ -1618,7 +1618,8 @@ lnet_notify(lnet_ni_t *ni, lnet_nid_t nid, int alive, cfs_time_t when) lnet_notify_locked(lp, ni == NULL, alive, when); - lnet_ni_notify_locked(ni, lp); + if (ni != NULL) + lnet_ni_notify_locked(ni, lp); lnet_peer_decref_locked(lp); diff --git a/lnet/selftest/conctl.c b/lnet/selftest/conctl.c index 3b7a96e..7fe5fb2 100644 --- a/lnet/selftest/conctl.c +++ b/lnet/selftest/conctl.c @@ -758,13 +758,18 @@ int lst_test_add_ioctl(lstio_test_args_t *args) goto out; LIBCFS_ALLOC(dst_name, args->lstio_tes_dgrp_nmlen + 1); - if (dst_name == NULL) + if (dst_name == NULL) goto out; if (args->lstio_tes_param != NULL) { LIBCFS_ALLOC(param, args->lstio_tes_param_len); if (param == NULL) goto out; + if (copy_from_user(param, args->lstio_tes_param, + args->lstio_tes_param_len)) { + rc = -EFAULT; + goto out; + } } rc = -EFAULT; @@ -773,9 +778,7 @@ int lst_test_add_ioctl(lstio_test_args_t *args) copy_from_user(src_name, args->lstio_tes_sgrp_name, args->lstio_tes_sgrp_nmlen) || copy_from_user(dst_name, args->lstio_tes_dgrp_name, - args->lstio_tes_dgrp_nmlen) || - copy_from_user(param, args->lstio_tes_param, - args->lstio_tes_param_len)) + args->lstio_tes_dgrp_nmlen)) goto out; rc = lstcon_test_add(batch_name, diff --git a/lnet/selftest/framework.c b/lnet/selftest/framework.c index 6959a5b..71b1677 100644 --- a/lnet/selftest/framework.c +++ b/lnet/selftest/framework.c @@ -544,9 +544,9 @@ sfw_debug_session (srpc_debug_reqst_t *request, srpc_debug_reply_t *reply) return 0; } - reply->dbg_status = 0; - reply->dbg_sid = sn->sn_id; - reply->dbg_timeout = sn->sn_timeout; + reply->dbg_status = 0; + reply->dbg_sid = sn->sn_id; + reply->dbg_timeout = sn->sn_timeout; if (strlcpy(reply->dbg_name, &sn->sn_name[0], sizeof(reply->dbg_name)) >= sizeof(reply->dbg_name)) return -E2BIG; @@ -568,10 +568,16 @@ sfw_test_rpc_fini (srpc_client_rpc_t *rpc) static inline int sfw_test_buffers(sfw_test_instance_t *tsi) { - struct sfw_test_case *tsc = sfw_find_test_case(tsi->tsi_service); - struct srpc_service *svc = tsc->tsc_srv_service; + struct sfw_test_case *tsc; + struct srpc_service *svc; int nbuf; + LASSERT(tsi != NULL); + tsc = sfw_find_test_case(tsi->tsi_service); + LASSERT(tsc != NULL); + svc = tsc->tsc_srv_service; + LASSERT(svc != NULL); + nbuf = min(svc->sv_wi_total, tsi->tsi_loop) / svc->sv_ncpts; return max(SFW_TEST_WI_MIN, nbuf + SFW_TEST_WI_EXTRA); } @@ -616,8 +622,10 @@ sfw_load_test(struct sfw_test_instance *tsi) void sfw_unload_test(struct sfw_test_instance *tsi) { - struct sfw_test_case *tsc = sfw_find_test_case(tsi->tsi_service); + struct sfw_test_case *tsc; + LASSERT(tsi != NULL); + tsc = sfw_find_test_case(tsi->tsi_service); LASSERT(tsc != NULL); if (tsi->tsi_is_client) diff --git a/lnet/utils/debug.c b/lnet/utils/debug.c index a0c0ce1..3a477a0 100644 --- a/lnet/utils/debug.c +++ b/lnet/utils/debug.c @@ -562,8 +562,8 @@ print: int jt_dbg_debug_kernel(int argc, char **argv) { - char filename[4096]; - struct stat st; + struct stat st; + char filename[PATH_MAX]; int raw = 0; int save_errno; int fdin; @@ -586,7 +586,7 @@ int jt_dbg_debug_kernel(int argc, char **argv) * then dump directly to any supplied filename, otherwise this is * just a temp file and we dump to the real file at convert time. */ if (argc > 1 && raw) { - if (strlen(argv[1]) > sizeof(filename)-1) { + if (strlen(argv[1]) >= sizeof(filename)) { fprintf(stderr, "File name too long: %s\n", argv[1]); return 1; } @@ -594,8 +594,8 @@ int jt_dbg_debug_kernel(int argc, char **argv) } else { if (snprintf(filename, sizeof(filename), "%s"CFS_TIME_T".%u", LIBCFS_DEBUG_FILE_PATH_DEFAULT, time(NULL), - getpid()) >= - sizeof(filename)) { + getpid()) + >= sizeof(filename)) { fprintf(stderr, "File name too long\n"); return 1; } @@ -806,29 +806,31 @@ int jt_dbg_clear_debug_buf(int argc, char **argv) int jt_dbg_mark_debug_buf(int argc, char **argv) { - static char scratch[MAX_MARK_SIZE] = { '\0' }; - int rc, max_size = MAX_MARK_SIZE-1; - struct libcfs_ioctl_data data = { 0 }; - char *text; - time_t now = time(NULL); - - if (argc > 1) { - int count; - text = scratch; - strncpy(text, argv[1], max_size); - max_size-=strlen(argv[1]); - for (count = 2; (count < argc) && (max_size > 0); count++){ - strncat(text, " ", max_size); - max_size -= 1; - strncat(text, argv[count], max_size); - max_size -= strlen(argv[count]); - } - } else { - text = ctime(&now); - } + static char scratch[MAX_MARK_SIZE] = ""; + struct libcfs_ioctl_data data = { 0 }; + char *text; + int rc; + + if (argc > 1) { + int count, max_size = sizeof(scratch) - 1; + + strncpy(scratch, argv[1], max_size); + max_size -= strlen(argv[1]); + for (count = 2; (count < argc) && (max_size > 1); count++) { + strncat(scratch, " ", max_size); + max_size -= 1; + strncat(scratch, argv[count], max_size); + max_size -= strlen(argv[count]); + } + scratch[sizeof(scratch) - 1] = '\0'; + text = scratch; + } else { + time_t now = time(NULL); + text = ctime(&now); + } - data.ioc_inllen1 = strlen(text) + 1; - data.ioc_inlbuf1 = text; + data.ioc_inllen1 = strlen(text) + 1; + data.ioc_inlbuf1 = text; if (libcfs_ioctl_pack(&data, &buf, max) != 0) { fprintf(stderr, "libcfs_ioctl_pack failed.\n"); return -1; @@ -894,7 +896,7 @@ int jt_dbg_modules(int argc, char **argv) char *path = ""; const char *proc = "/proc/modules"; char modname[128], buf[4096]; - long modaddr; + unsigned long modaddr; FILE *file; if (argc >= 2) diff --git a/lnet/utils/portals.c b/lnet/utils/portals.c index 110716c..2a8684c 100644 --- a/lnet/utils/portals.c +++ b/lnet/utils/portals.c @@ -1049,7 +1049,7 @@ jt_ptl_fail_nid (int argc, char **argv) { int rc; lnet_nid_t nid; - unsigned int threshold; + int threshold; struct libcfs_ioctl_data data; if (argc < 2 || argc > 3) @@ -1066,7 +1066,7 @@ jt_ptl_fail_nid (int argc, char **argv) if (argc < 3) { threshold = LNET_MD_THRESH_INF; - } else if (sscanf (argv[2], "%i", &threshold) != 1) { + } else if (sscanf(argv[2], "%i", &threshold) != 1) { fprintf (stderr, "Can't parse count \"%s\"\n", argv[2]); return (-1); } -- 1.8.3.1