properly verify sbpf v3 executables in loader at deploy / upgrade

[soundsonacid]

problem

relocations are removed in sbpf v3. this means that extern "C" syscalls, which require relocation, are now invalid if used in sbpf v3.

proposed solution

agave: add Executable::verify::<LocalVerifier>(); after the Executable::verify::<RequisiteVerifier>(); call in solana_program_runtime::deploy::deploy_program.rs.
fd: add an equivalent to the LocalVerifier's CALL_IMM check in flamenco/vm/fd_vm.c:fd_vm_validate
sig: add an equivalent in v2/components/runtime/vm/executable.zig:Executable.verify
mithril: add an equivalent in pkg/sbpf/verifier.go:sbpf.(*Verifier).VerifyProgram

why this

the minimum platform tools version that builds valid sbpf v3 is v1.53. this version does not contain the fix in anza-xyz/cargo-build-sbf/67 to add -z defs to rustflags.

the fix in anza-xyz/cargo-build-sbf/14910 only applies to solana program deploy.

we cannot assume:

1. everybody uses solana program deploy (squads v4 does not)
2. even if 67 is backported to all platform tools > v1.53 & min advertised for sbpf v3 is raised to a version containing the fix, everybody will make sure to update to get the fix

even if we do assume these things, it should not be possible to deploy a broken binary to mainnet. this effectively proposes a fix for what is a currently broken program verification process.

the only way to conclusively fix this is to reject the deploy / upgrade instruction. this necessitates a loader change. therefore it necessitates a simd.

[deanmlittle]

It is, should be, always has been and always will be possible to deploy broken binaries to mainnet. This sort of thing is not what the verifier exists to catch, and also why the current verifier catches almost zero edge cases. This is just one of hundreds of ways you could deploy a broken binary. After the program cache is fixed, we will eventually opt to remove the verifier from the loader entirely to unblock future core BPF migration and shift all verification of the binary to the JIT, and failing that, at runtime. This is not a runtime issue, it is a tooling and devex issue that is best solved before a program buffer ever touches the chain, not after a user has already gone through their program upgrade ceremony only to hit a wall on one of thousands of arbitrary ways you can produce an invalid binary.

[soundsonacid]

i agree it is and has been possible to deploy a broken binary to mainnet. however, i believe the bug exhibited here is particularly insidious and may affect very many program devs at the 4.3/sbpfv3 boundary.

i would like to agree that it is a tooling & devex issue. if there were not currently 2 released versions of platform tools that allow the everyman program dev to produce these binaries, i would not believe this change is necessary. if there were not notable protocols (e.g. squads) using bespoke deployment pipelines, i would not believe this change was necessary.

after, for example, sbpf v4, we can remove this verifier entirely as we will be certain that all valid toolchain distributions >= minimum contain this fix. i am not suggesting that these additional verifications should be permanent changes.

it is not app dev failure if we knowingly allow bugs like this and then app devs hit this bug. it is a failure on our part to correctly prevent the bug from occurring.

[cavemanloverboy]

the decision for whether something like this should be done imo comes down to reachability via the tooling

should we have to patch a broken program vector that is reachable only when using a hacky compiler or linker written by some random dev (dean please restrain yourself)?

no.

should we patch one that is reachable via a version of the canonical platform tools that was circulating out for a while before it was noticed?

maybe. depends on the severity, on the amount of time it was out for, and on the potential consequence.

if, for example, phoenix or a router ran into this and broke prod for several hours on mainnet, the ecosystem would be down millions of dollars.

this is an unlikely outcome but it is not out of the realm of possibility and this fix does not need to be in protocol forever to address it.

i almost think it's better to backport deprecation of v3 deployment and put out an identical v4 with a fixed toolchain. then we have the guarantee of nobody deploying a broken program (with a bug that would be commonly reachable with the canonical tooling) without putting a verifier in the hot path.

happy frank, happy dean.

[soundsonacid]

i think this is a very reasonable solution. i am more than happy to author a simd that supersedes / nullifies simd-500, skips straight to v4, and bumps what is currently sbpf v4 to sbpf v5

[buffalojoec]

should we patch one that is reachable via a version of the canonical platform tools that was circulating out for a while before it was noticed?

As I mentioned in my comment below, platform-tools is ignorant to this particular issue. We catch this in cargo build-sbf with the -z defs rust flag, but it can also be caught in post processing by whatever tooling someone may be using.

i almost think it's better to backport deprecation of v3 deployment and put out an identical v4 with a fixed toolchain. then we have the guarantee of nobody deploying a broken program (with a bug that would be commonly reachable with the canonical tooling) without putting a verifier in the hot path.

We would only want to consider something like this if we really wanted to couple the runtime verifier - instead of the VM - to the bytecode version, wherein we would explicitly state call -1 is invalid bytecode under SBPFv4. It's already treated as an invalid symbol in the VM under SBPFv3, though.

[deanmlittle]

if, for example, phoenix or a router ran into this and broke prod for several hours on mainnet, the ecosystem would be down millions of dollars.

This bug will be triggered on any up to date testing platofrm with any test that touches any instruction that invokes a syscall, where it will subsequently hit either the recursion limit or the CU limit. If any app out there is not doing a single pre-flight test, the ecosystem will be out a lot more money than whatever hypothetical outage this is that will never happen in practice.

i almost think it's better to backport deprecation of v3 deployment and put out an identical v4 with a fixed toolchain.

More unnecessary edge cases to maintain forever is a bad idea. We already didn't need to version for any of the features in V3 in the first place. The Solana toolchain has always been extremely broken. This is not the first time, nor will it be the last. The runtime is the wrong place to clean up toolchain issues.

[buffalojoec]

the minimum platform tools version that builds valid sbpf v3 is v1.53. this version does not contain the fix in anza-xyz/cargo-build-sbf/67 to add -z defs to rustflags.

the fix in anza-xyz/cargo-build-sbf/14910 only applies to solana program deploy.

The -z defs flag is emitted from running cargo build-sbf, which is how majority of developers - including Anchor - build programs.

we cannot assume:

1. everybody uses solana program deploy (squads v4 does not)
2. even if 67 is backported to all platform tools > v1.53 & min advertised for sbpf v3 is raised to a version containing the fix, everybody will make sure to update to get the fix

As mentioned above, the flag in #67 is provided by cargo build-sbf. It's not a platform-tools feature, nor is it a PR to platform-tools. The minimum version of platform-tools dictates the binary's compatibility with SBPFv3, not which rust flags are used by the build command.

I think we can assume the vast majority of devs use cargo build-sbf, even Squads.

---

While I agree this particular issue can be nasty, I think catching it in development is trivial for program developers. Any testing library or framework that runs through SVM will throw the CallDepthExceeded error for any instruction that invokes an extern "C" syscall binding. You'd have to upgrade to SBPFv3 and test none of your instructions which use syscalls to step on this.

Furthermore, we shipped tooling that can catch it in your binary as well, with #67. This will make its way into the Solana CLI, but devs don't have to wait for that, they can just update cargo-build-sbf via cargo install. Custom tooling can also just pass -z defs.

We will also publish a full SBPFv3 migration guide - geared towards Rust and assembly devs alike - with footguns like this explicitly detailed before moving SIMD-0500 back into the feature queue.

All remedies thus far have been on the tooling side. The compiler itself still does not deem call -1 as invalid bytecode, which means if you build outside of cargo-build-sbf with any SBPFv3-compatible platform-tools version, you could see this issue. However, there are also a plethora of other issues you could face without the help of cargo-build-sbf, since you have to manage all of the build flags on your own (including -z defs).

---

On the runtime side, we've actually long discussed removing the RequisiteVerifier entirely, and pushing the onus for verifying bytecode onto the tooling.

Agave provides a Config to the VM that dictates what features the VM should support. Anyone who builds for the wrong bytecode version would fail immediately in the VM. Anyone who crafts an ELF that uses a supported e_flags (ie. 0x3) but uses relocations or invalid symbols would see the same class of errors this report describes.

The idea is that the runtime should treat deployed bytecode just like any other user-provided input - such as transactions - and employ only the minimum required to protect the network from sabotage, gracefully handling invalid inputs by way of CUs and fees. This would mean letting the Loader program write pretty much anything into a program and letting the runtime deal with it during transaction processing.

[soundsonacid]

discussed with @buffalojoec and determined this is not best way forward