* [lustre-devel] staging: add Lustre file system client support
@ 2015-10-15 11:14 Dan Carpenter
2015-10-15 11:38 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2015-10-15 11:14 UTC (permalink / raw)
To: lustre-devel
Hello Lustre Devs,
The patch d7e09d0397e8: "staging: add Lustre file system client
support" from May 2, 2013, leads to the following static checker
warning:
drivers/staging/lustre/lnet/selftest/console.c:1330 lstcon_test_add()
error: 'paramlen' from user is not capped properly
drivers/staging/lustre/lnet/selftest/console.c
1273 int
1274 lstcon_test_add(char *batch_name, int type, int loop,
1275 int concur, int dist, int span,
1276 char *src_name, char *dst_name,
1277 void *param, int paramlen, int *retp,
1278 struct list_head *result_up)
1279 {
1280 lstcon_test_t *test = NULL;
1281 int rc;
1282 lstcon_group_t *src_grp = NULL;
1283 lstcon_group_t *dst_grp = NULL;
1284 lstcon_batch_t *batch = NULL;
1285
1286 /*
1287 * verify that a batch of the given name exists, and the groups
1288 * that will be part of the batch exist and have at least one
1289 * active node
1290 */
1291 rc = lstcon_verify_batch(batch_name, &batch);
1292 if (rc != 0)
1293 goto out;
1294
1295 rc = lstcon_verify_group(src_name, &src_grp);
1296 if (rc != 0)
1297 goto out;
1298
1299 rc = lstcon_verify_group(dst_name, &dst_grp);
1300 if (rc != 0)
1301 goto out;
1302
1303 if (dst_grp->grp_userland)
1304 *retp = 1;
1305
1306 LIBCFS_ALLOC(test, offsetof(lstcon_test_t, tes_param[paramlen]));
There is an underflow and integer overflow bug here.
1307 if (!test) {
1308 CERROR("Can't allocate test descriptor\n");
1309 rc = -ENOMEM;
1310
1311 goto out;
1312 }
1313
1314 test->tes_hdr.tsb_id = batch->bat_hdr.tsb_id;
1315 test->tes_batch = batch;
1316 test->tes_type = type;
1317 test->tes_oneside = 0; /* TODO */
1318 test->tes_loop = loop;
1319 test->tes_concur = concur;
1320 test->tes_stop_onerr = 1; /* TODO */
1321 test->tes_span = span;
1322 test->tes_dist = dist;
1323 test->tes_cliidx = 0; /* just used for creating RPC */
1324 test->tes_src_grp = src_grp;
1325 test->tes_dst_grp = dst_grp;
1326 INIT_LIST_HEAD(&test->tes_trans_list);
1327
1328 if (param != NULL) {
1329 test->tes_paramlen = paramlen;
1330 memcpy(&test->tes_param[0], param, paramlen);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the warning.
1331 }
The warning here is a false positive because the caller validates
"paramlen" when "param" is non-NULL. Unfortunately, on line 1306, we
use "paramlen" even when param is NULL. "paramlen" is signed so this
can mean "test" is smaller than expected leading to memory corruption.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] staging: add Lustre file system client support
2015-10-15 11:14 [lustre-devel] staging: add Lustre file system client support Dan Carpenter
@ 2015-10-15 11:38 ` Dan Carpenter
2015-10-15 11:43 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2015-10-15 11:38 UTC (permalink / raw)
To: lustre-devel
Actually this we can't overflow here, it will return -EFAULT if param is
NULL and paramlen is non-zero. I'll send a patch to clean this up.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] staging: add Lustre file system client support
2015-10-15 11:38 ` Dan Carpenter
@ 2015-10-15 11:43 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-10-15 11:43 UTC (permalink / raw)
To: lustre-devel
On Thu, Oct 15, 2015 at 02:38:45PM +0300, Dan Carpenter wrote:
> Actually this we can't overflow here, it will return -EFAULT if param is
> NULL and paramlen is non-zero. I'll send a patch to clean this up.
Ugh... No, I won't send a patch for this. That LIBCFS_ALLOC() macro is
inscrutible and I am not emotionally prepared to look at it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] staging: add Lustre file system client support
@ 2016-04-27 12:28 Dan Carpenter
2016-05-11 23:53 ` James Simmons
2016-05-13 16:18 ` Drokin, Oleg
0 siblings, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2016-04-27 12:28 UTC (permalink / raw)
To: lustre-devel
Hello Lustre devs,
The patch d7e09d0397e8: "staging: add Lustre file system client
support" from May 2, 2013, leads to the following Parfait warning.
Parfait is an Oracle static analysis tool. If there is a patch from
this could you give credit to Lidza Louina <lidza.louina@oracle.com>?
drivers/staging/lustre/lustre/ldlm/interval_tree.c
399 void interval_erase(struct interval_node *node,
400 struct interval_node **root)
401 {
402 struct interval_node *child, *parent;
403 int color;
404
405 LASSERT(interval_is_intree(node));
406 node->in_intree = 0;
407 if (!node->in_left) {
408 child = node->in_right;
409 } else if (!node->in_right) {
410 child = node->in_left;
411 } else { /* Both left and right child are not NULL */
412 struct interval_node *old = node;
413
414 node = interval_next(node);
^^^^^^^^^^^^^^^^^^^^^^^^^^
It looks like interval_next() can return NULL.
415 child = node->in_right;
416 parent = node->in_parent;
417 color = node->in_color;
418
Here is the interval_next() function:
drivers/staging/lustre/lustre/ldlm/interval_tree.c
111 static struct interval_node *interval_next(struct interval_node *node)
112 {
113 if (!node)
114 return NULL;
115 if (node->in_right)
116 return interval_first(node->in_right);
117 while (node->in_parent && node_is_right_child(node))
^^^^^^^^^^^^^^^
We assume that ->in_parent can be NULL here. Is that actually possible?
118 node = node->in_parent;
119 return node->in_parent;
120 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] staging: add Lustre file system client support
2016-04-27 12:28 Dan Carpenter
@ 2016-05-11 23:53 ` James Simmons
2016-05-13 16:18 ` Drokin, Oleg
1 sibling, 0 replies; 8+ messages in thread
From: James Simmons @ 2016-05-11 23:53 UTC (permalink / raw)
To: lustre-devel
> Hello Lustre devs,
>
> The patch d7e09d0397e8: "staging: add Lustre file system client
> support" from May 2, 2013, leads to the following Parfait warning.
> Parfait is an Oracle static analysis tool. If there is a patch from
> this could you give credit to Lidza Louina <lidza.louina@oracle.com>
Just to let you know we have a ticket to track this issue at:
https://jira.hpdd.intel.com/browse/LU-8128
The reason we have been avoiding this is that in the past small
fixes to the ldlm layer has caused havoc at very large scale.
So we are very careful when approaching this class of fixes.
> drivers/staging/lustre/lustre/ldlm/interval_tree.c
> 399 void interval_erase(struct interval_node *node,
> 400 struct interval_node **root)
> 401 {
> 402 struct interval_node *child, *parent;
> 403 int color;
> 404
> 405 LASSERT(interval_is_intree(node));
> 406 node->in_intree = 0;
> 407 if (!node->in_left) {
> 408 child = node->in_right;
> 409 } else if (!node->in_right) {
> 410 child = node->in_left;
> 411 } else { /* Both left and right child are not NULL */
> 412 struct interval_node *old = node;
> 413
> 414 node = interval_next(node);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> It looks like interval_next() can return NULL.
>
> 415 child = node->in_right;
> 416 parent = node->in_parent;
> 417 color = node->in_color;
> 418
>
> Here is the interval_next() function:
>
> drivers/staging/lustre/lustre/ldlm/interval_tree.c
> 111 static struct interval_node *interval_next(struct interval_node *node)
> 112 {
> 113 if (!node)
> 114 return NULL;
> 115 if (node->in_right)
> 116 return interval_first(node->in_right);
> 117 while (node->in_parent && node_is_right_child(node))
> ^^^^^^^^^^^^^^^
> We assume that ->in_parent can be NULL here. Is that actually possible?
>
> 118 node = node->in_parent;
> 119 return node->in_parent;
> 120 }
>
> regards,
> dan carpenter
> _______________________________________________
> devel mailing list
> devel at linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] staging: add Lustre file system client support
2016-04-27 12:28 Dan Carpenter
2016-05-11 23:53 ` James Simmons
@ 2016-05-13 16:18 ` Drokin, Oleg
1 sibling, 0 replies; 8+ messages in thread
From: Drokin, Oleg @ 2016-05-13 16:18 UTC (permalink / raw)
To: lustre-devel
On Apr 27, 2016, at 8:28 AM, Dan Carpenter wrote:
> Hello Lustre devs,
>
> The patch d7e09d0397e8: "staging: add Lustre file system client
> support" from May 2, 2013, leads to the following Parfait warning.
> Parfait is an Oracle static analysis tool. If there is a patch from
> this could you give credit to Lidza Louina <lidza.louina@oracle.com>?
>
> drivers/staging/lustre/lustre/ldlm/interval_tree.c
> 399 void interval_erase(struct interval_node *node,
> 400 struct interval_node **root)
> 401 {
> 402 struct interval_node *child, *parent;
> 403 int color;
> 404
> 405 LASSERT(interval_is_intree(node));
> 406 node->in_intree = 0;
> 407 if (!node->in_left) {
> 408 child = node->in_right;
> 409 } else if (!node->in_right) {
> 410 child = node->in_left;
> 411 } else { /* Both left and right child are not NULL */
> 412 struct interval_node *old = node;
> 413
> 414 node = interval_next(node);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> It looks like interval_next() can return NULL.
In fact it cannot.
When we are called here, node->in_right is not NULL.
>
> 415 child = node->in_right;
> 416 parent = node->in_parent;
> 417 color = node->in_color;
> 418
>
> Here is the interval_next() function:
>
> drivers/staging/lustre/lustre/ldlm/interval_tree.c
> 111 static struct interval_node *interval_next(struct interval_node *node)
> 112 {
> 113 if (!node)
> 114 return NULL;
> 115 if (node->in_right)
> 116 return interval_first(node->in_right);
So we take this branch here. and interval_first does not return NULL unless node itself is NULL.
Thanks.
Bye,
Oleg
^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] staging: add Lustre file system client support
@ 2016-03-17 20:09 Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2016-03-17 20:09 UTC (permalink / raw)
To: lustre-devel
Hello Peng Tao,
The patch d7e09d0397e8: "staging: add Lustre file system client
support" from May 2, 2013, leads to the following static checker
warning:
drivers/staging/lustre/lustre/obdecho/echo_client.c:1040 cl_echo_enqueue0()
error: 'lck' dereferencing possible ERR_PTR()
drivers/staging/lustre/lustre/obdecho/echo_client.c
1034
1035 lck = cl_lock_request(env, io, descr, "ec enqueue", eco);
^^^^^^^^^^^^^^^
This can return either an ERR_PTR or NULL. It's not clear from the
documentation what that means. There is actually some documentation
for this function though so that's good.
1036 if (lck) {
^^^
Ugh. Success handling.
1037 struct echo_client_obd *ec = eco->eo_dev->ed_ec;
1038 struct echo_lock *el;
1039
1040 rc = cl_wait(env, lck);
1041 if (rc == 0) {
^^^^^^^
More success handling and nested indents. If we flip these around to
test for failure instead the code becomes much clearer.
1042 el = cl2echo_lock(cl_lock_at(lck, &echo_device_type));
1043 spin_lock(&ec->ec_lock);
1044 if (list_empty(&el->el_chain)) {
1045 list_add(&el->el_chain, &ec->ec_locks);
1046 el->el_cookie = ++ec->ec_unique;
1047 }
1048 atomic_inc(&el->el_refcount);
1049 *cookie = el->el_cookie;
1050 spin_unlock(&ec->ec_lock);
1051 } else {
1052 cl_lock_release(env, lck, "ec enqueue", current);
1053 }
1054 }
1055 return rc;
1056 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] staging: add Lustre file system client support
@ 2015-10-15 10:59 Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-10-15 10:59 UTC (permalink / raw)
To: lustre-devel
Hello Lustre Devs,
The patch d7e09d0397e8: "staging: add Lustre file system client
support" from May 2, 2013, leads to the following static checker
warning:
drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c:150 libcfs_kkuc_group_rem()
error: buffer overflow 'kkuc_groups' 3 <= s32max
drivers/staging/lustre/lustre/libcfs/kernel_user_comm.c
146 int libcfs_kkuc_group_rem(int uid, int group)
147 {
148 struct kkuc_reg *reg, *next;
149
150 if (kkuc_groups[group].next == NULL)
group doesn't appear to have been validated@all. It comes from the
user. The call tree is:
-> lmv_iocontrol()
-> lmv_hsm_ct_unregister()
-> libcfs_kkuc_group_rem()
It looks like this code could oops.
151 return 0;
152
153 if (uid == 0) {
154 /* Broadcast a shutdown message */
155 struct kuc_hdr lh;
156
157 lh.kuc_magic = KUC_MAGIC;
158 lh.kuc_transport = KUC_TRANSPORT_GENERIC;
159 lh.kuc_msgtype = KUC_MSG_SHUTDOWN;
160 lh.kuc_msglen = sizeof(lh);
161 libcfs_kkuc_group_put(group, &lh);
162 }
163
164 down_write(&kg_sem);
165 list_for_each_entry_safe(reg, next, &kkuc_groups[group], kr_chain) {
166 if ((uid == 0) || (uid == reg->kr_uid)) {
167 list_del(®->kr_chain);
168 CDEBUG(D_KUC, "Removed uid=%d fp=%p from group %d\n",
169 reg->kr_uid, reg->kr_fp, group);
170 if (reg->kr_fp != NULL)
171 fput(reg->kr_fp);
172 kfree(reg);
173 }
174 }
175 up_write(&kg_sem);
176
177 return 0;
178 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-05-13 16:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-15 11:14 [lustre-devel] staging: add Lustre file system client support Dan Carpenter
2015-10-15 11:38 ` Dan Carpenter
2015-10-15 11:43 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2016-04-27 12:28 Dan Carpenter
2016-05-11 23:53 ` James Simmons
2016-05-13 16:18 ` Drokin, Oleg
2016-03-17 20:09 Dan Carpenter
2015-10-15 10:59 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.