bpf: verifier improvement in 32bit shift sign extension pattern

This patch improves the verifier to correctly compute bounds for
sign extension compiler pattern composed of left shift by 32bits
followed by a sign right shift by 32bits.  Pattern in the verifier was
limitted to positive value bounds and would reset bound computation for
negative values.  New code allows both positive and negative values for
sign extension without compromising bound computation and verifier to
pass.

This change is required by GCC which generate such pattern, and was
detected in the context of systemd, as described in the following GCC
bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119731

Three new tests were added in verifier_subreg.c.

Signed-off-by: Cupertino Miranda  <cupertino.miranda@oracle.com>
Signed-off-by: Andrew Pinski  <andrew.pinski@oss.qualcomm.com>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Cc: David Faust  <david.faust@oracle.com>
Cc: Jose Marchesi  <jose.marchesi@oracle.com>
Cc: Elena Zannoni  <elena.zannoni@oracle.com>
Link: https://lore.kernel.org/r/20251202180220.11128-2-cupertino.miranda@oracle.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bb7eca1025c35ab5da5768b279918ee2395aa0de..a31c032b2dd6343ecd36313af32a667b80617604 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15300,21 +15300,17 @@ static void __scalar64_min_max_lsh(struct bpf_reg_state *dst_reg,
                                   u64 umin_val, u64 umax_val)
 {
        /* Special case <<32 because it is a common compiler pattern to sign
-        * extend subreg by doing <<32 s>>32. In this case if 32bit bounds are
-        * positive we know this shift will also be positive so we can track
-        * bounds correctly. Otherwise we lose all sign bit information except
-        * what we can pick up from var_off. Perhaps we can generalize this
-        * later to shifts of any length.
+        * extend subreg by doing <<32 s>>32. smin/smax assignments are correct
+        * because s32 bounds don't flip sign when shifting to the left by
+        * 32bits.
         */
-       if (umin_val == 32 && umax_val == 32 && dst_reg->s32_max_value >= 0)
+       if (umin_val == 32 && umax_val == 32) {
                dst_reg->smax_value = (s64)dst_reg->s32_max_value << 32;
-       else
-               dst_reg->smax_value = S64_MAX;
-
-       if (umin_val == 32 && umax_val == 32 && dst_reg->s32_min_value >= 0)
                dst_reg->smin_value = (s64)dst_reg->s32_min_value << 32;
-       else
+       } else {
+               dst_reg->smax_value = S64_MAX;
                dst_reg->smin_value = S64_MIN;
+       }
 
        /* If we might shift our top bit out, then we know nothing */
        if (dst_reg->umax_value > 1ULL << (63 - umax_val)) {