Closed pcy190 closed 7 months ago
Will PR a patch recently :)
Thanks a lot for the follow-up! And apologies for the delay. This looks correct, and also consistent with the draft BPF ISA specification.
Note that in the kernel interpreter implementation, it masks the
SRC
/IMM
as well.
Not accurate - It does apply the mask for a SRC
register, but it rejects the program in the verifier if the operation uses an IMM
value that it judges incorrect. I think this is what you meant, given that you mentioned we don't do the check in the verifier.
Thanks a lot for the patch!
This issue follows #99 and #100. The previous PR demonstrates the implementation incompliance on the logic shift implementation. However, the arithmetic shift operation still meets this problem, and needs the additional mask offset as well.
The current implementation of ARSH64 is not compliant, and undefined behavior happens if we overflow the number of bits we have when trying to shift.
The following PoC program could trigger this inconsistency:
Note that in the kernel interpreter implementation, it masks the
SRC
/IMM
as well. https://github.com/torvalds/linux/blob/610a9b8f49fbcf1100716370d3b5f6f884a2835a/kernel/bpf/core.c#L1794-L1805Hence we need to mask the shift offset with 63 in the following code. Since we don't perform verifier check on the immediate value, we need to mask the
IMM
as well.https://github.com/qmonnet/rbpf/blob/7bebff57bec037cab5acd57ddbd72363fb163faf/src/interpreter.rs#L289-L290