crypto/nss_util: Get TPM slot id, do lookup by id instead of by name.

crypto/nss_util: Get TPM slot id, do lookup by id instead of by name.

chromeos/cert_loader: store slot id as int.

BUG=302124
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231126

[mattm, 2013-10-23 05:55:03]

satorux: chromeos/
rsleevi: crypto/

Splitting a little chunk off of my big WIP CL.

[mattm, 2013-10-23 23:05:32]

Updated with a fix for the chromeos_unittests problem.

(When loading chaps on a desktop, SECMOD_LoadUserModule would return a non-NULL
value, but the "loaded" member was false, so the SECMOD_UpdateSlotList call
segfaulted. Even after fixing it not to try looking up the slot if the module
wasn't fully loaded, the tests still failed since they expected the old tpm slot
look-up by name to find the test db slot. I just hacked it so the
InitializeTPMToken will automatically use the test_slot_ if it is set and
initializing the chaps module fails.)

[Ryan Sleevi, 2013-10-24 01:26:21]

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:277: tpm_slot_ = GetTPMSlotForId(token_slot_id);
CASTING DANGER: CK_SLOT_ID is a CK_ULONG - which is PKCS#11-header/platform
specific, but at least 32 bits

For NSS, it's an unsigned long int.

You're casting a (signed) int up here. While I realize this is "probably" safe,
by virtue of it only talking to Chaps, the API divergence is at least worth
noting (perhaps in the comments for GetTPMSlotForID or here)

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util_internal...
crypto/nss_util_internal.h:44: CRYPTO_EXPORT PK11SlotInfo*
GetTPMSlotForId(CK_SLOT_ID slot_id);
I'm not sure why you need to expose this - did I miss where it was actually
called outside of nss_util ?

[mattm, 2013-10-24 01:59:07]

Updated.

(note: still hunting down the test problem. Seems like a sandbox is trying to
use nss without calling ForceNSSNoDBInit or InitNSSSafely, but it doesn't repro
locally.)

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:277: tpm_slot_ = GetTPMSlotForId(token_slot_id);
On 2013/10/24 01:26:21, Ryan Sleevi wrote:
> CASTING DANGER: CK_SLOT_ID is a CK_ULONG - which is PKCS#11-header/platform
> specific, but at least 32 bits
> 
> For NSS, it's an unsigned long int.
> 
> You're casting a (signed) int up here. While I realize this is "probably"
safe,
> by virtue of it only talking to Chaps, the API divergence is at least worth
> noting (perhaps in the comments for GetTPMSlotForID or here)

Done.

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/36593002/diff/130001/crypto/nss_util_internal...
crypto/nss_util_internal.h:44: CRYPTO_EXPORT PK11SlotInfo*
GetTPMSlotForId(CK_SLOT_ID slot_id);
On 2013/10/24 01:26:21, Ryan Sleevi wrote:
> I'm not sure why you need to expose this - did I miss where it was actually
> called outside of nss_util ?

Yeah, it's not in this CL. I'll remove it for now.

[mattm, 2013-10-24 05:12:26]

I "fixed" the problem by removing the NOTREACHED (since it was already broken
before, just not hitting the NOTREACHED.) We can re-add the NOTREACHED once
issue 310972 is fixed.

[mattm, 2013-10-24 21:43:24]

+stevenjb for chromeos/network/

[stevenjb, 2013-10-24 22:11:00]

chromeos/ changes lgtm

[mattm, 2013-10-24 22:21:30]

+wtc for crypto/ since rsleevi went OOO.

[satorux1, 2013-10-25 06:54:27]

I'm not familiar with this code. I'd defer to stevenjb@

[wtc, 2013-10-25 18:06:27]

Patch set 8 LGTM.

https://codereview.chromium.org/36593002/diff/370002/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/36593002/diff/370002/chromeos/cert_loader.h#n...
chromeos/cert_loader.h:103: int tpm_token_slot() const { return tpm_token_slot_;
}

Can we use unsigned long?

I suggest renaming the member and its getter "tpm_token_slot_id*" to make it
clear it is an integer ID now.

https://codereview.chromium.org/36593002/diff/370002/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/36593002/diff/370002/crypto/nss_util.h#newcod...
crypto/nss_util.h:120: int token_slot_id);

Nit: I suggest listing token_slot_id immediately after token_name because they
are strongly related.

[mattm, 2013-10-25 18:41:36]

https://codereview.chromium.org/36593002/diff/370002/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/36593002/diff/370002/chromeos/cert_loader.h#n...
chromeos/cert_loader.h:103: int tpm_token_slot() const { return tpm_token_slot_;
}
On 2013/10/25 18:06:27, wtc wrote:
> 
> Can we use unsigned long?

I just asked Darren about this.  I'll file a bug for it unless he has some
reason not to.

> I suggest renaming the member and its getter "tpm_token_slot_id*" to make it
> clear it is an integer ID now.

Done.

https://codereview.chromium.org/36593002/diff/370002/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/36593002/diff/370002/crypto/nss_util.h#newcod...
crypto/nss_util.h:120: int token_slot_id);
On 2013/10/25 18:06:27, wtc wrote:
> 
> Nit: I suggest listing token_slot_id immediately after token_name because they
> are strongly related.

Done.

[commit-bot: I haz the power, 2013-10-25 19:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/36593002/630001

[commit-bot: I haz the power, 2013-10-25 22:03:29]

Change committed as 231126