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.

Sign up for GitHub

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

Jump to bottom

Virtiofs structs might be allocated with wrong alignment #691

Closed
mkroening opened this issue Mar 23, 2023 · 4 comments · Fixed by #709
Closed

Virtiofs structs might be allocated with wrong alignment #691

mkroening opened this issue Mar 23, 2023 · 4 comments · Fixed by #709
Assignees
@joannejchen
Labels
soundness Something is unsound.

Comments

@mkroening
Copy link
Member

mkroening commented Mar 23, 2023 •
edited
Loading

There are places that manually allocate fuse DSTs with this layout:

https://github.com/hermitcore/libhermit-rs/blob/4a9c2c4f4b3df5a335286939e5eff7f7484d331f/src/fs/fuse.rs#L372

where Cmd is defined like this:

https://github.com/hermitcore/libhermit-rs/blob/4a9c2c4f4b3df5a335286939e5eff7f7484d331f/src/fs/fuse.rs#L291-L297

The layout of the manual allocation has to exactly match the layout returned by Layout::from_value of the allocated value (this is something we should add assertions for).

Specifically, the alignment might be wrong here for certain fields.
The alignment of a parent struct is not the alignment of the first field as assumed in this code, but the biggest alignment of all fields.
So instead of just the alignment of the first field (fuse_in_header in this case), we should request the maximum alignment of all fields.
@joannejchen
Copy link
Contributor

Hi @mkroening! @scotran and I are part of a Virtualization course at UT Austin, and as part of a project we have to contribute to open source repositories. Can we take up this issue?

@mkroening mkroening added the soundness Something is unsound. label Mar 26, 2023
@mkroening
Copy link
Member Author

Go for it! :)

joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 15, 2023
@joannejchen
Change alignment of virtiofs structs to be based on the first field.
dd021d4 
Alignment of the virtiofs struct was previously based on the alignment
of the largest field in fuse_in_header/fuse_out_header. Switched to
explicitly align on u32, the type of the first field in the header.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
@joannejchen joannejchen mentioned this issue Apr 15, 2023
Merged
@joannejchen
Copy link
Contributor

Hi @mrkroening! I created a pull request (#709) with the alignment switched to be based off of the u32 type (the type of the first field in fuse_in_header / fuse_out_header). Are these the changes you intended?

@mkroening mkroening changed the title Virtiofs structs are allocated with possibly wrong alignment Virtiofs structs might be allocated with wrong alignment Apr 16, 2023
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
46f75df 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
e85ddb0 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
554649c 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
d987bd9 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
0ff5752 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
joannejchen added a commit to joannejchen/libhermit-rs that referenced this issue Apr 18, 2023
@joannejchen
Change alignment of virtiofs structs to maximum alignment of all fields.
9095ed0 
Alignment of the virtiofs struct was previously based on the alignment
of only the first field. Changed to explicitly use the maximum alignment of all of
the fields and added assertions to ensure this manually-allocated layout
matches the compiler-generated layout.

Fixes hermit-os#691

Signed-off-by: joannejchen <chenjjoanne@gmail.com>
@joannejchen
Copy link
Contributor

Thanks for the clarification! I have updated it to pad the layout (and consequently changes the asserts and passing around of the layout as you mentioned), and luckily it doesn't seem to have broken anything!

LMK if there's any other issues or fixes!

bors bot added a commit that referenced this issue Apr 19, 2023
@bors @joannejchen
Merge #709
df7483d 
709: Change alignment of virtiofs structs to be based on the first field. r=mkroening a=joannejchen

Alignment of the virtiofs struct was previously based on the alignment of the largest field in fuse_in_header/fuse_out_header. Switched to explicitly align on u32, the type of the first field in the header.

Fixes #691

Co-authored-by: joannejchen <chenjjoanne@gmail.com>
@bors bors bot closed this as completed in d8745ad Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joannejchen joannejchen

Labels
soundness Something is unsound.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change alignment of virtiofs structs to be based on the first field. joannejchen/libhermit-rs
2 participants
@mkroening @joannejchen