Eduard Zingerman eddyz87@gmail.com writes:
Could you please point me to a location, where exact error code returned by updated push_stack() matters? I checked push_stack() callgraph (in the attachment), but can't find anything.
Only with the final patch 11 ("bpf: Fall back to nospec for spec path verification") applied, the error code should matter. Then, the error code either matches `state->speculative && error_recoverable_with_nospec(err)` in do_check() if it was EINVAL (in this case we heuristically avoided nested speculative path verification but have to add a nospec), or we continue to raise the error (e.g., ENOMEM) from do_check().
Or is your question on this part from the commit message of patch 9?
This changes the sanitization-case to returning -ENOMEM. However, this is more fitting as -EFAULT would indicate a verifier-internal bug.
This was referring to the sanitize_speculative_path() calls in check_cond_jmp_op(). For that case, the error should also only be used in do_check() with patch 11 applied. However, regarding this, EFAULT and ENOMEM are treated the same (they both don't satisfy error_recoverable_with_nospec()), therefore this change is primarily made to not complicate the code.
I just became aware that there is some special handling of EFAULT as discussed in c7a897843224 ("bpf: don't leave partial mangled prog in jit_subprogs error path"). I will have look into this in detail to make sure changing push_stack() from EFAULT to ENOMEM is OK.
Hope this answers your question.
Adding some of these details to v2 won't hurt I guess.