[RFC,v11,15/19] fsverity: consume builtin signature via LSM hook

Commit Message

Fan Wu Oct. 4, 2023, 10:09 p.m. UTC

fsverity represents a mechanism to support both integrity and
authenticity protection of a file, supporting both signed and unsigned
digests.

An LSM which controls access to a resource based on authenticity and
integrity of said resource, can then use this data to make an informed
decision on the authorization (provided by the LSM's policy) of said
claim.

This effectively allows the extension of a policy enforcement layer in
LSM for fsverity, allowing for more granular control of how a
particular authenticity claim can be used. For example, "all (built-in)
signed fsverity files should be allowed to execute, but only these
hashes are allowed to be loaded as kernel modules".

This enforcement must be done in kernel space, as a userspace only
solution would fail a simple litmus test: Download a self-contained
malicious binary that never touches the userspace stack. This
binary would still be able to execute.

Signed-off-by: Deven Bowers <[e-mail]>
Signed-off-by: Fan Wu <[e-mail]>
---
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
---
 fs/verity/fsverity_private.h |  2 +-
 fs/verity/open.c             | 26 +++++++++++++++++++++++++-
 include/linux/fsverity.h     |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

Comments

Eric Biggers Oct. 5, 2023, 2:27 a.m. UTC | #1

On Wed, Oct 04, 2023 at 03:09:42PM -0700, Fan Wu wrote:
> +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> +static int fsverity_inode_setsecurity(struct inode *inode,
> +				      struct fsverity_descriptor *desc)
> +{
> +	return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> +					  desc->signature,
> +					  le32_to_cpu(desc->sig_size), 0);
> +}

Why isn't the type of the second argument 'const struct fsverity_descriptor *'?

- Eric

Fan Wu Oct. 5, 2023, 2:49 a.m. UTC | #2

On 10/4/2023 7:27 PM, Eric Biggers wrote:
> On Wed, Oct 04, 2023 at 03:09:42PM -0700, Fan Wu wrote:
>> +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
>> +static int fsverity_inode_setsecurity(struct inode *inode,
>> +				      struct fsverity_descriptor *desc)
>> +{
>> +	return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
>> +					  desc->signature,
>> +					  le32_to_cpu(desc->sig_size), 0);
>> +}
> 
> Why isn't the type of the second argument 'const struct fsverity_descriptor *'?
> 
> - Eric

Thanks for the suggestion. I agree that adding 'const' here is a better 
approach. I will update this in the next version.

-Fan

Paul Moore Oct. 24, 2023, 3:52 a.m. UTC | #3

On Oct  4, 2023 Fan Wu <[e-mail]> wrote:
> 
> fsverity represents a mechanism to support both integrity and
> authenticity protection of a file, supporting both signed and unsigned
> digests.
> 
> An LSM which controls access to a resource based on authenticity and
> integrity of said resource, can then use this data to make an informed
> decision on the authorization (provided by the LSM's policy) of said
> claim.
> 
> This effectively allows the extension of a policy enforcement layer in
> LSM for fsverity, allowing for more granular control of how a
> particular authenticity claim can be used. For example, "all (built-in)
> signed fsverity files should be allowed to execute, but only these
> hashes are allowed to be loaded as kernel modules".
> 
> This enforcement must be done in kernel space, as a userspace only
> solution would fail a simple litmus test: Download a self-contained
> malicious binary that never touches the userspace stack. This
> binary would still be able to execute.
> 
> Signed-off-by: Deven Bowers <[e-mail]>
> Signed-off-by: Fan Wu <[e-mail]>
> ---
> 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
> ---
>  fs/verity/fsverity_private.h |  2 +-
>  fs/verity/open.c             | 26 +++++++++++++++++++++++++-
>  include/linux/fsverity.h     |  2 ++
>  3 files changed, 28 insertions(+), 2 deletions(-)

We need an ACK from some VFS folks on this.

> diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> index d071a6e32581..4a82716e852f 100644
> --- a/fs/verity/fsverity_private.h
> +++ b/fs/verity/fsverity_private.h
> @@ -108,7 +108,7 @@ int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
>  				     unsigned int log_blocksize,
>  				     const u8 *salt, size_t salt_size);
>  
> -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> +struct fsverity_info *fsverity_create_info(struct inode *inode,
>  					   struct fsverity_descriptor *desc);
>  
>  void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index 6c31a871b84b..5b48e2c39086 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -8,6 +8,7 @@
>  #include "fsverity_private.h"
>  
>  #include <linux/mm.h>
> +#include <linux/security.h>
>  #include <linux/slab.h>
>  
>  static struct kmem_cache *fsverity_info_cachep;
> @@ -172,12 +173,28 @@ static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
>  	return err;
>  }
>  
> +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> +static int fsverity_inode_setsecurity(struct inode *inode,
> +				      struct fsverity_descriptor *desc)
> +{
> +	return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> +					  desc->signature,
> +					  le32_to_cpu(desc->sig_size), 0);
> +}
> +#else
> +static inline int fsverity_inode_setsecurity(struct inode *inode,
> +					     struct fsverity_descriptor *desc)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_IPE_PROP_FS_VERITY*/
> +
>  /*
>   * Create a new fsverity_info from the given fsverity_descriptor (with optional
>   * appended builtin signature), and check the signature if present.  The
>   * fsverity_descriptor must have already undergone basic validation.
>   */
> -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> +struct fsverity_info *fsverity_create_info(struct inode *inode,
>  					   struct fsverity_descriptor *desc)
>  {
>  	struct fsverity_info *vi;
> @@ -242,6 +259,13 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
>  		spin_lock_init(&vi->hash_page_init_lock);
>  	}
>  
> +	err = fsverity_inode_setsecurity(inode, desc);
> +	if (err == -EOPNOTSUPP)
> +		err = 0;
> +
> +	if (err)
> +		goto fail;
> +
>  	return vi;
>  
>  fail:
> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index 1eb7eae580be..9666721baf15 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -319,4 +319,6 @@ static inline int fsverity_prepare_setattr(struct dentry *dentry,
>  	return 0;
>  }
>  
> +#define FS_VERITY_INODE_SEC_NAME "fsverity.builtin-sig"
> +
>  #endif	/* _LINUX_FSVERITY_H */
> -- 
> 2.25.1

--
paul-moore.com

Paul Moore Nov. 2, 2023, 12:40 a.m. UTC | #4

On Mon, Oct 23, 2023 at 11:52 PM Paul Moore <[e-mail]> wrote:
> On Oct  4, 2023 Fan Wu <[e-mail]> wrote:
> >
> > fsverity represents a mechanism to support both integrity and
> > authenticity protection of a file, supporting both signed and unsigned
> > digests.
> >
> > An LSM which controls access to a resource based on authenticity and
> > integrity of said resource, can then use this data to make an informed
> > decision on the authorization (provided by the LSM's policy) of said
> > claim.
> >
> > This effectively allows the extension of a policy enforcement layer in
> > LSM for fsverity, allowing for more granular control of how a
> > particular authenticity claim can be used. For example, "all (built-in)
> > signed fsverity files should be allowed to execute, but only these
> > hashes are allowed to be loaded as kernel modules".
> >
> > This enforcement must be done in kernel space, as a userspace only
> > solution would fail a simple litmus test: Download a self-contained
> > malicious binary that never touches the userspace stack. This
> > binary would still be able to execute.
> >
> > Signed-off-by: Deven Bowers <[e-mail]>
> > Signed-off-by: Fan Wu <[e-mail]>
> > ---
> > 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
> > ---
> >  fs/verity/fsverity_private.h |  2 +-
> >  fs/verity/open.c             | 26 +++++++++++++++++++++++++-
> >  include/linux/fsverity.h     |  2 ++
> >  3 files changed, 28 insertions(+), 2 deletions(-)
>
> We need an ACK from some VFS folks on this.

Eric and/or Ted, can we get either an ACK or some feedback on this patch?

For reference, the full patchset can be found on lore at the link below:

https://lore.kernel.org/linux-security-module/[e-mail]/

> > diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
> > index d071a6e32581..4a82716e852f 100644
> > --- a/fs/verity/fsverity_private.h
> > +++ b/fs/verity/fsverity_private.h
> > @@ -108,7 +108,7 @@ int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
> >                                    unsigned int log_blocksize,
> >                                    const u8 *salt, size_t salt_size);
> >
> > -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> > +struct fsverity_info *fsverity_create_info(struct inode *inode,
> >                                          struct fsverity_descriptor *desc);
> >
> >  void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
> > diff --git a/fs/verity/open.c b/fs/verity/open.c
> > index 6c31a871b84b..5b48e2c39086 100644
> > --- a/fs/verity/open.c
> > +++ b/fs/verity/open.c
> > @@ -8,6 +8,7 @@
> >  #include "fsverity_private.h"
> >
> >  #include <linux/mm.h>
> > +#include <linux/security.h>
> >  #include <linux/slab.h>
> >
> >  static struct kmem_cache *fsverity_info_cachep;
> > @@ -172,12 +173,28 @@ static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
> >       return err;
> >  }
> >
> > +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > +static int fsverity_inode_setsecurity(struct inode *inode,
> > +                                   struct fsverity_descriptor *desc)
> > +{
> > +     return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> > +                                       desc->signature,
> > +                                       le32_to_cpu(desc->sig_size), 0);
> > +}
> > +#else
> > +static inline int fsverity_inode_setsecurity(struct inode *inode,
> > +                                          struct fsverity_descriptor *desc)
> > +{
> > +     return 0;
> > +}
> > +#endif /* CONFIG_IPE_PROP_FS_VERITY*/
> > +
> >  /*
> >   * Create a new fsverity_info from the given fsverity_descriptor (with optional
> >   * appended builtin signature), and check the signature if present.  The
> >   * fsverity_descriptor must have already undergone basic validation.
> >   */
> > -struct fsverity_info *fsverity_create_info(const struct inode *inode,
> > +struct fsverity_info *fsverity_create_info(struct inode *inode,
> >                                          struct fsverity_descriptor *desc)
> >  {
> >       struct fsverity_info *vi;
> > @@ -242,6 +259,13 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
> >               spin_lock_init(&vi->hash_page_init_lock);
> >       }
> >
> > +     err = fsverity_inode_setsecurity(inode, desc);
> > +     if (err == -EOPNOTSUPP)
> > +             err = 0;
> > +
> > +     if (err)
> > +             goto fail;
> > +
> >       return vi;
> >
> >  fail:
> > diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> > index 1eb7eae580be..9666721baf15 100644
> > --- a/include/linux/fsverity.h
> > +++ b/include/linux/fsverity.h
> > @@ -319,4 +319,6 @@ static inline int fsverity_prepare_setattr(struct dentry *dentry,
> >       return 0;
> >  }
> >
> > +#define FS_VERITY_INODE_SEC_NAME "fsverity.builtin-sig"
> > +
> >  #endif       /* _LINUX_FSVERITY_H */
> > --
> > 2.25.1

Eric Biggers Nov. 2, 2023, 2:53 a.m. UTC | #5

On Wed, Nov 01, 2023 at 08:40:06PM -0400, Paul Moore wrote:
> On Mon, Oct 23, 2023 at 11:52 PM Paul Moore <[e-mail]> wrote:
> > On Oct  4, 2023 Fan Wu <[e-mail]> wrote:
> > >
> > > fsverity represents a mechanism to support both integrity and
> > > authenticity protection of a file, supporting both signed and unsigned
> > > digests.
> > >
> > > An LSM which controls access to a resource based on authenticity and
> > > integrity of said resource, can then use this data to make an informed
> > > decision on the authorization (provided by the LSM's policy) of said
> > > claim.
> > >
> > > This effectively allows the extension of a policy enforcement layer in
> > > LSM for fsverity, allowing for more granular control of how a
> > > particular authenticity claim can be used. For example, "all (built-in)
> > > signed fsverity files should be allowed to execute, but only these
> > > hashes are allowed to be loaded as kernel modules".
> > >
> > > This enforcement must be done in kernel space, as a userspace only
> > > solution would fail a simple litmus test: Download a self-contained
> > > malicious binary that never touches the userspace stack. This
> > > binary would still be able to execute.
> > >
> > > Signed-off-by: Deven Bowers <[e-mail]>
> > > Signed-off-by: Fan Wu <[e-mail]>
> > > ---
> > > 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
> > > ---
> > >  fs/verity/fsverity_private.h |  2 +-
> > >  fs/verity/open.c             | 26 +++++++++++++++++++++++++-
> > >  include/linux/fsverity.h     |  2 ++
> > >  3 files changed, 28 insertions(+), 2 deletions(-)
> >
> > We need an ACK from some VFS folks on this.
> 
> Eric and/or Ted, can we get either an ACK or some feedback on this patch?
> 
> For reference, the full patchset can be found on lore at the link below:
> 
> https://lore.kernel.org/linux-security-module/[e-mail]/

Well, technically I already gave some (minor) feedback on this exact patch, and
it's not yet been addressed:
https://lore.kernel.org/linux-security-module/[e-mail]/

Of course, it would also be nice if the commit message mentioned what the patch
actually does.

At a higher level, I've said before, I'm not super happy about the use of
fsverity builtin signatures growing.  (For some of the reasons why, see the
guidance in the fsverity documentation at
https://docs.kernel.org/filesystems/fsverity.html#built-in-signature-verification)
That being said, if the people who are doing the broader review of IPE believe
this is how its fsverity integration should work, I can live with that; I don't
intend to block the IPE patchset if enough people want it to be merged.  I've
really been hoping to see engagement with the people involved in IMA, as IPE
basically duplicates/replaces IMA.  But I haven't seen that, so maybe things
need to move on without them.

- Eric

Paul Moore Nov. 2, 2023, 3:42 p.m. UTC | #6

On Wed, Nov 1, 2023 at 10:54 PM Eric Biggers <[e-mail]> wrote:
> On Wed, Nov 01, 2023 at 08:40:06PM -0400, Paul Moore wrote:
> > On Mon, Oct 23, 2023 at 11:52 PM Paul Moore <[e-mail]> wrote:
> > > On Oct  4, 2023 Fan Wu <[e-mail]> wrote:
> > > >
> > > > fsverity represents a mechanism to support both integrity and
> > > > authenticity protection of a file, supporting both signed and unsigned
> > > > digests.
> > > >
> > > > An LSM which controls access to a resource based on authenticity and
> > > > integrity of said resource, can then use this data to make an informed
> > > > decision on the authorization (provided by the LSM's policy) of said
> > > > claim.
> > > >
> > > > This effectively allows the extension of a policy enforcement layer in
> > > > LSM for fsverity, allowing for more granular control of how a
> > > > particular authenticity claim can be used. For example, "all (built-in)
> > > > signed fsverity files should be allowed to execute, but only these
> > > > hashes are allowed to be loaded as kernel modules".
> > > >
> > > > This enforcement must be done in kernel space, as a userspace only
> > > > solution would fail a simple litmus test: Download a self-contained
> > > > malicious binary that never touches the userspace stack. This
> > > > binary would still be able to execute.
> > > >
> > > > Signed-off-by: Deven Bowers <[e-mail]>
> > > > Signed-off-by: Fan Wu <[e-mail]>
> > > > ---
> > > > 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
> > > > ---
> > > >  fs/verity/fsverity_private.h |  2 +-
> > > >  fs/verity/open.c             | 26 +++++++++++++++++++++++++-
> > > >  include/linux/fsverity.h     |  2 ++
> > > >  3 files changed, 28 insertions(+), 2 deletions(-)
> > >
> > > We need an ACK from some VFS folks on this.
> >
> > Eric and/or Ted, can we get either an ACK or some feedback on this patch?
> >
> > For reference, the full patchset can be found on lore at the link below:
> >
> > https://lore.kernel.org/linux-security-module/[e-mail]/
>
> Well, technically I already gave some (minor) feedback on this exact patch, and
> it's not yet been addressed:
> https://lore.kernel.org/linux-security-module/[e-mail]/

Hopefully Fan can comment on that, unless I'm forgetting some
implementation details it seems like a reasonable request.

> Of course, it would also be nice if the commit message mentioned what the patch
> actually does.

While I think the commit description does provide a reasonable summary
of IPE as it relates to fsverify, I agree that the specifics of the
changes presented in the patch are lacking.  Fan, could you update the
commit description to add a paragraph explaining what IPE would do in
the security_inode_setsecurity() hook called from within
fsverity_inode_setsecurity()?

> At a higher level, I've said before, I'm not super happy about the use of
> fsverity builtin signatures growing.  (For some of the reasons why, see the
> guidance in the fsverity documentation at
> https://docs.kernel.org/filesystems/fsverity.html#built-in-signature-verification)
> That being said, if the people who are doing the broader review of IPE believe
> this is how its fsverity integration should work, I can live with that ...

Fan can surely elaborate on this more, but my understanding is that
IPE can help provide the missing authorization piece.

> ... I don't
> intend to block the IPE patchset if enough people want it to be merged.  I've
> really been hoping to see engagement with the people involved in IMA, as IPE
> basically duplicates/replaces IMA.  But I haven't seen that, so maybe things
> need to move on without them.

We are getting a bit beyond the integration of IPE and fsverity so I
don't want to spend a lot of time here, but IPE and IMA work quite a
bit differently as they serve different purposes.  IPE provides a file
authorization mechanism that grants access based on the specified
policy and the file's underlying backing store; IPE does not measure
files like IMA to provide additional integrity checking, it relies on
the storage medium's integrity mechanisms.

I have no doubt Fan could provide a much better summary if needed, and
of course there are the documentation patches in the patchset too.

Fan Wu Nov. 2, 2023, 7:33 p.m. UTC | #7

On 11/2/2023 8:42 AM, Paul Moore wrote:
> On Wed, Nov 1, 2023 at 10:54 PM Eric Biggers <[e-mail]> wrote:
>> On Wed, Nov 01, 2023 at 08:40:06PM -0400, Paul Moore wrote:
>>> On Mon, Oct 23, 2023 at 11:52 PM Paul Moore <[e-mail]> wrote:
>>>> On Oct  4, 2023 Fan Wu <[e-mail]> wrote:
>>>>>
>>>>> fsverity represents a mechanism to support both integrity and
>>>>> authenticity protection of a file, supporting both signed and unsigned
>>>>> digests.
>>>>>
>>>>> An LSM which controls access to a resource based on authenticity and
>>>>> integrity of said resource, can then use this data to make an informed
>>>>> decision on the authorization (provided by the LSM's policy) of said
>>>>> claim.
>>>>>
>>>>> This effectively allows the extension of a policy enforcement layer in
>>>>> LSM for fsverity, allowing for more granular control of how a
>>>>> particular authenticity claim can be used. For example, "all (built-in)
>>>>> signed fsverity files should be allowed to execute, but only these
>>>>> hashes are allowed to be loaded as kernel modules".
>>>>>
>>>>> This enforcement must be done in kernel space, as a userspace only
>>>>> solution would fail a simple litmus test: Download a self-contained
>>>>> malicious binary that never touches the userspace stack. This
>>>>> binary would still be able to execute.
>>>>>
>>>>> Signed-off-by: Deven Bowers <[e-mail]>
>>>>> Signed-off-by: Fan Wu <[e-mail]>
>>>>> ---
>>>>> 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
>>>>> ---
>>>>>   fs/verity/fsverity_private.h |  2 +-
>>>>>   fs/verity/open.c             | 26 +++++++++++++++++++++++++-
>>>>>   include/linux/fsverity.h     |  2 ++
>>>>>   3 files changed, 28 insertions(+), 2 deletions(-)
>>>>
>>>> We need an ACK from some VFS folks on this.
>>>
>>> Eric and/or Ted, can we get either an ACK or some feedback on this patch?
>>>
>>> For reference, the full patchset can be found on lore at the link below:
>>>
>>> https://lore.kernel.org/linux-security-module/[e-mail]/
>>
>> Well, technically I already gave some (minor) feedback on this exact patch, and
>> it's not yet been addressed:
>> https://lore.kernel.org/linux-security-module/[e-mail]/
Yes thanks for the review. We will certainly add the parameter 
constification in the next version.

> 
> Hopefully Fan can comment on that, unless I'm forgetting some
> implementation details it seems like a reasonable request.
> 
>> Of course, it would also be nice if the commit message mentioned what the patch
>> actually does.
> 
> While I think the commit description does provide a reasonable summary
> of IPE as it relates to fsverify, I agree that the specifics of the
> changes presented in the patch are lacking.  Fan, could you update the
> commit description to add a paragraph explaining what IPE would do in
> the security_inode_setsecurity() hook called from within
> fsverity_inode_setsecurity()?
>
Sorry for the lack of clarity. The hook call happens right after the 
built-in signature verification in fsverity. This hook's main job is to 
allow LSM to store the verified signature so that it can be used for 
future LSM checks.

>> At a higher level, I've said before, I'm not super happy about the use of
>> fsverity builtin signatures growing.  (For some of the reasons why, see the
>> guidance in the fsverity documentation at
>> https://docs.kernel.org/filesystems/fsverity.html#built-in-signature-verification)
>> That being said, if the people who are doing the broader review of IPE believe
>> this is how its fsverity integration should work, I can live with that ...
> 
> Fan can surely elaborate on this more, but my understanding is that
> IPE can help provide the missing authorization piece.
>
As Paul mentioned, with IPE, we can add kernel enforcement that only 
files with the fs-verity built-in signature enabled are allowed. 
Currently, IPE uses the above hook to save the signature in the file's 
security blob, and then it decides whether to permit or deny file 
operations based on the presence of the fs-verity built-in signature.

But our plans don't end there. In the future, we intend to add more 
features to address the security gaps mentioned in the documentation 
related to the fs-verity built-in signature. For instance, we will 
introduce a certificate policy rule to limit which signatures can be 
trusted, thus resolving issues related to the validity period of 
certificates. Additionally, we will implement file path rules, allowing 
different certificates to permit or deny access to different files.

>> ... I don't
>> intend to block the IPE patchset if enough people want it to be merged.  I've
>> really been hoping to see engagement with the people involved in IMA, as IPE
>> basically duplicates/replaces IMA.  But I haven't seen that, so maybe things
>> need to move on without them.
> 
> We are getting a bit beyond the integration of IPE and fsverity so I
> don't want to spend a lot of time here, but IPE and IMA work quite a
> bit differently as they serve different purposes.  IPE provides a file
> authorization mechanism that grants access based on the specified
> policy and the file's underlying backing store; IPE does not measure
> files like IMA to provide additional integrity checking, it relies on
> the storage medium's integrity mechanisms.
> 
> I have no doubt Fan could provide a much better summary if needed, and
> of course there are the documentation patches in the patchset too.
> 

I would like to emphasize that IPE is not meant to replace IMA.

IPE is a different approach, specifically designed for a particular part 
of IMA's capabilities, which is "appraisal. While IPE's approach also 
limits IPE's use cases to highly secure, locked-down devices where 
almost everything is made read-only. In contrast, IMA is a versatile 
solution suitable for a wide range of scenarios, and IPE cannot address 
all of these.

In practice, we often use both IMA (including measurement and appraisal) 
and IPE simultaneously in production. These two solutions can coexist 
and complement each other, and they are not mutually exclusive.

-Fan

Patch

diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index d071a6e32581..4a82716e852f 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -108,7 +108,7 @@  int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
 				     unsigned int log_blocksize,
 				     const u8 *salt, size_t salt_size);
 
-struct fsverity_info *fsverity_create_info(const struct inode *inode,
+struct fsverity_info *fsverity_create_info(struct inode *inode,
 					   struct fsverity_descriptor *desc);
 
 void fsverity_set_info(struct inode *inode, struct fsverity_info *vi);
diff --git a/fs/verity/open.c b/fs/verity/open.c
index 6c31a871b84b..5b48e2c39086 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -8,6 +8,7 @@ 
 #include "fsverity_private.h"
 
 #include <linux/mm.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 
 static struct kmem_cache *fsverity_info_cachep;
@@ -172,12 +173,28 @@  static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
 	return err;
 }
 
+#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
+static int fsverity_inode_setsecurity(struct inode *inode,
+				      struct fsverity_descriptor *desc)
+{
+	return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
+					  desc->signature,
+					  le32_to_cpu(desc->sig_size), 0);
+}
+#else
+static inline int fsverity_inode_setsecurity(struct inode *inode,
+					     struct fsverity_descriptor *desc)
+{
+	return 0;
+}
+#endif /* CONFIG_IPE_PROP_FS_VERITY*/
+
 /*
  * Create a new fsverity_info from the given fsverity_descriptor (with optional
  * appended builtin signature), and check the signature if present.  The
  * fsverity_descriptor must have already undergone basic validation.
  */
-struct fsverity_info *fsverity_create_info(const struct inode *inode,
+struct fsverity_info *fsverity_create_info(struct inode *inode,
 					   struct fsverity_descriptor *desc)
 {
 	struct fsverity_info *vi;
@@ -242,6 +259,13 @@  struct fsverity_info *fsverity_create_info(const struct inode *inode,
 		spin_lock_init(&vi->hash_page_init_lock);
 	}
 
+	err = fsverity_inode_setsecurity(inode, desc);
+	if (err == -EOPNOTSUPP)
+		err = 0;
+
+	if (err)
+		goto fail;
+
 	return vi;
 
 fail:
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 1eb7eae580be..9666721baf15 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -319,4 +319,6 @@  static inline int fsverity_prepare_setattr(struct dentry *dentry,
 	return 0;
 }
 
+#define FS_VERITY_INODE_SEC_NAME "fsverity.builtin-sig"
+
 #endif	/* _LINUX_FSVERITY_H */