Received-SPF: pass (google.com: domain of linux-kernel+bounces-194904-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 29 May 2024 20:06:05 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Paul Moore <paul@paul-moore.com>
Cc: Fan Wu <wufan@linux.microsoft.com>, corbet@lwn.net, zohar@linux.ibm.com,
	jmorris@namei.org, serge@hallyn.com, tytso@mit.edu, axboe@kernel.dk,
	agk@redhat.com, snitzer@kernel.org, mpatocka@redhat.com,
	eparis@redhat.com, linux-doc@vger.kernel.org,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, fsverity@lists.linux.dev,
	linux-block@vger.kernel.org, dm-devel@lists.linux.dev,
	audit@vger.kernel.org, linux-kernel@vger.kernel.org,
	Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [PATCH v19 15/20] fsverity: expose verified fsverity built-in
 signatures to LSMs
Message-ID: <20240530030605.GA29189@sol.localdomain>
References: <1716583609-21790-1-git-send-email-wufan@linux.microsoft.com>
 <1716583609-21790-16-git-send-email-wufan@linux.microsoft.com>
 <CAHC9VhRsnGjZATBj7-evK6Gdryr54raTTKMYO_vup8AGXLwjQg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHC9VhRsnGjZATBj7-evK6Gdryr54raTTKMYO_vup8AGXLwjQg@mail.gmail.com>

On Wed, May 29, 2024 at 09:46:57PM -0400, Paul Moore wrote:
> On Fri, May 24, 2024 at 4:46 PM Fan Wu <wufan@linux.microsoft.com> wrote:
> >
> > This patch enhances fsverity's capabilities to support both integrity and
> > authenticity protection by introducing the exposure of built-in
> > signatures through a new LSM hook. This functionality allows LSMs,
> > e.g. IPE, to enforce policies based on the authenticity and integrity of
> > files, specifically focusing on built-in fsverity signatures. It enables
> > a policy enforcement layer within LSMs for fsverity, offering granular
> > control over the usage of authenticity claims. For instance, a policy
> > could be established to permit the execution of all files with verified
> > built-in fsverity signatures while restricting kernel module loading
> > from specified fsverity files via fsverity digests.
> >
> > The introduction of a security_inode_setintegrity() hook call within
> > fsverity's workflow ensures that the verified built-in signature of a file
> > is exposed to LSMs. This enables LSMs to recognize and label fsverity files
> > that contain a verified built-in fsverity signature. This hook is invoked
> > subsequent to the fsverity_verify_signature() process, guaranteeing the
> > signature's verification against fsverity's keyring. This mechanism is
> > crucial for maintaining system security, as it operates in kernel space,
> > effectively thwarting attempts by malicious binaries to bypass user space
> > stack interactions.
> >
> > The second to last commit in this patch set will add a link to the IPE
> > documentation in fsverity.rst.
> >
> > Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> > Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> >
> > ---
> > v1-v6:
> >   + Not present
> >
> > v7:
> >   Introduced
> >
> > v8:
> >   + Split fs/verity/ changes and security/ changes into separate patches
> >   + Change signature of fsverity_create_info to accept non-const inode
> >   + Change signature of fsverity_verify_signature to accept non-const inode
> >   + Don't cast-away const from inode.
> >   + Digest functionality dropped in favor of:
> >     ("fs-verity: define a function to return the integrity protected
> >       file digest")
> >   + Reworded commit description and title to match changes.
> >   + Fix a bug wherein no LSM implements the particular fsverity @name
> >     (or LSM is disabled), and returns -EOPNOTSUPP, causing errors.
> >
> > v9:
> >   + No changes
> >
> > v10:
> >   + Rename the signature blob key
> >   + Cleanup redundant code
> >   + Make the hook call depends on CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> >
> > v11:
> >   + No changes
> >
> > v12:
> >   + Add constification to the hook call
> >
> > v13:
> >   + No changes
> >
> > v14:
> >   + Add doc/comment to built-in signature verification
> >
> > v15:
> >   + Add more docs related to IPE
> >   + Switch the hook call to security_inode_setintegrity()
> >
> > v16:
> >   + Explicitly mention "fsverity builtin signatures" in the commit
> >     message
> >   + Amend documentation in fsverity.rst
> >   + Fix format issue
> >   + Change enum name
> >
> > v17:
> >   + Fix various documentation issues
> >   + Use new enum name LSM_INT_FSVERITY_BUILTINSIG_VALID
> >
> > v18:
> >   + Fix typos
> >   + Move the inode_setintegrity hook call into fsverity_verify_signature()
> >
> > v19:
> >   + Cleanup code w.r.t inode_setintegrity hook refactoring
> > ---
> >  Documentation/filesystems/fsverity.rst | 23 +++++++++++++++++++++--
> >  fs/verity/signature.c                  | 18 +++++++++++++++++-
> >  include/linux/security.h               |  1 +
> >  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> Eric, can you give this patch in particular a look to make sure you
> are okay with everything?  I believe Fan has addressed all of your
> previous comments and it would be nice to have your Ack/Review tag if
> you are okay with the current revision.

Sorry, I've just gotten a bit tired of finding so many basic issues in this
patchset even after years of revisions.

This patch in particular is finally looking better.  There are a couple issues
that I still see.  (BTW, you're welcome to review it too to help find these
things, given that you seem to have an interest in getting this landed...):

> +	err = security_inode_setintegrity(inode,
> +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
> +					  signature,
> +					  le32_to_cpu(sig_size));

This is doing le32_to_cpu() on a variable of type size_t, which will do the
wrong thing on big endian systems and will generate a 'sparse' warning.

Also, the commit message still incorrectly claims that this patch allows
"restricting kernel module loading from specified fsverity files via fsverity
digests".  As I said before (sigh...), this is not correct as that can be done
without this patch.

- Eric