From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9913DC63798 for ; Tue, 24 Nov 2020 15:01:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 31818208C3 for ; Tue, 24 Nov 2020 15:01:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="mNl/Joee" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387680AbgKXPBY (ORCPT ); Tue, 24 Nov 2020 10:01:24 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731164AbgKXPBX (ORCPT ); Tue, 24 Nov 2020 10:01:23 -0500 Received: from mail-lj1-x241.google.com (mail-lj1-x241.google.com [IPv6:2a00:1450:4864:20::241]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 719CAC061A4D for ; Tue, 24 Nov 2020 07:01:21 -0800 (PST) Received: by mail-lj1-x241.google.com with SMTP id 142so22331343ljj.10 for ; Tue, 24 Nov 2020 07:01:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4cufC7N8nRv3SkCz2IJuYYqy8EP5LoCq+PiCIULlPW0=; b=mNl/Joee3mZwk30XNCTYG5kdWOulL4KcpSFJJnGh86vK4xb4jgc7VmmrSomvrjsLff ne/69Cv+zuk5lr94nZFUG8iyt/sqpOPgj1Nuj9NcXlC4eqX6IlxgB23FD2Ic4AJfB1bV QX0D9GA4sqn7jOUNma4wX9HoeEG2LHXYjKKe4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4cufC7N8nRv3SkCz2IJuYYqy8EP5LoCq+PiCIULlPW0=; b=F0VZ06LkD/VnuVolpHwlbVD2KriOQ9yFMrQ5pFgBz5bMy03L3WV6WltX69DLZ6qyyj fJci2wZbbh6qVkIoLEwx4F2FkLJGa592YIp2kSBKMWsWMwY/kv+EkVQWqnCICu5jU9BQ nLKSmxYz5ca5XQow5WpoB1rEGSIKL/Ozc4oFxxRQ14k5Of+DRYEfVZeqOBKv7O8BpUfD Z4bb9wlEKpvF0FXdt6Gp20H3/Bs/oyR1giTi3o4FgiQZlB8ayBCt8iPVhpY8K2oTE0Q7 QnE4g5C/yao9awaMag5GXTArZ2x6Zic5RPWgDfNP6N3dT1VYL5kdPtxeOKCSpyiu5c+K 7BWA== X-Gm-Message-State: AOAM531CmEUBR+kkXjweYGHPmVlRAgY7k5KItW2FIPPWvOEkcY1D+siX rJjggYZmvThp4qKKN26odDwSSukk0O4QoQiOY6wlww== X-Google-Smtp-Source: ABdhPJwIEoDcDkp7w38V6oWL+60rjnYt6TjmgnqxWK3R1hsqDpgobsUas8XwaZu6zdQQBEl+Zuzw9LtexHB9RzsHpjQ= X-Received: by 2002:a2e:85c6:: with SMTP id h6mr2139852ljj.110.1606230075181; Tue, 24 Nov 2020 07:01:15 -0800 (PST) MIME-Version: 1.0 References: <20201120131708.3237864-1-kpsingh@chromium.org> <20201120131708.3237864-2-kpsingh@chromium.org> <20201124040220.oyajc7wqn7gqgyib@ast-mbp> In-Reply-To: From: KP Singh Date: Tue, 24 Nov 2020 16:01:04 +0100 Message-ID: Subject: Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode To: Alexei Starovoitov Cc: James Morris , open list , bpf , Linux Security Module list , Alexei Starovoitov , Daniel Borkmann , Florent Revest , Brendan Jackman , Mimi Zohar Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 24, 2020 at 12:04 PM KP Singh wrote: > > On Tue, Nov 24, 2020 at 5:02 AM Alexei Starovoitov > wrote: > > > > On Fri, Nov 20, 2020 at 01:17:07PM +0000, KP Singh wrote: > > > + > > > +static bool bpf_ima_inode_hash_allowed(const struct bpf_prog *prog) > > > +{ > > > + return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id); > > > +} > > > + > > > +BTF_ID_LIST_SINGLE(bpf_ima_inode_hash_btf_ids, struct, inode) > > > + > > > +const static struct bpf_func_proto bpf_ima_inode_hash_proto = { > > > + .func = bpf_ima_inode_hash, > > > + .gpl_only = false, > > > + .ret_type = RET_INTEGER, > > > + .arg1_type = ARG_PTR_TO_BTF_ID, > > > + .arg1_btf_id = &bpf_ima_inode_hash_btf_ids[0], > > > + .arg2_type = ARG_PTR_TO_UNINIT_MEM, > > > + .arg3_type = ARG_CONST_SIZE_OR_ZERO, > > > + .allowed = bpf_ima_inode_hash_allowed, > > > +}; > > > + > > > static const struct bpf_func_proto * > > > bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > > { > > > @@ -97,6 +121,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > > return &bpf_task_storage_delete_proto; > > > case BPF_FUNC_bprm_opts_set: > > > return &bpf_bprm_opts_set_proto; > > > + case BPF_FUNC_ima_inode_hash: > > > + return &bpf_ima_inode_hash_proto; > > > > That's not enough for correctness. > > Not only hook has to sleepable, but the program has to be sleepable too. > > The patch 3 should be causing all sort of kernel warnings > > for calling mutex from preempt disabled. > > There it calls bpf_ima_inode_hash() from SEC("lsm/file_mprotect") program. Okay I dug into why I did not get any warnings, I do have CONFIG_DEBUG_ATOMIC_SLEEP and friends enabled and I do look at dmesg and... I think you misread the diff of my patch :) it's indeed attaching to "lsm.s/bprm_committed_creds": [https://lore.kernel.org/bpf/CACYkzJ7Oi8wXf=9a-e=fFHJirRbD=u47z+3+M2cRTCy_1fwtgw@mail.gmail.com/T/#m8d55bf0cdda614338cecd7154476497628612f6a] SEC("lsm/file_mprotect") int BPF_PROG(test_int_hook, struct vm_area_struct *vma, @@ -65,8 +67,11 @@ int BPF_PROG(test_void_hook, struct linux_binprm *bprm) __u32 key = 0; __u64 *value; - if (monitored_pid == pid) + if (monitored_pid == pid) { bprm_count++; + ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode, + &ima_hash, sizeof(ima_hash)); + } bpf_copy_from_user(args, sizeof(args), (void *)bprm->vma->vm_mm->arg_start); bpf_copy_from_user(args, sizeof(args), (void *)bprm->mm->arg_start); -- The diff makes it look like it is attaching to "lsm/file_mprotect" but it's actually attaching to "lsm.s/bprm_committed_creds". Now we can either check for prod->aux->sleepable in bpf_ima_inode_hash_allowed or just not expose the helper to non-sleepable hooks. I went with the latter as this is what we do for bpf_copy_from_user. - KP > > I did actually mean to use SEC("lsm.s/bprm_committed_creds"), my bad. > > > "lsm/" is non-sleepable. "lsm.s/" is. > > please enable CONFIG_DEBUG_ATOMIC_SLEEP=y in your config. > > Oops, yes I did notice that during recent work on the test cases. > > Since we need a stronger check than just warnings, I am doing > something similar to > what we do for bpf_copy_from_user i.e. > > return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL;