Hi Mike,

On Mon, Jul 12 2021 at  8:48P -0400,
Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote:

For a given system, various external services/infrastructure tools
(including the attestation service) interact with it - both during the
setup and during rest of the system run-time.  They share sensitive data
and/or execute critical workload on that system.  The external services
may want to verify the current run-time state of the relevant kernel
subsystems before fully trusting the system with business-critical
data/workload.

Device mapper is one such kernel subsystem that plays a critical role on
a given system by providing various important functionalities to the
block devices with various target types like crypt, verity, integrity 
etc.  Each of these target types’ functionalities can be configured with
various attributes.  The attributes chosen to configure these target types
can significantly impact the security profile of the block device,
and in-turn, of the system itself.  For instance, the type of encryption
algorithm and the key size determines the strength of encryption for a
given block device.

Therefore, verifying the current state of various block devices as well
as their various target attributes is crucial for external services
before fully trusting the system with business-critical data/workload.

IMA provides the necessary functionality for device mapper to measure the
state and configuration of various block devices -
  - BY device mapper itself, from within the kernel,
  - in a tamper resistant way,
  - and re-measured - triggered on state/configuration change.

This patch series uses this IMA functionality, by calling the function
ima_measure_critical_data(), when a block device state is changed (e.g.
on device create, resume, rename, remove etc.)  It measures the device
state and configuration and stores it in IMA logs, so that it can be
used by external services for managing the system.

I needed to EXPORT_SYMBOL_GPL(ima_measure_critical_data); otherwise I
couldn't compile.. not sure but I can only imagine you compile DM
(and all targets) to be builtin?

I believe the  EXPORT_SYMBOL_GPL(ima_measure_critical_data) is
now needed because of the move “#ifdef CONFIG_IMA from dm-ima.c
to dm-ima.h”, and the associated change in the Makefile 

+ifeq ($(CONFIG_IMA),y)
dm-mod-objs += dm-ima.o
+endif

Earlier I needed to export symbol only if I was calling
ima_measure_critical_data() directly from various
dm-*.c files. 
With my earlier implementation, it wasn’t needed when it
was called from the wrapper dm_ima_measure_data() in dm-ima.c.

In addition to fixing that (in first table load patch) I changed
various things along the way while I reviewed each patch.

Thank you so much for making the necessary changes Mike.
Really appreciate it.

Things that I recall are:
- moved #ifdef CONFIG_IMA from dm-ima.c to dm-ima.h
- fixed various typos and whitespace
- consistently prepend "," in STATUSTYPE_IMA's DMEMIT()s as opposed to
  having a mix or pre and postfix throughout targets
- fixed what seemed like malformed STATUSTYPE_IMA handling for
  dm-multipath -- it was DMEMIT(";") for each dm-mpath's pathgroup
- added some fields to dm-mpath, renamed some IMA names in name=value
  pairs to be more clear.

I went through the changes you made. They look fine.
In addition to the above changes, you also fixed a warning in
dm-raid.c. (initializing the recovery variable) 
Thanks again. :)

I've staged the result in linux-next via linux-dm.git's dm-5.15
branch, see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.15

I've compiled tested both with and without CONFIG_IMA set.  But
haven't actually tested the code.

No worries. I will test the changes thoroughly again.

Please send any incremental fixes relative to the dm-5.15 branch and
I'll get them folded in where appropriate.

Ok. I will send incremental patches against dm-5.15 as you've suggested.
Thanks again for the review and all the help so far.
Really appreciate it.

~Tushar

Thanks,
Mike