From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [PATCH bpf] selftests/bpf: fix test_align Date: Mon, 8 Jan 2018 15:35:39 -0800 Message-ID: <4ee1531d-0c7b-8bf4-e656-5ab33f52738b@fb.com> References: <20180105230200.2183754-1-ast@fb.com> <14ffd15b-6f12-5d92-992a-66616af1a38f@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Daniel Borkmann , To: Edward Cree , "David S . Miller" Return-path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:44466 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756802AbeAHXgL (ORCPT ); Mon, 8 Jan 2018 18:36:11 -0500 In-Reply-To: <14ffd15b-6f12-5d92-992a-66616af1a38f@solarflare.com> Sender: netdev-owner@vger.kernel.org List-ID: On 1/8/18 8:38 AM, Edward Cree wrote: > On 05/01/18 23:02, Alexei Starovoitov wrote: >> since commit 82abbf8d2fc4 the verifier rejects the bit-wise >> arithmetic on pointers earlier. >> The test 'dubious pointer arithmetic' now has less output to match on. >> Adjust it. >> >> Fixes: 82abbf8d2fc4 ("bpf: do not allow root to mangle valid pointers") >> Reported-by: kernel test robot >> Signed-off-by: Alexei Starovoitov >> --- >> tools/testing/selftests/bpf/test_align.c | 22 +--------------------- >> 1 file changed, 1 insertion(+), 21 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/test_align.c b/tools/testing/selftests/bpf/test_align.c >> index 8591c89c0828..471bbbdb94db 100644 >> --- a/tools/testing/selftests/bpf/test_align.c >> +++ b/tools/testing/selftests/bpf/test_align.c >> @@ -474,27 +474,7 @@ static struct bpf_align_test tests[] = { >> .result = REJECT, >> .matches = { >> {4, "R5=pkt(id=0,off=0,r=0,imm=0)"}, >> - /* ptr & 0x40 == either 0 or 0x40 */ >> - {5, "R5=inv(id=0,umax_value=64,var_off=(0x0; 0x40))"}, >> - /* ptr << 2 == unknown, (4n) */ >> - {7, "R5=inv(id=0,smax_value=9223372036854775804,umax_value=18446744073709551612,var_off=(0x0; 0xfffffffffffffffc))"}, >> - /* (4n) + 14 == (4n+2). We blow our bounds, because >> - * the add could overflow. >> - */ >> - {8, "R5=inv(id=0,var_off=(0x2; 0xfffffffffffffffc))"}, >> - /* Checked s>=0 */ >> - {10, "R5=inv(id=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"}, >> - /* packet pointer + nonnegative (4n+2) */ >> - {12, "R6=pkt(id=1,off=0,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"}, >> - {14, "R4=pkt(id=1,off=4,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"}, >> - /* NET_IP_ALIGN + (4n+2) == (4n), alignment is fine. >> - * We checked the bounds, but it might have been able >> - * to overflow if the packet pointer started in the >> - * upper half of the address space. >> - * So we did not get a 'range' on R6, and the access >> - * attempt will fail. >> - */ >> - {16, "R6=pkt(id=1,off=0,r=0,umin_value=2,umax_value=9223372036854775806,var_off=(0x2; 0x7ffffffffffffffc))"}, >> + /* R5 bitwise operator &= on pointer prohibited */ >> } >> }, >> { > Rather than neutering this test, we should change it to keep the part where > it tests that a large pkt_ptr offset prevents us getting a reg->range. > Specifically, in this test we have > r2 = pkt > r5 = large unknown scalar > r6 = r2 + r5 > r4 = r6 + 4 > Then we check r4 < pkt_end, which normally would give r6->range = 4, but in > this case must not do so since r6 could be (u64)(-2) in which case r4 = 2 > < pkt_end despite r6 not pointing into the packet. > AFAICT there is not other coverage of this case in test_align, and I don't > recall such a test being in test_verifier either. So please instead replace > the insns that do prohibited ops on pointers with some other way of creating > a large unknown scalar, and keep the rest of the test case intact. makes sense. will send a follow up patch when security dust settles.