Date:   Sat, 4 Sep 2021 09:24:57 +0000
From:   Weiß, Michael 
        <michael.weiss@...ec.fraunhofer.de>
To:     "paul@...l-moore.com" <paul@...l-moore.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-audit@...hat.com" <linux-audit@...hat.com>,
        "casey@...aufler-ca.com" <casey@...aufler-ca.com>,
        "agk@...hat.com" <agk@...hat.com>,
        "linux-raid@...r.kernel.org" <linux-raid@...r.kernel.org>,
        "song@...nel.org" <song@...nel.org>,
        "eparis@...hat.com" <eparis@...hat.com>,
        "snitzer@...hat.com" <snitzer@...hat.com>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>
Subject: Re: [PATCH v3 1/3] dm: introduce audit event module for device mapper

On Fri, 2021-09-03 at 13:27 -0400, Paul Moore wrote:
> On Tue, Aug 31, 2021 at 3:19 PM Michael Weiß
> <michael.weiss@...ec.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_CTRL type=1336 and AUDIT_DM_EVENT type=1337 out of the
> > audit event messages range in the corresponding userspace api in
> > 'include/uapi/linux/audit.h' for those events.
> > 
> > AUDIT_DM_CTRL is used to provide information about creation and
> > destruction of device mapper targets which are triggered by user space
> > admin control actions.
> > AUDIT_DM_EVENT is used to provide information about actual errors
> > during operation of the mapped device, showing e.g. integrity
> > violations in audit log.
> > 
> > Following commits to device mapper targets actually will make use of
> > this to emit those events in relevant cases.
> > 
> > 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(1630425039.363:184): module=integrity
> > op=ctr ppid=3807 pid=3819 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > type=UNKNOWN[1336] msg=audit(1630425039.471:185): module=integrity
> > op=dtr ppid=3807 pid=3819 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > type=UNKNOWN[1336] msg=audit(1630425039.611:186): module=integrity
> > op=ctr ppid=3807 pid=3819 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > type=UNKNOWN[1336] msg=audit(1630425054.475:187): module=integrity
> > op=dtr ppid=3807 pid=3819 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > 
> > type=UNKNOWN[1336] msg=audit(1630425073.171:191): module=integrity
> > op=ctr ppid=3807 pid=3883 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > 
> > type=UNKNOWN[1336] msg=audit(1630425087.239:192): module=integrity
> > op=dtr ppid=3807 pid=3902 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > 
> > type=UNKNOWN[1336] msg=audit(1630425093.755:193): module=integrity
> > op=ctr ppid=3807 pid=3906 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0
> > egid=0 sgid=0 fsgid=0 tty=pts2 ses=3 comm="integritysetup"
> > exe="/sbin/integritysetup" subj==unconfined dev=254:3
> > error_msg='success' res=1
> > 
> > type=UNKNOWN[1337] msg=audit(1630425112.119:194): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:195): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:196): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:197): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:198): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:199): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:200): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:201): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:202): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > type=UNKNOWN[1337] msg=audit(1630425112.119:203): module=integrity
> > op=integrity-checksum dev=254:3 sector 77480 res=0
> > 
> > Signed-off-by: Michael Weiß <michael.weiss@...ec.fraunhofer.de>
> > ---
> >  drivers/md/Kconfig         | 10 +++++
> >  drivers/md/Makefile        |  4 ++
> >  drivers/md/dm-audit.c      | 79 ++++++++++++++++++++++++++++++++++++++
> >  drivers/md/dm-audit.h      | 62 ++++++++++++++++++++++++++++++
> >  include/uapi/linux/audit.h |  2 +
> >  5 files changed, 157 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..2f83d649500d 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..761ecfdcd49a
> > --- /dev/null
> > +++ b/drivers/md/dm-audit.c
> > @@ -0,0 +1,79 @@
> > +// 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@...ec.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"
> > +
> > +static struct audit_buffer *dm_audit_log_start(int audit_type,
> > +                                              const char *dm_msg_prefix,
> > +                                              const char *op)
> > +{
> > +       struct audit_buffer *ab;
> > +
> > +       if (audit_enabled == AUDIT_OFF)
> > +               return NULL;
> > +
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, audit_type);
> > +       if (unlikely(!ab))
> > +               return NULL;
> > +
> > +       audit_log_format(ab, "module=%s op=%s", dm_msg_prefix, op);
> > +       return ab;
> > +}
> > +
> > +void dm_audit_log_ti(int audit_type, 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);
> > +       int dev_major = dm_disk(md)->major;
> > +       int dev_minor = dm_disk(md)->first_minor;
> > +
> > +       ab = dm_audit_log_start(audit_type, dm_msg_prefix, op);
> > +       if (unlikely(!ab))
> > +               return;
> > +
> > +       switch (audit_type) {
> > +       case AUDIT_DM_CTRL:
> > +               audit_log_task_info(ab);
> > +               audit_log_format(ab, " dev=%d:%d error_msg='%s'", dev_major,
> > +                                dev_minor, !result ? ti->error : "success");
> > +               break;
> > +       case AUDIT_DM_EVENT:
> > +               audit_log_format(ab, " dev=%d:%d sector=?", dev_major,
> > +                                dev_minor);
> > +               break;
> > +       }
> > +       audit_log_format(ab, " res=%d", result);
> > +       audit_log_end(ab);
> > +}
> > +EXPORT_SYMBOL_GPL(dm_audit_log_ti);
> 
> Just checking, but are you okay when the inevitable happens and
> someone passes an @audit_type that is not either AUDIT_CM_CTRL or
> AUDIT_DM_EVENT?  Right now that will succeed without error and give a
> rather short audit record.
> 
Well, this function is not intended to be called directly from other dm
modules. Only the corresponding wrapper functions in dm-audit.h should
be called. I will add a comment in the header and provide a default case
which does not log anything here.

> > +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;
> > +       int dev_major = MAJOR(bio->bi_bdev->bd_dev);
> > +       int dev_minor = MINOR(bio->bi_bdev->bd_dev);
> > +
> > +       ab = dm_audit_log_start(AUDIT_DM_EVENT, dm_msg_prefix, op);
> > +       if (unlikely(!ab))
> > +               return;
> > +
> > +       audit_log_format(ab, " dev=%d:%d sector %llu res=%d",
> > +                        dev_major, dev_minor, sector, result);
> 
> I think you forgot the "=" after "sector", e.g. "sector=%llu".
> 
Damn, you are right. I'll fix that.

> > +       audit_log_end(ab);
> > +}
> > +EXPORT_SYMBOL_GPL(dm_audit_log_bio);

-- 
Thanks,
Michael

Powered by blists - more mailing lists