From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Cree Subject: Re: [PATCH] bpf: verifier: BPF_MOV don't mark dst reg if src == dst Date: Mon, 30 Jul 2018 17:40:53 +0100 Message-ID: References: <7b0ce11d-ad54-9ba8-740c-58fd2675cc69@iogearbox.net> <64b39825-3f74-15f6-6212-057c96a84999@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Cc: Alexei Starovoitov , Alexei Starovoitov , Network Development To: Daniel Borkmann , Arthur Fabre Return-path: Received: from dispatch1-us1.ppe-hosted.com ([148.163.129.52]:58378 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726962AbeG3SQs (ORCPT ); Mon, 30 Jul 2018 14:16:48 -0400 In-Reply-To: <64b39825-3f74-15f6-6212-057c96a84999@iogearbox.net> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 30/07/18 13:03, Daniel Borkmann wrote: > On 07/30/2018 12:58 PM, Arthur Fabre wrote: >> The check_alu_op() call handles 32bit and 64bit cases, but then in the >> 32bit case >> mark_reg_unknown() is called, discarding all the dst register state. >> I think this is equivalent to keeping a copy of dst and always marking >> dst as unknown. >> >> I think we could actually always use check_reg_arg() with DST_OP_NO_MARK: >> >> In the 32bit case, we call mark_reg_unknown() anyways. >> >> In the 64bit case, we copy src to dst, so marking dst as unknown is pointless. >> >> For plain BPF, we call __mark_reg_known() anyways. > For imms this approach would be buggy since we leave a stale reg->off behind > which is uncleared from previous reg state. So for them the mark_reg_unknown() > is useful in the sense that it clears all reg state whereas __mark_reg_known() > might only initialize a subset of it. I guess the right thing is to change the existing call to DST_OP_NO_MARK,  and then in the "case: R = imm" branch add another check_reg_arg(DST_OP)  (or maybe just a mark_reg_unknown()). -Ed