Skip to content

ICP: Fix out of bounds write #9660

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

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Conversation

AttilaFueloep
Copy link
Contributor

Motivation and Context

Bug fix.

Description

If gcm_mode_encrypt_contiguous_blocks() is called more than once in succession, with the accumulated lengths being less than blocksize, ctx->copy_to will be incorrectly advanced. Later, if out is NULL, bcopy() will overflow ctx->gcm_copy_to since ctx->gcm_remainder_len is larger than the ctx->gcm_copy_to buffer can hold.

For ZoL the issue may be academic, since in all my testing I wasn't able to hit neither of both conditions needed to trigger it, but other consumers can easily do so.

The fix is to set ctx->copy_to only if it's not already set.

How Has This Been Tested?

As part of another branch this survived 6 hours of zloop. Currently zloop fails with unmodified master within one hour, so can't use zloop to test this PR, sorry.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

If gcm_mode_encrypt_contiguous_blocks() is called more than once
in succession, with the accumulated lengths being less than
blocksize, ctx->copy_to will be incorrectly advanced. Later, if
out is NULL, the bcopy at line 114 will overflow
ctx->gcm_copy_to since ctx->gcm_remainder_len is larger than the
ctx->gcm_copy_to buffer can hold.

The fix is to set ctx->copy_to only if it's not already set.

For ZoL the issue may be academic, since in all my testing I wasn't
able to hit neither of both conditions needed to trigger it, but
other consumers can easily do so.

Signed-off-by: Attila Fülöp <[email protected]>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 2, 2019
@behlendorf behlendorf added the Component: Encryption "native encryption" feature label Dec 2, 2019
@behlendorf behlendorf requested a review from tcaputi December 2, 2019 20:46
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #9660 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9660      +/-   ##
==========================================
+ Coverage   79.32%   79.34%   +0.01%     
==========================================
  Files         418      418              
  Lines      123544   123544              
==========================================
+ Hits        98003    98026      +23     
+ Misses      25541    25518      -23
Flag Coverage Δ
#kernel 79.97% <0%> (+0.02%) ⬆️
#user 66.71% <0%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5142032...bdd5e10. Read the comment docs.

@AttilaFueloep
Copy link
Contributor Author

With unmodified master ztest fails in my environment as well, so I think it's not related to this change.

Copy link
Contributor

@tcaputi tcaputi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me. This does raise some interesting questions though about pushing ICP fixes back to Illumos.

@AttilaFueloep
Copy link
Contributor Author

Right, I already sent a mail to illumos-devel. Let's see if someone chimes in.

@AttilaFueloep
Copy link
Contributor Author

Illumos issue 12048.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ztest appear to be unrelated, but I've resubmitted those builds to the CI for good measure. Thanks for following up with illumos so they can comment on and track the proposed fix.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 5, 2019
@behlendorf behlendorf merged commit 3ac34ca into openzfs:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Encryption "native encryption" feature Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants