Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zero the stack after AVX-512 XTS #1415

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Zero the stack after AVX-512 XTS #1415

merged 1 commit into from
Jan 30, 2024

Conversation

pittma
Copy link
Contributor

@pittma pittma commented Jan 22, 2024

Description of changes:

A small patch to zero out the used stack frames in the AVX-512 XTS implementation.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@pittma pittma requested a review from a team as a code owner January 22, 2024 21:16
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eaa19c7) 76.89% compared to head (3af4098) 76.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   76.89%   76.85%   -0.05%     
==========================================
  Files         425      425              
  Lines       71527    71527              
==========================================
- Hits        55000    54970      -30     
- Misses      16527    16557      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nebeid
Copy link
Contributor

nebeid commented Jan 23, 2024

Thank you, Dan @pittma.
I believe we could add:

  • clearing the locations on the stack where the registers were backed up, taking into account the extra ones in the case of win64. This is done in aesni-x86_64.pl
  • this change and the suggestion above to the decryption function.

@pittma
Copy link
Contributor Author

pittma commented Jan 24, 2024

As far as I can tell, this CI failure is unrelated. It looks like HEAD moved a few hours ago for the tpm2 dependency in this test, and now there is a conflict with the patch:

commit 84ead1b7a35362d5fae9b26a0edb9a68eeb6e5c8 (HEAD -> master, origin/master, origin/HEAD)
Author: Juergen Repp <[email protected]>
Date:   Tue Jan 23 12:00:45 2024 +0100

    FAPI: Clean command context before execution.
    
    The FAPI command context now is cleared in the async function of
    every command.
    
    Signed-off-by: Juergen Repp <[email protected]>

This is the error I'm seeing:

$ git apply /home/dpitt/src/aws-lc/tests/ci/integration/tpm2_tss_patch/aws-lc-tpm2-tss.patch
error: patch failed: test/unit/fapi-eventlog.c:138
error: test/unit/fapi-eventlog.c: patch does not apply

@torben-hansen
Copy link
Contributor

As far as I can tell, this CI failure is unrelated. It looks like HEAD moved a few hours ago for the tpm2 dependency in this test, and now there is a conflict with the patch:

commit 84ead1b7a35362d5fae9b26a0edb9a68eeb6e5c8 (HEAD -> master, origin/master, origin/HEAD)
Author: Juergen Repp <[email protected]>
Date:   Tue Jan 23 12:00:45 2024 +0100

    FAPI: Clean command context before execution.
    
    The FAPI command context now is cleared in the async function of
    every command.
    
    Signed-off-by: Juergen Repp <[email protected]>

This is the error I'm seeing:

$ git apply /home/dpitt/src/aws-lc/tests/ci/integration/tpm2_tss_patch/aws-lc-tpm2-tss.patch
error: patch failed: test/unit/fapi-eventlog.c:138
error: test/unit/fapi-eventlog.c: patch does not apply

Resolved here: #1422

@nebeid
Copy link
Contributor

nebeid commented Jan 25, 2024

I think the comments that used to be there before line 1881 for the special case of zeroising were helpful:

# Stack usage isn't divisible by 64, so we use a kmask register to
# only use 48 of the bytes (6 quad-words).

The prior comments were good too.

@pittma
Copy link
Contributor Author

pittma commented Jan 26, 2024

I think the comments that used to be there before line 1881 for the special case of zeroising were helpful:

# Stack usage isn't divisible by 64, so we use a kmask register to
# only use 48 of the bytes (6 quad-words).

The prior comments were good too.

Thanks for saying so @nebeid. I'd mostly left them as mnemonics for myself and I began to fear I was the only one! I've added them back.

@nebeid nebeid merged commit 67d3e50 into aws:main Jan 30, 2024
37 checks passed
@pittma pittma deleted the xts-stack branch September 9, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants