Fix verifier shift instruction overflows imm value

[disconnect3d]

Before this commit, the verifier didn't properly check against shift overflows in instructions that operate on 32-bit registers. Those instructions are: lsh32_imm, rsh32_imm and arsh32_imm.

For those instructions, the verifier used the same check for shift overflow as for instructions that operate on 64-bit registers: it called the check_imm_shift(&insn, insn_ptr) function, which was implemented in the following way:

/// Check that the imm is a valid shift operand
fn check_imm_shift(insn: &ebpf::Insn, insn_ptr: usize) -> Result<(), VerifierError> {
    if insn.imm < 0 || insn.imm as u64 >= 64 {
        return Err(VerifierError::ShiftWithOverflow(adj_insn_ptr(insn_ptr)));
    }
    Ok(())
}

Note that here, we ensure the argument is not below 0 (insn.imm < 0) and that it is not greater or equal than 64 (insn.imm as u64 >= 64).

This check is incorrect for 32-bit versions of shift instructions which won't be marked with shift overflow for arguments in range [32, 63].

This can be problematic as shifting by more bits than the register size can lead to UB.

Please note that this bug can't be triggered with current Solana examples (https://github.com/solana-labs/example-helloworld/tree/52b0f1afb90ccb1af28c1c25030366e27a1e9834). This is because:

1. The Rust compiler won't allow for such shift overflow
2. The C compiler warns about it and the C example is compiled with the -Werror flag which makes the overflowing shift an error

However, this of course doesn't prevent the bytecode itself from having such invalid and overflowing shift value and so this commit fixes it making the verifier to check against those cases.

[Lichtso]

Good work!

Will have to make a config option (like this here), feature gate it and also check if none of the already deployed programs fail it.

[msuiche]

@Lichtso in which scenario would you have verify_shift32_imm set to false though?

[Lichtso]

@msuiche It is just a feature flag to coordinate the switch from one version to the next across the distributed network. So in the future it will always be true and can then be removed.

[msuiche]

Ah got it. Thanks for the explanation :) I didn't realize it had to be coordinated, I thought it was just for internal testing purposes.