Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 31 Jul 2018 17:28:35 -0700
From:   Roman Gushchin <guro@fb.com>
To:     Daniel Borkmann <daniel@iogearbox.net>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel-team@fb.com>, Alexei Starovoitov <ast@kernel.org>
Subject: Re: [PATCH v4 bpf-next 08/14] bpf: introduce the
 bpf_get_local_storage() helper function
Message-ID: <20180801002831.GA25953@castle.DHCP.thefacebook.com>
References: <20180727215243.3850-1-guro@fb.com>
 <20180727215243.3850-9-guro@fb.com>
 <ad6c58e3-08a8-aa9c-2d69-528007fe126e@iogearbox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ad6c58e3-08a8-aa9c-2d69-528007fe126e@iogearbox.net>
User-Agent: Mutt/1.9.2 (2017-12-15)
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2018 00:28:40.7366 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cb097fb-8ea6-4ef5-75d0-08d5f745bb83
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR15MB0167
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-07-31_09:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 01, 2018 at 12:50:16AM +0200, Daniel Borkmann wrote:
> On 07/27/2018 11:52 PM, Roman Gushchin wrote:
> [...]
> > @@ -2533,6 +2541,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> >  	}
> >  
> >  	regs = cur_regs(env);
> > +
> > +	/* check that flags argument in get_local_storage(map, flags) is 0,
> > +	 * this is required because get_local_storage() can't return an error.
> > +	 */
> > +	if (func_id == BPF_FUNC_get_local_storage &&
> > +	    !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) {
> > +		verbose(env, "get_local_storage() doesn't support non-zero flags\n");
> > +		return -EINVAL;
> > +	}
> 
> Hmm, this check is actually not correct. You will still be able to pass non-zero
> values in there. arg2_type from the helper is ARG_ANYTHING, so the register type
> could for example be one of the pointer types and it will still pass the verifier.
> The correct way to check would be to use register_is_null().
> 
> > +
> >  	/* reset caller saved regs */
> >  	for (i = 0; i < CALLER_SAVED_REGS; i++) {
> >  		mark_reg_not_init(env, regs, caller_saved[i]);

Oh, perfect catch!
The diff is below. Please, let me know if you prefer me to resend
the whole patch/patchset.

Thank you!

Roman

--

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0f69f827ff9f..4547e07ab3db 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2546,7 +2546,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
         * this is required because get_local_storage() can't return an error.
         */
        if (func_id == BPF_FUNC_get_local_storage &&
-           !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) {
+           !register_is_null(&regs[BPF_REG_2])) {
                verbose(env, "get_local_storage() doesn't support non-zero flags\n");
                return -EINVAL;
        }