On 10/24/23 4:53 PM, Andrii Nakryiko wrote:
On Tue, Oct 24, 2023 at 6:08 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
On 10/22/23 10:57 PM, Andrii Nakryiko wrote:
Add smin/smax derivation from appropriate umin/umax values. Previously the
logic was surprisingly asymmetric, trying to derive umin/umax from smin/smax
(if possible), but not trying to do the same in the other direction. A simple
addition to __reg64_deduce_bounds() fixes this.
Do you have a concrete example case where bounds get further refined? Might be
useful to add this to the commit description or as comment in the code for future
reference to make this one here more obvious.
Yes, it's one of the crafted tests. I've been adding those issues
where I found bugs or discrepancies between kernel and selftest to the
"crafted list" to make sure all previously broken cases are covered.
Unfortunately I didn't keep a detailed log of cases (as there were
initially too many). I'll try to undo each of these changes and see
what breaks, will take a bit to do this one by one, but it's fine.
What level of details is necessary? Just having a test case? Showing
how the kernel adjusts stuff (I can get verbose debugging logs both
from kernel and selftest)? I'm trying to understand the desired
balance between too little and too much information (and save myself a
lot of time, if I can ;)
I think a concrete walk-through example before/after with reg state would
be nice and some more analysis on how the change relates to the subsequent
adjustments done in __reg64_deduce_bounds() further below where we learn
from signed and later again from unsigned bounds.
(Btw, patch 1 looks good/trivial so I applied that one in the meantime.)
Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
---
kernel/bpf/verifier.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f8fca3fbe20f..885dd4a2ff3a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2164,6 +2164,13 @@ static void __reg32_deduce_bounds(struct bpf_reg_state *reg)
static void __reg64_deduce_bounds(struct bpf_reg_state *reg)
{
+ /* u64 range forms a valid s64 range (due to matching sign bit),
+ * so try to learn from that
+ */
+ if ((s64)reg->umin_value <= (s64)reg->umax_value) {
+ reg->smin_value = max_t(s64, reg->smin_value, reg->umin_value);
+ reg->smax_value = min_t(s64, reg->smax_value, reg->umax_value);
+ }
/* Learn sign from signed bounds.
* If we cannot cross the sign boundary, then signed and unsigned bounds
* are the same, so combine. This works even in the negative case, e.g.
