Received-SPF: pass (google.com: domain of linux-kernel+bounces-51314-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 03 Feb 2024 17:25:13 -0500
Message-ID: <6ac3cca9d1d3505f3ed9c7196512f2db@paul-moore.com>
Precedence: bulk
MIME-Version: 1.0 
Content-Type: text/plain; charset=utf-8 
Content-Disposition: inline 
Content-Transfer-Encoding: 8bit
From: Paul Moore <paul@paul-moore.com>
To: Fan Wu <wufan@linux.microsoft.com>, corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org, serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org, axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org, eparis@redhat.com
Cc: linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org, 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>, Fan Wu <wufan@linux.microsoft.com>
Subject: Re: [PATCH RFC v12 15/20] ipe: add support for dm-verity as a trust  provider
References: <1706654228-17180-16-git-send-email-wufan@linux.microsoft.com>
In-Reply-To: <1706654228-17180-16-git-send-email-wufan@linux.microsoft.com>

On Jan 30, 2024 Fan Wu <wufan@linux.microsoft.com> wrote:
> 
> Allows author of IPE policy to indicate trust for a singular dm-verity
> volume, identified by roothash, through "dmverity_roothash" and all
> signed dm-verity volumes, through "dmverity_signature".
> 
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> ---
> v2:
>   + No Changes
> 
> v3:
>   + No changes
> 
> v4:
>   + No changes
> 
> v5:
>   + No changes
> 
> v6:
>   + Fix an improper cleanup that can result in
>     a leak
> 
> v7:
>   + Squash patch 08/12, 10/12 to [11/16]
> 
> v8:
>   + Undo squash of 08/12, 10/12 - separating drivers/md/ from security/
>     & block/
>   + Use common-audit function for dmverity_signature.
>   + Change implementation for storing the dm-verity digest to use the
>     newly introduced dm_verity_digest structure introduced in patch
>     14/20.
> 
> v9:
>   + Adapt to the new parser
> 
> v10:
>   + Select the Kconfig when all dependencies are enabled
> 
> v11:
>   + No changes
> 
> v12:
>   + Refactor to use struct digest_info* instead of void*
>   + Correct audit format
> ---
>  security/ipe/Kconfig         |  18 ++++++
>  security/ipe/Makefile        |   1 +
>  security/ipe/audit.c         |  37 ++++++++++-
>  security/ipe/digest.c        | 120 +++++++++++++++++++++++++++++++++++
>  security/ipe/digest.h        |  26 ++++++++
>  security/ipe/eval.c          |  90 +++++++++++++++++++++++++-
>  security/ipe/eval.h          |  10 +++
>  security/ipe/hooks.c         |  67 +++++++++++++++++++
>  security/ipe/hooks.h         |   8 +++
>  security/ipe/ipe.c           |  15 +++++
>  security/ipe/ipe.h           |   4 ++
>  security/ipe/policy.h        |   3 +
>  security/ipe/policy_parser.c |  26 +++++++-
>  13 files changed, 421 insertions(+), 4 deletions(-)
>  create mode 100644 security/ipe/digest.c
>  create mode 100644 security/ipe/digest.h
> 
> diff --git a/security/ipe/Kconfig b/security/ipe/Kconfig
> index ac4d558e69d5..7afb1ce0cb99 100644
> --- a/security/ipe/Kconfig
> +++ b/security/ipe/Kconfig
> @@ -8,6 +8,7 @@ menuconfig SECURITY_IPE
>  	depends on SECURITY && SECURITYFS && AUDIT && AUDITSYSCALL
>  	select PKCS7_MESSAGE_PARSER
>  	select SYSTEM_DATA_VERIFICATION
> +	select IPE_PROP_DM_VERITY if DM_VERITY && DM_VERITY_VERIFY_ROOTHASH_SIG
>  	help
>  	  This option enables the Integrity Policy Enforcement LSM
>  	  allowing users to define a policy to enforce a trust-based access
> @@ -15,3 +16,20 @@ menuconfig SECURITY_IPE
>  	  admins to reconfigure trust requirements on the fly.
>  
>  	  If unsure, answer N.
> +
> +if SECURITY_IPE
> +menu "IPE Trust Providers"
> +
> +config IPE_PROP_DM_VERITY
> +	bool "Enable support for dm-verity volumes"
> +	depends on DM_VERITY && DM_VERITY_VERIFY_ROOTHASH_SIG
> +	help
> +	  This option enables the properties 'dmverity_signature' and
> +	  'dmverity_roothash' in IPE policy. These properties evaluates
> +	  to TRUE when a file is evaluated against a dm-verity volume
> +	  that was mounted with a signed root-hash or the volume's
> +	  root hash matches the supplied value in the policy.
> +
> +endmenu
> +
> +endif
> diff --git a/security/ipe/Makefile b/security/ipe/Makefile
> index 2279eaa3cea3..66de53687d11 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -6,6 +6,7 @@
>  #
>  
>  obj-$(CONFIG_SECURITY_IPE) += \
> +	digest.o \
>  	eval.o \
>  	hooks.o \
>  	fs.o \
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index ed390d32c641..a4ad8e888df0 100644
> --- a/security/ipe/audit.c
> +++ b/security/ipe/audit.c
> @@ -13,6 +13,7 @@
>  #include "hooks.h"
>  #include "policy.h"
>  #include "audit.h"
> +#include "digest.h"
>  
>  #define ACTSTR(x) ((x) == IPE_ACTION_ALLOW ? "ALLOW" : "DENY")
>  
> @@ -54,8 +55,30 @@ static const char *const audit_prop_names[__IPE_PROP_MAX] = {
>  	"boot_verified=FALSE",
>  	"boot_verified=TRUE",
>  #endif /* CONFIG_BLK_DEV_INITRD */
> +#ifdef CONFIG_IPE_PROP_DM_VERITY
> +	"dmverity_roothash=",
> +	"dmverity_signature=FALSE",
> +	"dmverity_signature=TRUE",
> +#endif /* CONFIG_IPE_PROP_DM_VERITY */
>  };
>  
> +#ifdef CONFIG_IPE_PROP_DM_VERITY
> +/**
> + * audit_dmv_roothash - audit a roothash of a dmverity volume.
> + * @ab: Supplies a pointer to the audit_buffer to append to.
> + * @rh: Supplies a pointer to the digest structure.
> + */
> +static void audit_dmv_roothash(struct audit_buffer *ab, const void *rh)
> +{
> +	audit_log_format(ab, "%s", audit_prop_names[IPE_PROP_DMV_ROOTHASH]);
> +	ipe_digest_audit(ab, rh);
> +}
> +#else
> +static void audit_dmv_roothash(struct audit_buffer *ab, const void *rh)
> +{
> +}
> +#endif /* CONFIG_IPE_PROP_DM_VERITY */

I talked about this back in my review of the v11 patchset and I'm
guessing you may have missed it ... the problem with the above code is
that the fields in an audit record should remain constant, even if
there is no data for that particular field.  In cases where there is no
data to record for a given field, a "?" should be used as the field's
value, for example:

  dmverify_roothash=?

My guess is that you would want to do something like this:

  #else  /* !CONFIG_IPE_PROP_DM_VERITY */
  static void audit_dmv_roothash(...)
  {
    audit_log_format(ab, "%s=?", audit_prop_names[...]);
  }
  #endif /* CONFIG_IPE_PROP_DM_VERITY */

--
paul-moore.com