https://tbpl.mozilla.org/php/getParsedLog.php?id=41403307&tree=Date Only on Win64 debug. After the call to PK11_GenerateKeyPairWithFlags [1], rbx is supposed to be |this| but instead it's pointing into freebl3 code. [1] http://hg.mozilla.org/mozilla-central/file/80431d4fd0da/security/manager/ssl/src/nsKeygenThread.cpp#l189

Found it. In s_mp_sqr_comba_4, the |push rbx| from line 7872 needs to happen before the |sub rsp, 80| at line 7866. http://hg.mozilla.org/mozilla-central/file/80431d4fd0da/security/nss/lib/freebl/mpi/mp_comba_amd64_masm.asm#l7857 How does freebl work? Do we just change the code directly, or is there some upstream for this?

Moving the |push rbx| also fixes the SSL xpcshell test failures in: https://tbpl.mozilla.org/php/getParsedLog.php?id=41409112&tree=Date

Haven't heard from Kai. Adding some other security folks: how can I get a change into freebl3?

(In reply to David Major [:dmajor] from comment #3) > Haven't heard from Kai. Adding some other security folks: how can I get a > change into freebl3? Please attach a patch to the bug and ask one of the people who has most recently committed a change to that file to review the patch using the review? flag. I am not the best reviewer for this, because I haven't looked at or touched that code.

I know how to submit patches in general. But all of the source history in this area goes like "Update to NSS <version>" so it sounds like I should really be making this change somewhere other than m-c. That's what I'm trying to figure out.

This is the patch based on m-c.

(In reply to David Major [:dmajor] from comment #5) > I know how to submit patches in general. But all of the source history in > this area goes like "Update to NSS <version>" so it sounds like I should > really be making this change somewhere other than m-c. That's what I'm > trying to figure out. https://hg.mozilla.org/projects/nss is the upstream NSS repository. It is best if you can post patches against that repository instead of mozilla-central, but people can also generally figure out how to deal with a mozilla-central patch (patch -p3, IIRC). Please use the changelog of the file in question from the https://hg.mozilla.org/projects/nss repository to pick a reviewer.

Aha. That's the information I was looking for. Thanks!

I guess Makoto would be the best reviewer, based on https://hg.mozilla.org/projects/nss/annotate/777ff0acc9e8/lib/freebl/mpi/mp_comba_amd64_masm.asm#l7854

Comment on attachment 8445665 [details] [diff] [review] Fix register saving for amd64 Review of attachment 8445665 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/freebl/mpi/mp_comba_amd64_masm.asm @@ +7862,5 @@ > mov rdi, rcx > mov rsi, rdx > > push rbp > + push rbx David: thanks for the patch. Could you explain this change? rbx is not used until line 7873: mov rbx, rsi So it seems fine to push rbx on line 7872. Is it because of the side effect of line 7867? sub rsp, 80

(In reply to Wan-Teh Chang from comment #10) Yes, it's because of the sub instruction. The order needs to match the order of the epilogue: add rsp, 80 pop rbx pop rbp mov dword ptr [r11], eax pop rsi pop rdi

Can you push this to nss for me? Thanks!

Comment on attachment 8445665 [details] [diff] [review] Fix register saving for amd64 r=wtc. Patch checked in: https://hg.mozilla.org/projects/nss/rev/81e8b725bf7a David: this patch should appear in mozilla-central within two weeks as part of a regular NSS update. Do you need it in m-c sooner?

> David: this patch should appear in mozilla-central within two weeks as > part of a regular NSS update. Do you need it in m-c sooner? Two weeks should be fine, thanks for the checkin.

If we're dealing with garbage memory, it seems like this should be a sec bug. Is that not right?

No objection. You would know better than me.

Comment on attachment 8445665 [details] [diff] [review] Fix register saving for amd64 Patch pushed to mozilla-inbound as part of NSS_3_17_BETA1: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e672df388ed

If this is Win64 only, it sounds like we don't need to bother backporting it.

Can we remove the security flag from this? It's been several weeks of nightlies now.

for nss 3.16.4: https://hg.mozilla.org/projects/nss/rev/0f6effa542e6