311577 - PK11_InitPin sets slot->lastLoginCheck without holding a lock

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

When I read the code in lib/pk11wrap/pk11auth.c,
I got the impression that slot->lastLoginCheck
is always set while we are holding the slot lock or
a RW session, with only one exception:

In PK11_InitPin, we have:

    /* get a rwsession */
    rwsession = PK11_GetRWSession(slot);
    if (rwsession == CK_INVALID_SESSION) {
        PORT_SetError(SEC_ERROR_BAD_DATA);
        slot->lastLoginCheck = 0;
        return rv;
    }

Should the "slot->lastLoginCheck = 0;" statement
be removed or be done inside a PK11_EnterSlotMonitor(slot)
PK11_ExitSlotMonitor(slot) block?

Compare PK11_InitPin with PK11_CheckSSOPassword, which
has a similar structure.  The corresponding code in
PK11_CheckSSOPassword is:

    /* get a rwsession */
    rwsession = PK11_GetRWSession(slot);
    if (rwsession == CK_INVALID_SESSION) {
        PORT_SetError(SEC_ERROR_BAD_DATA);
        return rv;
    }

Comment 1

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 2

[Lars Eggert [:lars]]

Attachment: Bug 311577 - PK11_InitPin sets slot->lastLoginCheck without holding the slot monitor r=beurdouche,jschanck,#nss-reviewers

PK11_IsLoggedIn reads slot->lastLoginCheck under PK11_EnterSlotMonitor, but
PK11_InitPin writes it in three places without holding the monitor, causing a
data race. Wrap all three writes with PK11_EnterSlotMonitor/PK11_ExitSlotMonitor.

Comment 3

[Lars Eggert [:lars]]

https://phabricator.services.mozilla.com/D300715 triggers this repeatably.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/a8286355abeb
PK11_InitPin sets slot->lastLoginCheck without holding the slot monitor r=jschanck,nss-reviewers

Comment 6

[John Schanck [:jschanck]]

Attachment: Bug 311577 - drop slot monitor in PK11_ResetToken before calling PK11_InitToken. r=#nss-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/80c777852ce1
drop slot monitor in PK11_ResetToken before calling PK11_InitToken. r=nss-reviewers,keeler

Comment 8

[John Schanck [:jschanck]]

Attachment: Bug 311577 - drop slot monitor in pk11_CheckPassword before calling PK11_InitToken. r=#nss-reviewers.

Comment 9

[BugBot [:suhaib / :marco]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)