Received-SPF: pass (google.com: domain of linux-kernel+bounces-108226-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240315181032.645161-1-cgzones@googlemail.com>
 <20240315181032.645161-2-cgzones@googlemail.com> <CAHC9VhRkawYWQN0UY2R68Qn4pRijpXgu97YOr6XPA7Ls0-zQcA@mail.gmail.com>
 <5368DC74-41CF-4450-AF6F-FFB51EFCCF99@kernel.org>
In-Reply-To: <5368DC74-41CF-4450-AF6F-FFB51EFCCF99@kernel.org>
From: Paul Moore <paul@paul-moore.com>
Date: Tue, 19 Mar 2024 19:10:02 -0400
Message-ID: <CAHC9VhTpTbVL4=tchR3Bpcfe0Hsijf5XJ6wsEvU7cu8eUy_iDA@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] lsm: introduce new hook security_vm_execstack
To: Kees Cook <kees@kernel.org>
Cc: =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>, 
	linux-security-module@vger.kernel.org, Eric Biederman <ebiederm@xmission.com>, 
	Kees Cook <keescook@chromium.org>, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>, James Morris <jmorris@namei.org>, 
	"Serge E. Hallyn" <serge@hallyn.com>, Khadija Kamran <kamrankhadijadj@gmail.com>, 
	Andrii Nakryiko <andrii@kernel.org>, Casey Schaufler <casey@schaufler-ca.com>, 
	Alexei Starovoitov <ast@kernel.org>, Ondrej Mosnacek <omosnace@redhat.com>, 
	Roberto Sassu <roberto.sassu@huawei.com>, Alfred Piccioni <alpic@google.com>, 
	John Johansen <john.johansen@canonical.com>, linux-mm@kvack.org, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 15, 2024 at 11:24=E2=80=AFPM Kees Cook <kees@kernel.org> wrote:
> On March 15, 2024 1:22:39 PM PDT, Paul Moore <paul@paul-moore.com> wrote:
> >On Fri, Mar 15, 2024 at 2:10=E2=80=AFPM Christian G=C3=B6ttsche
> ><cgzones@googlemail.com> wrote:
> >>
> >> Add a new hook guarding instantiations of programs with executable
> >> stack.  They are being warned about since commit 47a2ebb7f505 ("execve=
:
> >> warn if process starts with executable stack").  Lets give LSMs the
> >> ability to control their presence on a per application basis.
> >>
> >> Signed-off-by: Christian G=C3=B6ttsche <cgzones@googlemail.com>
> >> ---
> >>  fs/exec.c                     |  4 ++++
> >>  include/linux/lsm_hook_defs.h |  1 +
> >>  include/linux/security.h      |  6 ++++++
> >>  security/security.c           | 13 +++++++++++++
> >>  4 files changed, 24 insertions(+)
> >
> >Looking at the commit referenced above, I'm guessing the existing
> >security_file_mprotect() hook doesn't catch this?
> >
> >> diff --git a/fs/exec.c b/fs/exec.c
> >> index 8cdd5b2dd09c..e6f9e980c6b1 100644
> >> --- a/fs/exec.c
> >> +++ b/fs/exec.c
> >> @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
> >>         BUG_ON(prev !=3D vma);
> >>
> >>         if (unlikely(vm_flags & VM_EXEC)) {
> >> +               ret =3D security_vm_execstack();
> >> +               if (ret)
> >> +                       goto out_unlock;
> >> +
> >>                 pr_warn_once("process '%pD4' started with executable s=
tack\n",
> >>                              bprm->file);
> >>         }
> >
> >Instead of creating a new LSM hook, have you considered calling the
> >existing security_file_mprotect() hook?  The existing LSM controls
> >there may not be a great fit in this case, but I'd like to hear if
> >you've tried that, and if you have, what made you decide a new hook
> >was the better option?
>
> Also, can't MDWE handle this already?
> https://git.kernel.org/linus/b507808ebce23561d4ff8c2aa1fb949fe402bc61

It looks like it, but that doesn't mean there isn't also value in an
associated LSM hook as the LSM hook would admins and security policy
developers/analysts to incorporate this as part of the system's
security policy.  It's great that we have all of these cool knobs that
we can play with independent of each other, but sometimes you really
want a single unified security policy that you can look at, analyze,
and reason about.

Regardless, my previous comments still stand, I'd like to hear
verification that the existing security_file_mprotect() hook is not
sufficient, and if its current placement is lacking, why calling it
from a second location wasn't practical and required the creation of a
new LSM hook.

--=20
paul-moore.com