diff mbox series

[v19,15/20] fsverity: expose verified fsverity built-in signatures to LSMs

Message ID 1716583609-21790-16-git-send-email-wufan@linux.microsoft.com (mailing list archive)
State New, archived
Headers show
Series Integrity Policy Enforcement LSM (IPE) | expand

Commit Message

Fan Wu May 24, 2024, 8:46 p.m. UTC
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(-)

Comments

Paul Moore May 30, 2024, 1:44 a.m. UTC | #1
On May 24, 2024 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(-)

...

> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index 90c07573dd77..a4ed91c7049f 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -106,6 +111,17 @@ int fsverity_verify_signature(const struct fsverity_info *vi,
>  		return err;
>  	}
>  
> +	err = security_inode_setintegrity(inode,
> +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
> +					  signature,
> +					  le32_to_cpu(sig_size));

I like this much better without the explicit inode cast :)

> +	if (err) {
> +		fsverity_err(inode, "Error %d exposing file signature to LSMs",
> +			     err);
> +		return err;
> +	}
> +
>  	return 0;
>  }

--
paul-moore.com
Paul Moore May 30, 2024, 1:46 a.m. UTC | #2
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.
Eric Biggers May 30, 2024, 3:06 a.m. UTC | #3
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
Fan Wu May 30, 2024, 3:38 a.m. UTC | #4
On 5/29/2024 8:06 PM, Eric Biggers wrote:
> 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.
> 
Sorry for the mistake. As sig_size is already converted in open.c, there 
is indeed no need to call this function again. I will remove this 
unnecessary conversion.

> 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

As for the commit message, my intention was to provide an example of a 
policy that with the patch IPE can enforce, not to claim that this 
specific restriction requires the patch. However, I will remove it as it 
seems to be causing confusion.
-Fan
Jarkko Sakkinen May 30, 2024, 5:51 a.m. UTC | #5
On Thu May 30, 2024 at 4:44 AM EEST, Paul Moore wrote:
> > +	err = security_inode_setintegrity(inode,
> > +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
> > +					  signature,
> > +					  le32_to_cpu(sig_size));
>
> I like this much better without the explicit inode cast :)

Would be nice btw if that was 'ret' or 'rc' because err is such
a common name for exception handler alike goto-labels... Looks
confusing just because of that :-)

BR, Jarkko
Eric Biggers May 30, 2024, 6:01 a.m. UTC | #6
On Thu, May 30, 2024 at 08:51:21AM +0300, Jarkko Sakkinen wrote:
> On Thu May 30, 2024 at 4:44 AM EEST, Paul Moore wrote:
> > > +	err = security_inode_setintegrity(inode,
> > > +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
> > > +					  signature,
> > > +					  le32_to_cpu(sig_size));
> >
> > I like this much better without the explicit inode cast :)
> 
> Would be nice btw if that was 'ret' or 'rc' because err is such
> a common name for exception handler alike goto-labels... Looks
> confusing just because of that :-)
> 

A lot of kernel code, including the rest of fs/verity/, uses the convention that
"0 or negative errno" return values are named 'err' (and return values that
aren't necessarily an errno are named something else).  So it's fine as-is.

- Eric
Jarkko Sakkinen May 30, 2024, 6:07 a.m. UTC | #7
On Thu May 30, 2024 at 9:01 AM EEST, Eric Biggers wrote:
> On Thu, May 30, 2024 at 08:51:21AM +0300, Jarkko Sakkinen wrote:
> > On Thu May 30, 2024 at 4:44 AM EEST, Paul Moore wrote:
> > > > +	err = security_inode_setintegrity(inode,
> > > > +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
> > > > +					  signature,
> > > > +					  le32_to_cpu(sig_size));
> > >
> > > I like this much better without the explicit inode cast :)
> > 
> > Would be nice btw if that was 'ret' or 'rc' because err is such
> > a common name for exception handler alike goto-labels... Looks
> > confusing just because of that :-)
> > 
>
> A lot of kernel code, including the rest of fs/verity/, uses the convention that
> "0 or negative errno" return values are named 'err' (and return values that
> aren't necessarily an errno are named something else).  So it's fine as-is.

Right, just hadn't seen such naming convention before.

BR, Jarkko
Paul Moore May 30, 2024, 8:54 p.m. UTC | #8
On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@kernel.org> wrote:
> 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.

...

> > 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...):

I too have been reviewing this patchset across multiple years and have
worked with Fan to fix locking issues, parsing issues, the initramfs
approach, etc.  My interest in getting this landed is simply a
combination of fulfilling my role as LSM maintainer as well as being
Fan's coworker.  While I realize you don't work with Fan, you are
listed as the fs-verity maintainer and as such I've been looking to
you to help review and authorize the fs-verity related code.  If you
are too busy, frustrated, or <fill in the blank> to continue reviewing
this patchset it would be helpful if you could identify an authorized
fs-verity reviewer.  I don't see any besides you and Ted listed in the
MAINTAINERS file, but perhaps the fs-verity entry is dated.

Regardless, I appreciate your time and feedback thus far and I'm sure
Fan does as well.
Eric Biggers May 31, 2024, 12:43 a.m. UTC | #9
On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > 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.
> 
> ...
> 
> > > 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...):
> 
> I too have been reviewing this patchset across multiple years and have
> worked with Fan to fix locking issues, parsing issues, the initramfs
> approach, etc.  

Sure, but none of the patches actually have your Reviewed-by.

> My interest in getting this landed is simply a
> combination of fulfilling my role as LSM maintainer as well as being
> Fan's coworker.  While I realize you don't work with Fan, you are
> listed as the fs-verity maintainer and as such I've been looking to
> you to help review and authorize the fs-verity related code.  If you
> are too busy, frustrated, or <fill in the blank> to continue reviewing
> this patchset it would be helpful if you could identify an authorized
> fs-verity reviewer.  I don't see any besides you and Ted listed in the
> MAINTAINERS file, but perhaps the fs-verity entry is dated.
> 
> Regardless, I appreciate your time and feedback thus far and I'm sure
> Fan does as well.

Maintainers are expected to do reviews and acks, but not to the extent of
extensive hand-holding of a half-baked submission.

- Eric
Paul Moore May 31, 2024, 3:51 p.m. UTC | #10
On Thu, May 30, 2024 at 8:43 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> > On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > 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.
> >
> > ...
> >
> > > > 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...):
> >
> > I too have been reviewing this patchset across multiple years and have
> > worked with Fan to fix locking issues, parsing issues, the initramfs
> > approach, etc.
>
> Sure, but none of the patches actually have your Reviewed-by.

As a general rule I don't post Acked-by/Reviewed-by tags for patches
that are targeting a subsystem that I maintain.  The logic being that
I'm going to be adding my Signed-off-by tag to the patches and arguing
these in front of Linus, so adding a Acked-by/Reviewed-by simply
creates more work later on where I have to strip them off and replace
them with my sign-off.

If the lack of a Reviewed-by tag is *really* what is preventing you
from reviewing the fs-verity patch, I can post that starting with the
next revision, but I'm guessing the lack of my tag isn't your core
issue (or at least I would argue it shouldn't be).

> > My interest in getting this landed is simply a
> > combination of fulfilling my role as LSM maintainer as well as being
> > Fan's coworker.  While I realize you don't work with Fan, you are
> > listed as the fs-verity maintainer and as such I've been looking to
> > you to help review and authorize the fs-verity related code.  If you
> > are too busy, frustrated, or <fill in the blank> to continue reviewing
> > this patchset it would be helpful if you could identify an authorized
> > fs-verity reviewer.  I don't see any besides you and Ted listed in the
> > MAINTAINERS file, but perhaps the fs-verity entry is dated.
> >
> > Regardless, I appreciate your time and feedback thus far and I'm sure
> > Fan does as well.
>
> Maintainers are expected to do reviews and acks, but not to the extent of
> extensive hand-holding of a half-baked submission.

Considering the current state of this patchset I don't believe that
verdict to be fair, or very considerate.

We clearly have different styles and approaches towards subsystem
maintainer roles.  I've had the good fortune to work with both hostile
and helpful senior developers during the early years of my time
working in the Linux kernel, and it helped reinforce the impact
patience and mentoring can have on contributors who are new to the
Linux kernel or perhaps system programming in general.  While I'm far
from perfect in this regard, I do hope and recommend that all of us in
maintainer, or senior developer, roles remember to exercise some
additional patience and education when working with new contributors.
Eric Biggers May 31, 2024, 5:47 p.m. UTC | #11
On Fri, May 31, 2024 at 11:51:47AM -0400, Paul Moore wrote:
> On Thu, May 30, 2024 at 8:43 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> > > On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > 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.
> > >
> > > ...
> > >
> > > > > 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...):
> > >
> > > I too have been reviewing this patchset across multiple years and have
> > > worked with Fan to fix locking issues, parsing issues, the initramfs
> > > approach, etc.
> >
> > Sure, but none of the patches actually have your Reviewed-by.
> 
> As a general rule I don't post Acked-by/Reviewed-by tags for patches
> that are targeting a subsystem that I maintain.  The logic being that
> I'm going to be adding my Signed-off-by tag to the patches and arguing
> these in front of Linus, so adding a Acked-by/Reviewed-by simply
> creates more work later on where I have to strip them off and replace
> them with my sign-off.
> 
> If the lack of a Reviewed-by tag is *really* what is preventing you
> from reviewing the fs-verity patch, I can post that starting with the
> next revision, but I'm guessing the lack of my tag isn't your core
> issue (or at least I would argue it shouldn't be).
>
> > > My interest in getting this landed is simply a
> > > combination of fulfilling my role as LSM maintainer as well as being
> > > Fan's coworker.  While I realize you don't work with Fan, you are
> > > listed as the fs-verity maintainer and as such I've been looking to
> > > you to help review and authorize the fs-verity related code.  If you
> > > are too busy, frustrated, or <fill in the blank> to continue reviewing
> > > this patchset it would be helpful if you could identify an authorized
> > > fs-verity reviewer.  I don't see any besides you and Ted listed in the
> > > MAINTAINERS file, but perhaps the fs-verity entry is dated.
> > >
> > > Regardless, I appreciate your time and feedback thus far and I'm sure
> > > Fan does as well.
> >
> > Maintainers are expected to do reviews and acks, but not to the extent of
> > extensive hand-holding of a half-baked submission.
> 
> Considering the current state of this patchset I don't believe that
> verdict to be fair, or very considerate.
> 
> We clearly have different styles and approaches towards subsystem
> maintainer roles.  I've had the good fortune to work with both hostile
> and helpful senior developers during the early years of my time
> working in the Linux kernel, and it helped reinforce the impact
> patience and mentoring can have on contributors who are new to the
> Linux kernel or perhaps system programming in general.  While I'm far
> from perfect in this regard, I do hope and recommend that all of us in
> maintainer, or senior developer, roles remember to exercise some
> additional patience and education when working with new contributors.
> 

It's not clear to me that you've done a close review of the verity related
patches, including not just this one but the dm-verity related ones and the
fsverity and dm-verity support in IPE itself, given the issues that I've been
finding in them in the last couple months.  As I said before, I'm not too
enthusiastic about IPE myself, for various reasons I've explained, so I've
really been looking to the people who actually want it to help drive it forward.

Anyway, as I also said, the fsverity and dm-verity support does seem to be
improved now after all the rounds of feedback, and I think it's close to the
finish line.  I just hope you can understand that I'm also a bit burnt out now,
and getting asked for an ack on this patch again and then seeing a bug in it
(despite it having been simplified to only a few lines now) and also still
misleading information in the commit message that I asked to be fixed before, is
a bit frustrating.  I think it's reasonable to expect a bit better, especially
for a security oriented feature.

Thanks,

- Eric
Paul Moore June 3, 2024, 1:40 a.m. UTC | #12
On Fri, May 31, 2024 at 1:47 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Fri, May 31, 2024 at 11:51:47AM -0400, Paul Moore wrote:
> > On Thu, May 30, 2024 at 8:43 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> > > > On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > 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.
> > > >
> > > > ...
> > > >
> > > > > > 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...):
> > > >
> > > > I too have been reviewing this patchset across multiple years and have
> > > > worked with Fan to fix locking issues, parsing issues, the initramfs
> > > > approach, etc.
> > >
> > > Sure, but none of the patches actually have your Reviewed-by.
> >
> > As a general rule I don't post Acked-by/Reviewed-by tags for patches
> > that are targeting a subsystem that I maintain.  The logic being that
> > I'm going to be adding my Signed-off-by tag to the patches and arguing
> > these in front of Linus, so adding a Acked-by/Reviewed-by simply
> > creates more work later on where I have to strip them off and replace
> > them with my sign-off.
> >
> > If the lack of a Reviewed-by tag is *really* what is preventing you
> > from reviewing the fs-verity patch, I can post that starting with the
> > next revision, but I'm guessing the lack of my tag isn't your core
> > issue (or at least I would argue it shouldn't be).
> >
> > > > My interest in getting this landed is simply a
> > > > combination of fulfilling my role as LSM maintainer as well as being
> > > > Fan's coworker.  While I realize you don't work with Fan, you are
> > > > listed as the fs-verity maintainer and as such I've been looking to
> > > > you to help review and authorize the fs-verity related code.  If you
> > > > are too busy, frustrated, or <fill in the blank> to continue reviewing
> > > > this patchset it would be helpful if you could identify an authorized
> > > > fs-verity reviewer.  I don't see any besides you and Ted listed in the
> > > > MAINTAINERS file, but perhaps the fs-verity entry is dated.
> > > >
> > > > Regardless, I appreciate your time and feedback thus far and I'm sure
> > > > Fan does as well.
> > >
> > > Maintainers are expected to do reviews and acks, but not to the extent of
> > > extensive hand-holding of a half-baked submission.
> >
> > Considering the current state of this patchset I don't believe that
> > verdict to be fair, or very considerate.
> >
> > We clearly have different styles and approaches towards subsystem
> > maintainer roles.  I've had the good fortune to work with both hostile
> > and helpful senior developers during the early years of my time
> > working in the Linux kernel, and it helped reinforce the impact
> > patience and mentoring can have on contributors who are new to the
> > Linux kernel or perhaps system programming in general.  While I'm far
> > from perfect in this regard, I do hope and recommend that all of us in
> > maintainer, or senior developer, roles remember to exercise some
> > additional patience and education when working with new contributors.
> >
>
> It's not clear to me that you've done a close review of the verity related
> patches, including not just this one but the dm-verity related ones and the
> fsverity and dm-verity support in IPE itself, given the issues that I've been
> finding in them in the last couple months.

I have not been able to give the fs-verify or dm-verity patches the
same level of scrutiny as the associated subsystem maintainers simply
because I lack the deep history and background; I rely on the
associated maintainers to catch the important "gotchas" as we've seen
in the patchset.

> As I said before, I'm not too
> enthusiastic about IPE myself, for various reasons I've explained, so I've
> really been looking to the people who actually want it to help drive it forward.

Once again, that is what I have been doing in my effort to get this to
a point where it can be merged and sent to Linus.  I've spent numerous
hours reviewing patches on-list (and catching quite a few issues), and
working with Fan off-list, to ensure these patches continue to
improve.  I'm asking you to review the fs-verity patch(es) for two
main reasons: 1) to identify any fs-verity interaction problems, 2) as
a courtesy since you are the fs-verity maintainer and I want you to be
aware of this and accepting of the code being introduced in the
subsystem you are responsible for maintaining.

> Anyway, as I also said, the fsverity and dm-verity support does seem to be
> improved now after all the rounds of feedback, and I think it's close to the
> finish line.

I agree.  I appreciate your help in reviewing this patchset, and those
that came before it.  I've seen Fan voice his appreciation too
on-list.

>  I just hope you can understand that I'm also a bit burnt out now,

I can understand that, and I'm sympathetic.  I've been doing this long
enough to have gone through my own cycles of burnout and rejuvenation
and I know how disheartening it can be at times.

> and getting asked for an ack on this patch again and then seeing a bug in it
> (despite it having been simplified to only a few lines now) and also still
> misleading information in the commit message that I asked to be fixed before, is
> a bit frustrating.  I think it's reasonable to expect a bit better, especially
> for a security oriented feature.

I firmly believe that no one writes perfect code, and no one performs
a perfect review.  As far as I'm concerned, the important bit is that
you respond, learn, and strive to do better next time.
diff mbox series

Patch

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 13e4b18e5dbb..362b7a5dc300 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -86,6 +86,14 @@  authenticating fs-verity file hashes include:
   signature in their "security.ima" extended attribute, as controlled
   by the IMA policy.  For more information, see the IMA documentation.
 
+- Integrity Policy Enforcement (IPE).  IPE supports enforcing access
+  control decisions based on immutable security properties of files,
+  including those protected by fs-verity's built-in signatures.
+  "IPE policy" specifically allows for the authorization of fs-verity
+  files using properties ``fsverity_digest`` for identifying
+  files by their verity digest, and ``fsverity_signature`` to authorize
+  files with a verified fs-verity's built-in signature.
+
 - Trusted userspace code in combination with `Built-in signature
   verification`_.  This approach should be used only with great care.
 
@@ -457,7 +465,11 @@  Enabling this option adds the following:
    On success, the ioctl persists the signature alongside the Merkle
    tree.  Then, any time the file is opened, the kernel verifies the
    file's actual digest against this signature, using the certificates
-   in the ".fs-verity" keyring.
+   in the ".fs-verity" keyring. This verification happens as long as the
+   file's signature exists, regardless of the state of the sysctl variable
+   "fs.verity.require_signatures" described in the next item. The IPE LSM
+   relies on this behavior to recognize and label fsverity files
+   that contain a verified built-in fsverity signature.
 
 3. A new sysctl "fs.verity.require_signatures" is made available.
    When set to 1, the kernel requires that all verity files have a
@@ -481,7 +493,7 @@  be carefully considered before using them:
 
 - Builtin signature verification does *not* make the kernel enforce
   that any files actually have fs-verity enabled.  Thus, it is not a
-  complete authentication policy.  Currently, if it is used, the only
+  complete authentication policy.  Currently, if it is used, one
   way to complete the authentication policy is for trusted userspace
   code to explicitly check whether files have fs-verity enabled with a
   signature before they are accessed.  (With
@@ -490,6 +502,13 @@  be carefully considered before using them:
   could just store the signature alongside the file and verify it
   itself using a cryptographic library, instead of using this feature.
 
+- Another approach is to utilize fs-verity builtin signature
+  verification in conjunction with the IPE LSM, which supports defining
+  a kernel-enforced, system-wide authentication policy that allows only
+  files with a verified fs-verity builtin signature to perform certain
+  operations, such as execution. Note that IPE doesn't require
+  fs.verity.require_signatures=1.
+
 - A file's builtin signature can only be set at the same time that
   fs-verity is being enabled on the file.  Changing or deleting the
   builtin signature later requires re-creating the file.
diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index 90c07573dd77..a4ed91c7049f 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -17,6 +17,7 @@ 
 
 #include <linux/cred.h>
 #include <linux/key.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/verification.h>
 
@@ -41,7 +42,11 @@  static struct key *fsverity_keyring;
  * @sig_size: size of signature in bytes, or 0 if no signature
  *
  * If the file includes a signature of its fs-verity file digest, verify it
- * against the certificates in the fs-verity keyring.
+ * against the certificates in the fs-verity keyring. Note that signatures
+ * are verified regardless of the state of the 'fsverity_require_signatures'
+ * variable and the LSM subsystem relies on this behavior to help enforce
+ * file integrity policies. Please discuss changes with the LSM list
+ * (thank you!).
  *
  * Return: 0 on success (signature valid or not required); -errno on failure
  */
@@ -106,6 +111,17 @@  int fsverity_verify_signature(const struct fsverity_info *vi,
 		return err;
 	}
 
+	err = security_inode_setintegrity(inode,
+					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
+					  signature,
+					  le32_to_cpu(sig_size));
+
+	if (err) {
+		fsverity_err(inode, "Error %d exposing file signature to LSMs",
+			     err);
+		return err;
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/security.h b/include/linux/security.h
index ab489fb02f06..2e87e038bc79 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -92,6 +92,7 @@  struct dm_verity_digest {
 enum lsm_integrity_type {
 	LSM_INT_DMVERITY_SIG_VALID,
 	LSM_INT_DMVERITY_ROOTHASH,
+	LSM_INT_FSVERITY_BUILTINSIG_VALID,
 };
 
 /*