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

Force linking with spectre-mitigated libraries if /Qspectre is passed. #687

Closed
wants to merge 1 commit into from

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Jun 11, 2022

This is a kind of alternative to #673. I write "kind of" because it's not in direct opposition, but rather just a more sensible and robust approach.

I mark it as draft, because it needs more work, and maybe even further amendments based on feedback.

  • a warning would be appropriate in case spectre-mitigated libraries are not installed;
  • the section above, one that attempts to add reference to ATL, seems to be broken for contemporary compilers;
  • examine %LIB% environment variable, combine it with the suggested list, and deduplicate the result;
  • should there be a "global" method that would trigger linking with spectre-mitigated libraries in case the user doesn't compile any C/C++?
  • ...

@dot-asm
Copy link
Contributor Author

dot-asm commented Jun 11, 2022

  • should there be a "global" method that would trigger linking with spectre-mitigated libraries in case the user doesn't compile any C/C++?

Just in case, my personal opinion is "no." For the following reason. Rust-generated code has very weak dependency on the MSVC run-time. And as far as I can tell this small amount of dependencies doesn't actually require [or even have] the said mitigations. In other words, it makes sense to link with spectre-mitigated libraries only if you actually compile some C/C++. This makes the /Qspectre option passed to the compiler an adequate and sufficient trigger.

@ChrisDenton
Copy link
Member

I think we should aim to be consistent with the environment unless specified otherwise. This PR does not conflict with that but neither does it obsolete that method. With this PR alone we will sometimes follow the user intent (when /Qspectre is directly specified as an argument) and sometimes we will ignore it (when it's specified in the environment).

Btw, Rust practically always links to at least some C/C++ code. At a minimum, the vcruntime (including startup code and panics). Well, unless they're using no-std and providing their own entry point.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 11, 2022

Rust practically always links to at least some C/C++ code. At a minimum, the vcruntime (including startup code and panics).

Yes, I acknowledge that with "Rust-generated code has very weak dependency on the MSVC run-time. And as far as I can tell this small amount of dependencies doesn't actually require [or even have] the said mitigations."

Well, unless they're using no-std and providing their own entry point.

I explored the extent of the said dependency in https://github.com/dot-asm/min-crt-poc.

@ChrisDenton
Copy link
Member

Specifically I don't understand how you draw this conclusion:

And as far as I can tell this small amount of dependencies doesn't actually require [or even have] the said mitigations.

Exception handling is a fairly large surface area so, even if the startup code and mem* functions don't employ any mitigations, I would expect that SEH does,

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 11, 2022

Specifically I don't understand how you draw this conclusion:

And as far as I can tell this small amount of dependencies doesn't actually require [or even have] the said mitigations.

min-crt-poc demonstrates that pure Rust code can interact with Rust std in non-trivial manner (for example do things like spawning threads), and at the same time not have references to vcruntime. And the assertion is that none of the code implemented in dllmain.rs (that would otherwise come from vcruntime) require spectre mitigations. Nor does it [the corresponding code] seem to have ones in the vcruntime itself.

Exception handling is a fairly large surface area.

And how much of the said surface is used by pure Rust code? My assessment is effectively none. Well, not that I'm saying that it's the actual case, but I failed to spot a meaningful dependency... Can you think of an example? I mean of Rust code that would rely on exception handling...

@ChrisDenton
Copy link
Member

Panics are implemented in terms of SEH. For msvc toolchains, rust/llvm hard codes a dependency on _CxxThrowException and __CxxFrameHandler3. See https://github.com/rust-lang/rust/blob/25b764849625cb090e8b81d12d2bb2295d073788/library/panic_unwind/src/seh.rs

Even if a project doesn't use panics and the mem* functions don't have spectre mitigations then there is still the case of linking to other pre-built C/C+ libs.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 11, 2022

Just in case. Suggestion is not to use dllmain.rs from min-crt-poc in any real-life scenario. Its sole purpose is to explore the interface between pure Rust code and vcruntime.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 11, 2022

Panics are implemented in terms of SEH.

Yes, but those are terminal and a production application won't have them. I don't really count them as meaningful in the context. Because spectre attacks are statistical, attacker has to collect data over multitude of subroutine invocations in the same execution context... Besides, spectre attacks are about application data, while exception handler would be concentrating on data meaningful to stack unwinding. So that even if executed speculatively it won't access the application data, or more importantly use it as index...

@ChrisDenton
Copy link
Member

Panics aren't necessarily terminal (e.g. catch_unwind) and they are in all "normal" (not no_std) Rust applications even if the code paths aren't often taken. But this feels like we're getting into the weeds.

I'm still unsure as to what your point is. Microsoft have spectre mitigated versions of its C/C++ libs. Users may want to link these libraries into their application. The linker (link.exe) has no way to indicated this other than to pass in the path to spectre mitigated libraries. You can either do this by going through the C/C++ compiler (which rustc doesn't) or by setting the library paths (which rustc does via cc-rs which in turn defers to the environment if set).

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 11, 2022

I'm still unsure as to what your point is. Microsoft have spectre mitigated versions of its C/C++ libs. Users may want to link these libraries into their application.

  1. I [for one] [still] see no real benefit in linking pure Rust applications with the said libraries. Facilitating specifically this case would be kind of a punch in the air. It only makes sense if you actually compile some C/C++ that would introduce a meaningful dependency on vcruntime, meaningful in the context of spectre mitigations.
  2. It's beneficial to allow a library crate provider to make the assessment whether or not C/C++ library code needs to be compiled with /Qspectre and consequently linked with spectre-mitigated run-time. MSVC: Add support for linking against the "spectre-mitigated" CRT #673 would require the library crate provider to involve the potential user of the library in this last part of the process and rely on the said user's discipline to take specific steps in the future determined by Rust release schedule. This suggestion on the other hand allows the library provider to take the said decision without involving the potential user and achieve the objective now, with current or even previous Rust versions, let alone future ones.

@ChrisDenton
Copy link
Member

I think, fundamentally, we should be trusting the user to make that determination and not overriding or ignoring their preferences unless there is a fundamental conflict. It's not the job of cc-rs to tell users they're wrong.

In the absence of information it makes sense to choose sensible defaults, or if there is conflicting sources of information it can make sense to see one as being more authoritative. E.g. if the explicitly given target triple differs from the environment, then it makes sense to trust the library user knows what they want.

Again, I have no real problem with this PR. However, I don't think it's sufficient on its own. It's a compliment to other sources of information.

@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 25, 2022

I think, fundamentally, we should be trusting the user to make that determination and not overriding or ignoring their preferences unless there is a fundamental conflict. It's not the job of cc-rs to tell users they're wrong.

Could you clarify? Is it a suggestion that the addition of /Qspectre option is insufficient to make the determination? Or that module that is compiled with /Qspectre has no business [whatsoever?] affecting how the final application is linked? Or is it a response to "issue a warning in case spectre-mitigated libraries are not [even] installed" [which would be telling users they're wrong]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Windows targets and toolchains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants