[PATCH v2 0/3] dm: audit event logging

[PATCH v2 0/3] dm: audit event logging

[Michael Weiß]

dm integrity and also stacked dm crypt devices track integrity
violations internally. Thus, integrity violations could be polled
from user space, e.g., by 'integritysetup status'.

From an auditing perspective, we only could see that there were
a number of integrity violations, but not when and where the
violation exactly was taking place. The current error log to
the kernel ring buffer, contains those information, time stamp and
sector on device. However, for auditing the audit subsystem provides
a separate logging mechanism which meets certain criteria for secure
audit logging.

With this small series we make use of the kernel audit framework
and extend the dm driver to log audit events in case of such
integrity violations. Further, we also log construction and
destruction of the device mappings.

We focus on dm-integrity and stacked dm-crypt devices for now.
However, the helper functions to log audit messages should be
applicable to dm verity too.

The first patch introduce generic audit wrapper functions.
The second patch makes use of the audit wrapper functions in the
dm-integrity.c.
The third patch uses the wrapper functions in dm-crypt.c.

The audit logs look like this if executing the following simple test:

# dd if=/dev/zero of=test.img bs=1M count=1024
# losetup -f test.img
# integritysetup -vD format --integrity sha256 -t 32 /dev/loop0 
# integritysetup open -D /dev/loop0 --integrity sha256 integritytest
# integritysetup status integritytest
# integritysetup close integritytest
# integritysetup open -D /dev/loop0 --integrity sha256 integritytest
# integritysetup status integritytest
# dd if=/dev/urandom of=/dev/loop0 bs=512 count=1 seek=100000
# dd if=/dev/mapper/integritytest of=/dev/null

-------------------------
audit.log from auditd

type=UNKNOWN[1336] msg=audit(1628692862.187:409): module=integrity dev=254:3 op=ctr res=1
type=UNKNOWN[1336] msg=audit(1628692862.443:410): module=integrity dev=254:3 op=dtr res=1
type=UNKNOWN[1336] msg=audit(1628692862.543:411): module=integrity dev=254:3 op=ctr res=1
type=UNKNOWN[1336] msg=audit(1628692877.943:412): module=integrity dev=254:3 op=dtr res=1

type=UNKNOWN[1336] msg=audit(1628692887.287:413): module=integrity dev=254:3 op=ctr res=1

type=UNKNOWN[1336] msg=audit(1628692925.156:417): module=integrity dev=254:3 op=dtr res=1

type=UNKNOWN[1336] msg=audit(1628692930.720:418): module=integrity dev=254:3 op=ctr res=1

type=UNKNOWN[1336] msg=audit(1628692989.344:419): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:420): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:421): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:422): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:423): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:424): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:425): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:426): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:427): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0
type=UNKNOWN[1336] msg=audit(1628692989.348:428): module=integrity dev=254:3 op=integrity-checksum sector=77480 res=0

v2 Changes:
- Fixed compile errors if CONFIG_DM_AUDIT is not set
- Fixed formatting and typos as suggested by Casey

Michael Weiß (3):
  dm: introduce audit event module for device mapper
  dm integrity: log audit events for dm-integrity target
  dm crypt: log aead integrity violations to audit subsystem

 drivers/md/Kconfig         | 10 +++++++
 drivers/md/Makefile        |  4 +++
 drivers/md/dm-audit.c      | 59 ++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-audit.h      | 33 +++++++++++++++++++++
 drivers/md/dm-crypt.c      | 22 +++++++++++---
 drivers/md/dm-integrity.c  | 25 +++++++++++++---
 include/uapi/linux/audit.h |  1 +
 7 files changed, 146 insertions(+), 8 deletions(-)
 create mode 100644 drivers/md/dm-audit.c
 create mode 100644 drivers/md/dm-audit.h

-- 
2.20.1

[PATCH v2 1/3] dm: introduce audit event module for device mapper

[Michael Weiß]

To be able to send auditing events to user space, we introduce
a generic dm-audit module. It provides helper functions to emit
audit events through the kernel audit subsystem. We claim the
AUDIT_DM type=1336 out of the audit event messages range in the
corresponding userspace api in 'include/uapi/linux/audit.h' for
those events.

Following commits to device mapper targets actually will make
use of this to emit those events in relevant cases.

Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
---
 drivers/md/Kconfig         | 10 +++++++
 drivers/md/Makefile        |  4 +++
 drivers/md/dm-audit.c      | 59 ++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-audit.h      | 33 +++++++++++++++++++++
 include/uapi/linux/audit.h |  1 +
 5 files changed, 107 insertions(+)
 create mode 100644 drivers/md/dm-audit.c
 create mode 100644 drivers/md/dm-audit.h

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 0602e82a9516..48adbec12148 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -608,6 +608,7 @@ config DM_INTEGRITY
 	select CRYPTO
 	select CRYPTO_SKCIPHER
 	select ASYNC_XOR
+	select DM_AUDIT if AUDIT
 	help
 	  This device-mapper target emulates a block device that has
 	  additional per-sector tags that can be used for storing
@@ -640,4 +641,13 @@ config DM_ZONED
 
 	  If unsure, say N.
 
+config DM_AUDIT
+	bool "DM audit events"
+	depends on AUDIT
+	help
+	  Generate audit events for device-mapper.
+
+	  Enables audit logging of several security relevant events in the
+	  particular device-mapper targets, especially the integrity target.
+
 endif # MD
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index a74aaf8b1445..4cd47623c742 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -103,3 +103,7 @@ endif
 ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
 dm-verity-objs			+= dm-verity-verify-sig.o
 endif
+
+ifeq ($(CONFIG_DM_AUDIT),y)
+dm-mod-objs				+= dm-audit.o
+endif
diff --git a/drivers/md/dm-audit.c b/drivers/md/dm-audit.c
new file mode 100644
index 000000000000..c7e5824821bb
--- /dev/null
+++ b/drivers/md/dm-audit.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Creating audit records for mapped devices.
+ *
+ * Copyright (C) 2021 Fraunhofer AISEC. All rights reserved.
+ *
+ * Authors: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
+ */
+
+#include <linux/audit.h>
+#include <linux/module.h>
+#include <linux/device-mapper.h>
+#include <linux/bio.h>
+#include <linux/blkdev.h>
+
+#include "dm-audit.h"
+#include "dm-core.h"
+
+void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
+		      struct bio *bio, sector_t sector, int result)
+{
+	struct audit_buffer *ab;
+
+	if (audit_enabled == AUDIT_OFF)
+		return;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
+	if (unlikely(!ab))
+		return;
+
+	audit_log_format(ab, "module=%s dev=%d:%d op=%s sector=%llu res=%d",
+			 dm_msg_prefix, MAJOR(bio->bi_bdev->bd_dev),
+			 MINOR(bio->bi_bdev->bd_dev), op, sector, result);
+	audit_log_end(ab);
+}
+EXPORT_SYMBOL_GPL(dm_audit_log_bio);
+
+void dm_audit_log_target(const char *dm_msg_prefix, const char *op,
+			 struct dm_target *ti, int result)
+{
+	struct audit_buffer *ab;
+	struct mapped_device *md = dm_table_get_md(ti->table);
+
+	if (audit_enabled == AUDIT_OFF)
+		return;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
+	if (unlikely(!ab))
+		return;
+
+	audit_log_format(ab, "module=%s dev=%s op=%s",
+			 dm_msg_prefix, dm_device_name(md), op);
+
+	if (!result && !strcmp("ctr", op))
+		audit_log_format(ab, " error_msg='%s'", ti->error);
+	audit_log_format(ab, " res=%d", result);
+	audit_log_end(ab);
+}
+EXPORT_SYMBOL_GPL(dm_audit_log_target);
diff --git a/drivers/md/dm-audit.h b/drivers/md/dm-audit.h
new file mode 100644
index 000000000000..b9e31b9e3682
--- /dev/null
+++ b/drivers/md/dm-audit.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Creating audit records for mapped devices.
+ *
+ * Copyright (C) 2021 Fraunhofer AISEC. All rights reserved.
+ *
+ * Authors: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
+ */
+
+#ifndef DM_AUDIT_H
+#define DM_AUDIT_H
+
+#include <linux/device-mapper.h>
+
+#ifdef CONFIG_DM_AUDIT
+void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
+		      struct bio *bio, sector_t sector, int result);
+void dm_audit_log_target(const char *dm_msg_prefix, const char *op,
+			 struct dm_target *ti, int result);
+#else
+static inline void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
+				    struct bio *bio, sector_t sector,
+				    int result)
+{
+}
+static inline void dm_audit_log_target(const char *dm_msg_prefix,
+				       const char *op, struct dm_target *ti,
+				       int result)
+{
+}
+#endif
+
+#endif
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index daa481729e9b..aebfeee1c5b1 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -118,6 +118,7 @@
 #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
 #define AUDIT_BPF		1334	/* BPF subsystem */
 #define AUDIT_EVENT_LISTENER	1335	/* Task joined multicast read socket */
+#define AUDIT_DM		1336	/* Device Mapper events */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
-- 
2.20.1

[PATCH v2 2/3] dm integrity: log audit events for dm-integrity target

[Michael Weiß]

dm-integrity signals integrity violations by returning I/O errors
to user space. To identify integrity violations by a controlling
instance, the kernel audit subsystem can be used to emit audit
events to user space. We use the new dm-audit submodule allowing
to emit audit events on relevant I/O errors.

The construction and destruction of integrity device mappings are
also relevant for auditing a system. Thus, those events are also
logged as audit events.

Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
---
 drivers/md/dm-integrity.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 20f2510db1f6..fbbb4c3f16cb 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -23,6 +23,8 @@
 #include <linux/async_tx.h>
 #include <linux/dm-bufio.h>
 
+#include "dm-audit.h"
+
 #define DM_MSG_PREFIX "integrity"
 
 #define DEFAULT_INTERLEAVE_SECTORS	32768
@@ -539,6 +541,7 @@ static int sb_mac(struct dm_integrity_c *ic, bool wr)
 		}
 		if (memcmp((__u8 *)ic->sb + (1 << SECTOR_SHIFT) - size, result, size)) {
 			dm_integrity_io_error(ic, "superblock mac", -EILSEQ);
+			dm_audit_log_target(DM_MSG_PREFIX, "mac-superblock", ic->ti, 0);
 			return -EILSEQ;
 		}
 	}
@@ -876,8 +879,10 @@ static void rw_section_mac(struct dm_integrity_c *ic, unsigned section, bool wr)
 		if (likely(wr))
 			memcpy(&js->mac, result + (j * JOURNAL_MAC_PER_SECTOR), JOURNAL_MAC_PER_SECTOR);
 		else {
-			if (memcmp(&js->mac, result + (j * JOURNAL_MAC_PER_SECTOR), JOURNAL_MAC_PER_SECTOR))
+			if (memcmp(&js->mac, result + (j * JOURNAL_MAC_PER_SECTOR), JOURNAL_MAC_PER_SECTOR)) {
 				dm_integrity_io_error(ic, "journal mac", -EILSEQ);
+				dm_audit_log_target(DM_MSG_PREFIX, "mac-journal", ic->ti, 0);
+			}
 		}
 	}
 }
@@ -1782,10 +1787,15 @@ static void integrity_metadata(struct work_struct *w)
 			if (unlikely(r)) {
 				if (r > 0) {
 					char b[BDEVNAME_SIZE];
-					DMERR_LIMIT("%s: Checksum failed at sector 0x%llx", bio_devname(bio, b),
-						    (sector - ((r + ic->tag_size - 1) / ic->tag_size)));
+					sector_t s;
+
+					s = sector - ((r + ic->tag_size - 1) / ic->tag_size);
+					DMERR_LIMIT("%s: Checksum failed at sector 0x%llx",
+						    bio_devname(bio, b), s);
 					r = -EILSEQ;
 					atomic64_inc(&ic->number_of_mismatches);
+					dm_audit_log_bio(DM_MSG_PREFIX, "integrity-checksum",
+							 bio, s, 0);
 				}
 				if (likely(checksums != checksums_onstack))
 					kfree(checksums);
@@ -1991,6 +2001,8 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio,
 					if (unlikely(memcmp(checksums_onstack, journal_entry_tag(ic, je), ic->tag_size))) {
 						DMERR_LIMIT("Checksum failed when reading from journal, at sector 0x%llx",
 							    logical_sector);
+						dm_audit_log_bio(DM_MSG_PREFIX, "journal-checksum",
+								 bio, logical_sector, 0);
 					}
 				}
 #endif
@@ -2534,8 +2546,10 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
 
 					integrity_sector_checksum(ic, sec + ((l - j) << ic->sb->log2_sectors_per_block),
 								  (char *)access_journal_data(ic, i, l), test_tag);
-					if (unlikely(memcmp(test_tag, journal_entry_tag(ic, je2), ic->tag_size)))
+					if (unlikely(memcmp(test_tag, journal_entry_tag(ic, je2), ic->tag_size))) {
 						dm_integrity_io_error(ic, "tag mismatch when replaying journal", -EILSEQ);
+						dm_audit_log_target(DM_MSG_PREFIX, "integrity-replay-journal", ic->ti, 0);
+					}
 				}
 
 				journal_entry_set_unused(je2);
@@ -4490,9 +4504,11 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (ic->discard)
 		ti->num_discard_bios = 1;
 
+	dm_audit_log_target(DM_MSG_PREFIX, "ctr", ti, 1);
 	return 0;
 
 bad:
+	dm_audit_log_target(DM_MSG_PREFIX, "ctr", ti, 0);
 	dm_integrity_dtr(ti);
 	return r;
 }
@@ -4566,6 +4582,7 @@ static void dm_integrity_dtr(struct dm_target *ti)
 	free_alg(&ic->journal_mac_alg);
 
 	kfree(ic);
+	dm_audit_log_target(DM_MSG_PREFIX, "dtr", ti, 1);
 }
 
 static struct target_type integrity_target = {
-- 
2.20.1

[PATCH v2 3/3] dm crypt: log aead integrity violations to audit subsystem

[Michael Weiß]

Since dm-crypt target can be stacked on dm-integrity targets to
provide authenticated encryption, integrity violations are recognized
here during aead computation. We use the dm-audit submodule to
signal those events to user space, too.

The construction and destruction of crypt device mappings are also
logged as audit events.

Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
---
 drivers/md/dm-crypt.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 50f4cbd600d5..2a336eacb50c 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -41,6 +41,8 @@
 
 #include <linux/device-mapper.h>
 
+#include "dm-audit.h"
+
 #define DM_MSG_PREFIX "crypt"
 
 /*
@@ -1362,8 +1364,12 @@ static int crypt_convert_block_aead(struct crypt_config *cc,
 
 	if (r == -EBADMSG) {
 		char b[BDEVNAME_SIZE];
-		DMERR_LIMIT("%s: INTEGRITY AEAD ERROR, sector %llu", bio_devname(ctx->bio_in, b),
-			    (unsigned long long)le64_to_cpu(*sector));
+		sector_t s = le64_to_cpu(*sector);
+
+		DMERR_LIMIT("%s: INTEGRITY AEAD ERROR, sector %llu",
+			    bio_devname(ctx->bio_in, b), s);
+		dm_audit_log_bio(DM_MSG_PREFIX, "integrity-aead",
+				 ctx->bio_in, s, 0);
 	}
 
 	if (!r && cc->iv_gen_ops && cc->iv_gen_ops->post)
@@ -2173,8 +2179,12 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 
 	if (error == -EBADMSG) {
 		char b[BDEVNAME_SIZE];
-		DMERR_LIMIT("%s: INTEGRITY AEAD ERROR, sector %llu", bio_devname(ctx->bio_in, b),
-			    (unsigned long long)le64_to_cpu(*org_sector_of_dmreq(cc, dmreq)));
+		sector_t s = le64_to_cpu(*org_sector_of_dmreq(cc, dmreq));
+
+		DMERR_LIMIT("%s: INTEGRITY AEAD ERROR, sector %llu",
+			    bio_devname(ctx->bio_in, b), s);
+		dm_audit_log_bio(DM_MSG_PREFIX, "integrity-aead",
+				 ctx->bio_in, s, 0);
 		io->error = BLK_STS_PROTECTION;
 	} else if (error < 0)
 		io->error = BLK_STS_IOERR;
@@ -2729,6 +2739,8 @@ static void crypt_dtr(struct dm_target *ti)
 	dm_crypt_clients_n--;
 	crypt_calculate_pages_per_client();
 	spin_unlock(&dm_crypt_clients_lock);
+
+	dm_audit_log_target(DM_MSG_PREFIX, "dtr", ti, 1);
 }
 
 static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
@@ -3357,9 +3369,11 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_flush_bios = 1;
 	ti->limit_swap_bios = true;
 
+	dm_audit_log_target(DM_MSG_PREFIX, "ctr", ti, 1);
 	return 0;
 
 bad:
+	dm_audit_log_target(DM_MSG_PREFIX, "ctr", ti, 0);
 	crypt_dtr(ti);
 	return ret;
 }
-- 
2.20.1

Re: [PATCH v2 1/3] dm: introduce audit event module for device mapper

[Paul Moore]

On Sat, Aug 14, 2021 at 2:34 PM Michael Weiß
<michael.weiss@aisec.fraunhofer.de> wrote:
>
> To be able to send auditing events to user space, we introduce
> a generic dm-audit module. It provides helper functions to emit
> audit events through the kernel audit subsystem. We claim the
> AUDIT_DM type=1336 out of the audit event messages range in the
> corresponding userspace api in 'include/uapi/linux/audit.h' for
> those events.
>
> Following commits to device mapper targets actually will make
> use of this to emit those events in relevant cases.
>
> Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>

Hi Michael,

You went into more detail in your patchset cover letter, e.g. example
audit records, which I think would be helpful here in the commit
description so we have it as part of the git log.  I don't want to
discourage you from writing cover letters, but don't forget that the
cover letters can be lost to the ether after a couple of years whereas
the git log has a much longer lifetime (we hope!) and a tighter
binding to the related code.

> ---
>  drivers/md/Kconfig         | 10 +++++++
>  drivers/md/Makefile        |  4 +++
>  drivers/md/dm-audit.c      | 59 ++++++++++++++++++++++++++++++++++++++
>  drivers/md/dm-audit.h      | 33 +++++++++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  5 files changed, 107 insertions(+)
>  create mode 100644 drivers/md/dm-audit.c
>  create mode 100644 drivers/md/dm-audit.h
>
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index 0602e82a9516..48adbec12148 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -608,6 +608,7 @@ config DM_INTEGRITY
>         select CRYPTO
>         select CRYPTO_SKCIPHER
>         select ASYNC_XOR
> +       select DM_AUDIT if AUDIT
>         help
>           This device-mapper target emulates a block device that has
>           additional per-sector tags that can be used for storing
> @@ -640,4 +641,13 @@ config DM_ZONED
>
>           If unsure, say N.
>
> +config DM_AUDIT
> +       bool "DM audit events"
> +       depends on AUDIT
> +       help
> +         Generate audit events for device-mapper.
> +
> +         Enables audit logging of several security relevant events in the
> +         particular device-mapper targets, especially the integrity target.
> +
>  endif # MD
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index a74aaf8b1445..4cd47623c742 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -103,3 +103,7 @@ endif
>  ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
>  dm-verity-objs                 += dm-verity-verify-sig.o
>  endif
> +
> +ifeq ($(CONFIG_DM_AUDIT),y)
> +dm-mod-objs                            += dm-audit.o
> +endif
> diff --git a/drivers/md/dm-audit.c b/drivers/md/dm-audit.c
> new file mode 100644
> index 000000000000..c7e5824821bb
> --- /dev/null
> +++ b/drivers/md/dm-audit.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Creating audit records for mapped devices.
> + *
> + * Copyright (C) 2021 Fraunhofer AISEC. All rights reserved.
> + *
> + * Authors: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
> + */
> +
> +#include <linux/audit.h>
> +#include <linux/module.h>
> +#include <linux/device-mapper.h>
> +#include <linux/bio.h>
> +#include <linux/blkdev.h>
> +
> +#include "dm-audit.h"
> +#include "dm-core.h"
> +
> +void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
> +                     struct bio *bio, sector_t sector, int result)
> +{
> +       struct audit_buffer *ab;
> +
> +       if (audit_enabled == AUDIT_OFF)
> +               return;
> +
> +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
> +       if (unlikely(!ab))
> +               return;
> +
> +       audit_log_format(ab, "module=%s dev=%d:%d op=%s sector=%llu res=%d",
> +                        dm_msg_prefix, MAJOR(bio->bi_bdev->bd_dev),
> +                        MINOR(bio->bi_bdev->bd_dev), op, sector, result);
> +       audit_log_end(ab);
> +}
> +EXPORT_SYMBOL_GPL(dm_audit_log_bio);
> +
> +void dm_audit_log_target(const char *dm_msg_prefix, const char *op,
> +                        struct dm_target *ti, int result)
> +{
> +       struct audit_buffer *ab;
> +       struct mapped_device *md = dm_table_get_md(ti->table);
> +
> +       if (audit_enabled == AUDIT_OFF)
> +               return;
> +
> +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
> +       if (unlikely(!ab))
> +               return;
> +
> +       audit_log_format(ab, "module=%s dev=%s op=%s",
> +                        dm_msg_prefix, dm_device_name(md), op);
> +
> +       if (!result && !strcmp("ctr", op))
> +               audit_log_format(ab, " error_msg='%s'", ti->error);
> +       audit_log_format(ab, " res=%d", result);
> +       audit_log_end(ab);
> +}

Generally speaking we try to keep a consistent format and ordering
within a given audit record type.  However you appear to have at least
three different formats for the AUDIT_DM record in this patch:

  "... module=%s dev=%d:%d op=%s sector=%llu res=%d"
  "... module=%s dev=%s op=%s error_msg='%s' res=%d"
  "... module=%s dev=%s op=%s res=%d"

The first thing that jumps out is that some fields, e.g. "sector", are
not always present in the record; we typically handle this by using a
"?" for the field value in those cases where you would otherwise drop
the field from the record, for example the following record:

  "... module=%s dev=%s op=%s res=%d"

... would be rewritten like this:

  "... module=%s dev=%s op=%s sector=? res=%d"

The second thing that I noticed is that the "dev" field changes from a
"major:minor" number representation to an arbitrary string value, e.g.
"dev=%s".  This generally isn't something we do with audit records,
please stick to a single representation for a given audit
record-type/field combination.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/3] dm: introduce audit event module for device mapper

[Weiß, Michael]

On Wed, 2021-08-18 at 14:59 -0400, Paul Moore wrote:
> On Sat, Aug 14, 2021 at 2:34 PM Michael Weiß
> <michael.weiss@aisec.fraunhofer.de> wrote:
> > To be able to send auditing events to user space, we introduce
> > a generic dm-audit module. It provides helper functions to emit
> > audit events through the kernel audit subsystem. We claim the
> > AUDIT_DM type=1336 out of the audit event messages range in the
> > corresponding userspace api in 'include/uapi/linux/audit.h' for
> > those events.
> > 
> > Following commits to device mapper targets actually will make
> > use of this to emit those events in relevant cases.
> > 
> > Signed-off-by: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
> 
> Hi Michael,
> 
> You went into more detail in your patchset cover letter, e.g. example
> audit records, which I think would be helpful here in the commit
> description so we have it as part of the git log.  I don't want to
> discourage you from writing cover letters, but don't forget that the
> cover letters can be lost to the ether after a couple of years whereas
> the git log has a much longer lifetime (we hope!) and a tighter
> binding to the related code.

Hi Paul,

at first thank you for your comments.
I see your point and I will respect that in providing the next version of
this patch-set.

> 
> > ---
> >  drivers/md/Kconfig         | 10 +++++++
> >  drivers/md/Makefile        |  4 +++
> >  drivers/md/dm-audit.c      | 59 ++++++++++++++++++++++++++++++++++++++
> >  drivers/md/dm-audit.h      | 33 +++++++++++++++++++++
> >  include/uapi/linux/audit.h |  1 +
> >  5 files changed, 107 insertions(+)
> >  create mode 100644 drivers/md/dm-audit.c
> >  create mode 100644 drivers/md/dm-audit.h
> > 
> > diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> > index 0602e82a9516..48adbec12148 100644
> > --- a/drivers/md/Kconfig
> > +++ b/drivers/md/Kconfig
> > @@ -608,6 +608,7 @@ config DM_INTEGRITY
> >         select CRYPTO
> >         select CRYPTO_SKCIPHER
> >         select ASYNC_XOR
> > +       select DM_AUDIT if AUDIT
> >         help
> >           This device-mapper target emulates a block device that has
> >           additional per-sector tags that can be used for storing
> > @@ -640,4 +641,13 @@ config DM_ZONED
> > 
> >           If unsure, say N.
> > 
> > +config DM_AUDIT
> > +       bool "DM audit events"
> > +       depends on AUDIT
> > +       help
> > +         Generate audit events for device-mapper.
> > +
> > +         Enables audit logging of several security relevant events in the
> > +         particular device-mapper targets, especially the integrity target.
> > +
> >  endif # MD
> > diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> > index a74aaf8b1445..4cd47623c742 100644
> > --- a/drivers/md/Makefile
> > +++ b/drivers/md/Makefile
> > @@ -103,3 +103,7 @@ endif
> >  ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
> >  dm-verity-objs                 += dm-verity-verify-sig.o
> >  endif
> > +
> > +ifeq ($(CONFIG_DM_AUDIT),y)
> > +dm-mod-objs                            += dm-audit.o
> > +endif
> > diff --git a/drivers/md/dm-audit.c b/drivers/md/dm-audit.c
> > new file mode 100644
> > index 000000000000..c7e5824821bb
> > --- /dev/null
> > +++ b/drivers/md/dm-audit.c
> > @@ -0,0 +1,59 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Creating audit records for mapped devices.
> > + *
> > + * Copyright (C) 2021 Fraunhofer AISEC. All rights reserved.
> > + *
> > + * Authors: Michael Weiß <michael.weiss@aisec.fraunhofer.de>
> > + */
> > +
> > +#include <linux/audit.h>
> > +#include <linux/module.h>
> > +#include <linux/device-mapper.h>
> > +#include <linux/bio.h>
> > +#include <linux/blkdev.h>
> > +
> > +#include "dm-audit.h"
> > +#include "dm-core.h"
> > +
> > +void dm_audit_log_bio(const char *dm_msg_prefix, const char *op,
> > +                     struct bio *bio, sector_t sector, int result)
> > +{
> > +       struct audit_buffer *ab;
> > +
> > +       if (audit_enabled == AUDIT_OFF)
> > +               return;
> > +
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
> > +       if (unlikely(!ab))
> > +               return;
> > +
> > +       audit_log_format(ab, "module=%s dev=%d:%d op=%s sector=%llu res=%d",
> > +                        dm_msg_prefix, MAJOR(bio->bi_bdev->bd_dev),
> > +                        MINOR(bio->bi_bdev->bd_dev), op, sector, result);
> > +       audit_log_end(ab);
> > +}
> > +EXPORT_SYMBOL_GPL(dm_audit_log_bio);
> > +
> > +void dm_audit_log_target(const char *dm_msg_prefix, const char *op,
> > +                        struct dm_target *ti, int result)
> > +{
> > +       struct audit_buffer *ab;
> > +       struct mapped_device *md = dm_table_get_md(ti->table);
> > +
> > +       if (audit_enabled == AUDIT_OFF)
> > +               return;
> > +
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_DM);
> > +       if (unlikely(!ab))
> > +               return;
> > +
> > +       audit_log_format(ab, "module=%s dev=%s op=%s",
> > +                        dm_msg_prefix, dm_device_name(md), op);
> > +
> > +       if (!result && !strcmp("ctr", op))
> > +               audit_log_format(ab, " error_msg='%s'", ti->error);
> > +       audit_log_format(ab, " res=%d", result);
> > +       audit_log_end(ab);
> > +}
> 
> Generally speaking we try to keep a consistent format and ordering
> within a given audit record type.  However you appear to have at least
> three different formats for the AUDIT_DM record in this patch:
> 
>   "... module=%s dev=%d:%d op=%s sector=%llu res=%d"
>   "... module=%s dev=%s op=%s error_msg='%s' res=%d"
>   "... module=%s dev=%s op=%s res=%d"
> 
> The first thing that jumps out is that some fields, e.g. "sector", are
> not always present in the record; we typically handle this by using a
> "?" for the field value in those cases where you would otherwise drop
> the field from the record, for example the following record:
> 
>   "... module=%s dev=%s op=%s res=%d"
> 
> ... would be rewritten like this:
> 
>   "... module=%s dev=%s op=%s sector=? res=%d"

Well, I didn't know that.
For target creation and destruction a sector is not relevant.
So would it also be an option for you if we just define two different
type of messages like this in audit.h?

#define AUDIT_DM_CTRL           1336    /* Device Mapper target control */
#define AUDIT_DM_EVENT		1337	/* Device Mapper events */

> 
> The second thing that I noticed is that the "dev" field changes from a
> "major:minor" number representation to an arbitrary string value, e.g.
> "dev=%s".  This generally isn't something we do with audit records,
> please stick to a single representation for a given audit
> record-type/field combination.

dm_device_name(md) already does provide a major:minor in string
representation, that is why I used it
directly with dev=%s
and in the bio case build it up manually out of major and minor
of the bdev.

I see two options here to be more clear on this in the code.
First, just provide a
comment or second use the major minor directly from
dm_disk(md)->major, dm_disk(md)->first_minor.

I'm not sure what's better here.


Thanks,
Michael