Skip to content

Commit df593ee

Browse files
dhowellsdhowells
committed
keys: Hoist locking out of __key_link_begin()
Hoist the locking of out of __key_link_begin() and into its callers. This is necessary to allow the upcoming key_move() operation to correctly order taking of the source keyring semaphore, the destination keyring semaphore and the keyring serialisation lock. Signed-off-by: David Howells <[email protected]>
1 parent eb0f68c commit df593ee

File tree

4 files changed

+76
-38
lines changed

4 files changed

+76
-38
lines changed

‎security/keys/internal.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ extern wait_queue_head_t request_key_conswq;
9393
extern struct key_type *key_type_lookup(const char *type);
9494
extern void key_type_put(struct key_type *ktype);
9595

96+
extern int __key_link_lock(struct key *keyring,
97+
const struct keyring_index_key *index_key);
9698
extern int __key_link_begin(struct key *keyring,
9799
const struct keyring_index_key *index_key,
98100
struct assoc_array_edit **_edit);

‎security/keys/key.c‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ int key_instantiate_and_link(struct key *key,
500500
struct key *authkey)
501501
{
502502
struct key_preparsed_payload prep;
503-
struct assoc_array_edit *edit;
503+
struct assoc_array_edit *edit = NULL;
504504
int ret;
505505

506506
memset(&prep, 0, sizeof(prep));
@@ -515,10 +515,14 @@ int key_instantiate_and_link(struct key *key,
515515
}
516516

517517
if (keyring) {
518-
ret = __key_link_begin(keyring, &key->index_key, &edit);
518+
ret = __key_link_lock(keyring, &key->index_key);
519519
if (ret < 0)
520520
goto error;
521521

522+
ret = __key_link_begin(keyring, &key->index_key, &edit);
523+
if (ret < 0)
524+
goto error_link_end;
525+
522526
if (keyring->restrict_link && keyring->restrict_link->check) {
523527
struct key_restriction *keyres = keyring->restrict_link;
524528

@@ -570,7 +574,7 @@ int key_reject_and_link(struct key *key,
570574
struct key *keyring,
571575
struct key *authkey)
572576
{
573-
struct assoc_array_edit *edit;
577+
struct assoc_array_edit *edit = NULL;
574578
int ret, awaken, link_ret = 0;
575579

576580
key_check(key);
@@ -583,7 +587,12 @@ int key_reject_and_link(struct key *key,
583587
if (keyring->restrict_link)
584588
return -EPERM;
585589

586-
link_ret = __key_link_begin(keyring, &key->index_key, &edit);
590+
link_ret = __key_link_lock(keyring, &key->index_key);
591+
if (link_ret == 0) {
592+
link_ret = __key_link_begin(keyring, &key->index_key, &edit);
593+
if (link_ret < 0)
594+
__key_link_end(keyring, &key->index_key, edit);
595+
}
587596
}
588597

589598
mutex_lock(&key_construction_mutex);
@@ -810,7 +819,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
810819
.description = description,
811820
};
812821
struct key_preparsed_payload prep;
813-
struct assoc_array_edit *edit;
822+
struct assoc_array_edit *edit = NULL;
814823
const struct cred *cred = current_cred();
815824
struct key *keyring, *key = NULL;
816825
key_ref_t key_ref;
@@ -860,12 +869,18 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
860869
}
861870
index_key.desc_len = strlen(index_key.description);
862871

863-
ret = __key_link_begin(keyring, &index_key, &edit);
872+
ret = __key_link_lock(keyring, &index_key);
864873
if (ret < 0) {
865874
key_ref = ERR_PTR(ret);
866875
goto error_free_prep;
867876
}
868877

878+
ret = __key_link_begin(keyring, &index_key, &edit);
879+
if (ret < 0) {
880+
key_ref = ERR_PTR(ret);
881+
goto error_link_end;
882+
}
883+
869884
if (restrict_link && restrict_link->check) {
870885
ret = restrict_link->check(keyring, index_key.type,
871886
&prep.payload, restrict_link->key);

‎security/keys/keyring.c‎

Lines changed: 47 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1199,14 +1199,34 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
11991199
return PTR_ERR(ctx.result) == -EAGAIN ? 0 : PTR_ERR(ctx.result);
12001200
}
12011201

1202+
/*
1203+
* Lock keyring for link.
1204+
*/
1205+
int __key_link_lock(struct key *keyring,
1206+
const struct keyring_index_key *index_key)
1207+
__acquires(&keyring->sem)
1208+
__acquires(&keyring_serialise_link_lock)
1209+
{
1210+
if (keyring->type != &key_type_keyring)
1211+
return -ENOTDIR;
1212+
1213+
down_write(&keyring->sem);
1214+
1215+
/* Serialise link/link calls to prevent parallel calls causing a cycle
1216+
* when linking two keyring in opposite orders.
1217+
*/
1218+
if (index_key->type == &key_type_keyring)
1219+
mutex_lock(&keyring_serialise_link_lock);
1220+
1221+
return 0;
1222+
}
1223+
12021224
/*
12031225
* Preallocate memory so that a key can be linked into to a keyring.
12041226
*/
12051227
int __key_link_begin(struct key *keyring,
12061228
const struct keyring_index_key *index_key,
12071229
struct assoc_array_edit **_edit)
1208-
__acquires(&keyring->sem)
1209-
__acquires(&keyring_serialise_link_lock)
12101230
{
12111231
struct assoc_array_edit *edit;
12121232
int ret;
@@ -1215,20 +1235,13 @@ int __key_link_begin(struct key *keyring,
12151235
keyring->serial, index_key->type->name, index_key->description);
12161236

12171237
BUG_ON(index_key->desc_len == 0);
1238+
BUG_ON(*_edit != NULL);
12181239

1219-
if (keyring->type != &key_type_keyring)
1220-
return -ENOTDIR;
1221-
1222-
down_write(&keyring->sem);
1240+
*_edit = NULL;
12231241

12241242
ret = -EKEYREVOKED;
12251243
if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
1226-
goto error_krsem;
1227-
1228-
/* serialise link/link calls to prevent parallel calls causing a cycle
1229-
* when linking two keyring in opposite orders */
1230-
if (index_key->type == &key_type_keyring)
1231-
mutex_lock(&keyring_serialise_link_lock);
1244+
goto error;
12321245

12331246
/* Create an edit script that will insert/replace the key in the
12341247
* keyring tree.
@@ -1239,7 +1252,7 @@ int __key_link_begin(struct key *keyring,
12391252
NULL);
12401253
if (IS_ERR(edit)) {
12411254
ret = PTR_ERR(edit);
1242-
goto error_sem;
1255+
goto error;
12431256
}
12441257

12451258
/* If we're not replacing a link in-place then we're going to need some
@@ -1258,11 +1271,7 @@ int __key_link_begin(struct key *keyring,
12581271

12591272
error_cancel:
12601273
assoc_array_cancel_edit(edit);
1261-
error_sem:
1262-
if (index_key->type == &key_type_keyring)
1263-
mutex_unlock(&keyring_serialise_link_lock);
1264-
error_krsem:
1265-
up_write(&keyring->sem);
1274+
error:
12661275
kleave(" = %d", ret);
12671276
return ret;
12681277
}
@@ -1312,9 +1321,6 @@ void __key_link_end(struct key *keyring,
13121321
BUG_ON(index_key->type == NULL);
13131322
kenter("%d,%s,", keyring->serial, index_key->type->name);
13141323

1315-
if (index_key->type == &key_type_keyring)
1316-
mutex_unlock(&keyring_serialise_link_lock);
1317-
13181324
if (edit) {
13191325
if (!edit->dead_leaf) {
13201326
key_payload_reserve(keyring,
@@ -1323,6 +1329,9 @@ void __key_link_end(struct key *keyring,
13231329
assoc_array_cancel_edit(edit);
13241330
}
13251331
up_write(&keyring->sem);
1332+
1333+
if (index_key->type == &key_type_keyring)
1334+
mutex_unlock(&keyring_serialise_link_lock);
13261335
}
13271336

13281337
/*
@@ -1358,25 +1367,32 @@ static int __key_link_check_restriction(struct key *keyring, struct key *key)
13581367
*/
13591368
int key_link(struct key *keyring, struct key *key)
13601369
{
1361-
struct assoc_array_edit *edit;
1370+
struct assoc_array_edit *edit = NULL;
13621371
int ret;
13631372

13641373
kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
13651374

13661375
key_check(keyring);
13671376
key_check(key);
13681377

1378+
ret = __key_link_lock(keyring, &key->index_key);
1379+
if (ret < 0)
1380+
goto error;
1381+
13691382
ret = __key_link_begin(keyring, &key->index_key, &edit);
1370-
if (ret == 0) {
1371-
kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
1372-
ret = __key_link_check_restriction(keyring, key);
1373-
if (ret == 0)
1374-
ret = __key_link_check_live_key(keyring, key);
1375-
if (ret == 0)
1376-
__key_link(key, &edit);
1377-
__key_link_end(keyring, &key->index_key, edit);
1378-
}
1383+
if (ret < 0)
1384+
goto error_end;
1385+
1386+
kdebug("begun {%d,%d}", keyring->serial, refcount_read(&keyring->usage));
1387+
ret = __key_link_check_restriction(keyring, key);
1388+
if (ret == 0)
1389+
ret = __key_link_check_live_key(keyring, key);
1390+
if (ret == 0)
1391+
__key_link(key, &edit);
13791392

1393+
error_end:
1394+
__key_link_end(keyring, &key->index_key, edit);
1395+
error:
13801396
kleave(" = %d {%d,%d}", ret, keyring->serial, refcount_read(&keyring->usage));
13811397
return ret;
13821398
}

‎security/keys/request_key.c‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
343343
struct key_user *user,
344344
struct key **_key)
345345
{
346-
struct assoc_array_edit *edit;
346+
struct assoc_array_edit *edit = NULL;
347347
struct key *key;
348348
key_perm_t perm;
349349
key_ref_t key_ref;
@@ -372,6 +372,9 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
372372
set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
373373

374374
if (dest_keyring) {
375+
ret = __key_link_lock(dest_keyring, &ctx->index_key);
376+
if (ret < 0)
377+
goto link_lock_failed;
375378
ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
376379
if (ret < 0)
377380
goto link_prealloc_failed;
@@ -423,6 +426,8 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
423426
return ret;
424427

425428
link_prealloc_failed:
429+
__key_link_end(dest_keyring, &ctx->index_key, edit);
430+
link_lock_failed:
426431
mutex_unlock(&user->cons_lock);
427432
key_put(key);
428433
kleave(" = %d [prelink]", ret);

0 commit comments

Comments
 (0)