dm integrity: introduce the "fix_hmac" argument

The "fix_hmac" argument improves security of internal_hash and
journal_mac:
- the section number is mixed to the mac, so that an attacker can't
  copy sectors from one journal section to another journal section
- the superblock is protected by journal_mac
- a 16-byte salt stored in the superblock is mixed to the mac, so
  that the attacker can't detect that two disks have the same hmac
  key and also to disallow the attacker to move sectors from one
  disk to another

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Daniel Glockner <dg@emlix.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> # ReST fix
Tested-by: Milan Broz <gmazyland@gmail.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 2cc5488..ef76285 100644 (file)
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -186,6 +186,17 @@ fix_padding
        space-efficient. If this option is not present, large padding is
        used - that is for compatibility with older kernels.
 
+fix_hmac
+       Improve security of internal_hash and journal_mac:
+
+       - the section number is mixed to the mac, so that an attacker can't
+         copy sectors from one journal section to another journal section
+       - the superblock is protected by journal_mac
+       - a 16-byte salt stored in the superblock is mixed to the mac, so
+         that the attacker can't detect that two disks have the same hmac
+         key and also to disallow the attacker to move sectors from one
+         disk to another
+
 legacy_recalculate
        Allow recalculating of volumes with HMAC keys. This is disabled by
        default for security reasons - an attacker could modify the volume,

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 7e48639..46b5d54 100644 (file)
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -40,6 +40,7 @@
 #define BITMAP_BLOCK_SIZE              4096    /* don't change it */
 #define BITMAP_FLUSH_INTERVAL          (10 * HZ)
 #define DISCARD_FILLER                 0xf6
+#define SALT_SIZE                      16
 
 /*
  * Warning - DEBUG_PRINT prints security-sensitive data to the log,
@@ -57,6 +58,7 @@
 #define SB_VERSION_2                   2
 #define SB_VERSION_3                   3
 #define SB_VERSION_4                   4
+#define SB_VERSION_5                   5
 #define SB_SECTORS                     8
 #define MAX_SECTORS_PER_BLOCK          8
 
@@ -72,12 +74,15 @@ struct superblock {
        __u8 log2_blocks_per_bitmap_bit;
        __u8 pad[2];
        __u64 recalc_sector;
+       __u8 pad2[8];
+       __u8 salt[SALT_SIZE];
 };
 
 #define SB_FLAG_HAVE_JOURNAL_MAC       0x1
 #define SB_FLAG_RECALCULATING          0x2
 #define SB_FLAG_DIRTY_BITMAP           0x4
 #define SB_FLAG_FIXED_PADDING          0x8
+#define SB_FLAG_FIXED_HMAC             0x10
 
 #define        JOURNAL_ENTRY_ROUNDUP           8
 
@@ -259,6 +264,7 @@ struct dm_integrity_c {
        bool recalculate_flag;
        bool discard;
        bool fix_padding;
+       bool fix_hmac;
        bool legacy_recalculate;
 
        struct alg_spec internal_hash_alg;
@@ -389,8 +395,11 @@ static int dm_integrity_failed(struct dm_integrity_c *ic)
 
 static bool dm_integrity_disable_recalculate(struct dm_integrity_c *ic)
 {
-       if ((ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
-           !ic->legacy_recalculate)
+       if (ic->legacy_recalculate)
+               return false;
+       if (!(ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) ?
+           ic->internal_hash_alg.key || ic->journal_mac_alg.key :
+           ic->internal_hash_alg.key && !ic->journal_mac_alg.key)
                return true;
        return false;
 }
@@ -477,7 +486,9 @@ static void wraparound_section(struct dm_integrity_c *ic, unsigned *sec_ptr)
 
 static void sb_set_version(struct dm_integrity_c *ic)
 {
-       if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_PADDING))
+       if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC))
+               ic->sb->version = SB_VERSION_5;
+       else if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_PADDING))
                ic->sb->version = SB_VERSION_4;
        else if (ic->mode == 'B' || ic->sb->flags & cpu_to_le32(SB_FLAG_DIRTY_BITMAP))
                ic->sb->version = SB_VERSION_3;
@@ -487,10 +498,58 @@ static void sb_set_version(struct dm_integrity_c *ic)
                ic->sb->version = SB_VERSION_1;
 }
 
+static int sb_mac(struct dm_integrity_c *ic, bool wr)
+{
+       SHASH_DESC_ON_STACK(desc, ic->journal_mac);
+       int r;
+       unsigned size = crypto_shash_digestsize(ic->journal_mac);
+
+       if (sizeof(struct superblock) + size > 1 << SECTOR_SHIFT) {
+               dm_integrity_io_error(ic, "digest is too long", -EINVAL);
+               return -EINVAL;
+       }
+
+       desc->tfm = ic->journal_mac;
+
+       r = crypto_shash_init(desc);
+       if (unlikely(r < 0)) {
+               dm_integrity_io_error(ic, "crypto_shash_init", r);
+               return r;
+       }
+
+       r = crypto_shash_update(desc, (__u8 *)ic->sb, (1 << SECTOR_SHIFT) - size);
+       if (unlikely(r < 0)) {
+               dm_integrity_io_error(ic, "crypto_shash_update", r);
+               return r;
+       }
+
+       if (likely(wr)) {
+               r = crypto_shash_final(desc, (__u8 *)ic->sb + (1 << SECTOR_SHIFT) - size);
+               if (unlikely(r < 0)) {
+                       dm_integrity_io_error(ic, "crypto_shash_final", r);
+                       return r;
+               }
+       } else {
+               __u8 result[HASH_MAX_DIGESTSIZE];
+               r = crypto_shash_final(desc, result);
+               if (unlikely(r < 0)) {
+                       dm_integrity_io_error(ic, "crypto_shash_final", r);
+                       return r;
+               }
+               if (memcmp((__u8 *)ic->sb + (1 << SECTOR_SHIFT) - size, result, size)) {
+                       dm_integrity_io_error(ic, "superblock mac", -EILSEQ);
+                       return -EILSEQ;
+               }
+       }
+
+       return 0;
+}
+
 static int sync_rw_sb(struct dm_integrity_c *ic, int op, int op_flags)
 {
        struct dm_io_request io_req;
        struct dm_io_region io_loc;
+       int r;
 
        io_req.bi_op = op;
        io_req.bi_op_flags = op_flags;
@@ -502,10 +561,28 @@ static int sync_rw_sb(struct dm_integrity_c *ic, int op, int op_flags)
        io_loc.sector = ic->start;
        io_loc.count = SB_SECTORS;
 
-       if (op == REQ_OP_WRITE)
+       if (op == REQ_OP_WRITE) {
                sb_set_version(ic);
+               if (ic->journal_mac && ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+                       r = sb_mac(ic, true);
+                       if (unlikely(r))
+                               return r;
+               }
+       }
 
-       return dm_io(&io_req, 1, &io_loc, NULL);
+       r = dm_io(&io_req, 1, &io_loc, NULL);
+       if (unlikely(r))
+               return r;
+
+       if (op == REQ_OP_READ) {
+               if (ic->mode != 'R' && ic->journal_mac && ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+                       r = sb_mac(ic, false);
+                       if (unlikely(r))
+                               return r;
+               }
+       }
+
+       return 0;
 }
 
 #define BITMAP_OP_TEST_ALL_SET         0
@@ -722,15 +799,32 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result
        desc->tfm = ic->journal_mac;
 
        r = crypto_shash_init(desc);
-       if (unlikely(r)) {
+       if (unlikely(r < 0)) {
                dm_integrity_io_error(ic, "crypto_shash_init", r);
                goto err;
        }
 
+       if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+               uint64_t section_le;
+
+               r = crypto_shash_update(desc, (__u8 *)&ic->sb->salt, SALT_SIZE);
+               if (unlikely(r < 0)) {
+                       dm_integrity_io_error(ic, "crypto_shash_update", r);
+                       goto err;
+               }
+
+               section_le = cpu_to_le64(section);
+               r = crypto_shash_update(desc, (__u8 *)&section_le, sizeof section_le);
+               if (unlikely(r < 0)) {
+                       dm_integrity_io_error(ic, "crypto_shash_update", r);
+                       goto err;
+               }
+       }
+
        for (j = 0; j < ic->journal_section_entries; j++) {
                struct journal_entry *je = access_journal_entry(ic, section, j);
                r = crypto_shash_update(desc, (__u8 *)&je->u.sector, sizeof je->u.sector);
-               if (unlikely(r)) {
+               if (unlikely(r < 0)) {
                        dm_integrity_io_error(ic, "crypto_shash_update", r);
                        goto err;
                }
@@ -740,7 +834,7 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result
 
        if (likely(size <= JOURNAL_MAC_SIZE)) {
                r = crypto_shash_final(desc, result);
-               if (unlikely(r)) {
+               if (unlikely(r < 0)) {
                        dm_integrity_io_error(ic, "crypto_shash_final", r);
                        goto err;
                }
@@ -753,7 +847,7 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result
                        goto err;
                }
                r = crypto_shash_final(desc, digest);
-               if (unlikely(r)) {
+               if (unlikely(r < 0)) {
                        dm_integrity_io_error(ic, "crypto_shash_final", r);
                        goto err;
                }
@@ -1556,6 +1650,14 @@ static void integrity_sector_checksum(struct dm_integrity_c *ic, sector_t sector
                goto failed;
        }
 
+       if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+               r = crypto_shash_update(req, (__u8 *)&ic->sb->salt, SALT_SIZE);
+               if (unlikely(r < 0)) {
+                       dm_integrity_io_error(ic, "crypto_shash_update", r);
+                       goto failed;
+               }
+       }
+
        r = crypto_shash_update(req, (const __u8 *)&sector_le, sizeof sector_le);
        if (unlikely(r < 0)) {
                dm_integrity_io_error(ic, "crypto_shash_update", r);
@@ -3149,6 +3251,7 @@ static void dm_integrity_status(struct dm_target *ti, status_type_t type,
                arg_count += !!ic->journal_crypt_alg.alg_string;
                arg_count += !!ic->journal_mac_alg.alg_string;
                arg_count += (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_PADDING)) != 0;
+               arg_count += (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) != 0;
                arg_count += ic->legacy_recalculate;
                DMEMIT("%s %llu %u %c %u", ic->dev->name, ic->start,
                       ic->tag_size, ic->mode, arg_count);
@@ -3173,6 +3276,8 @@ static void dm_integrity_status(struct dm_target *ti, status_type_t type,
                }
                if ((ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_PADDING)) != 0)
                        DMEMIT(" fix_padding");
+               if ((ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) != 0)
+                       DMEMIT(" fix_hmac");
                if (ic->legacy_recalculate)
                        DMEMIT(" legacy_recalculate");
 
@@ -3310,6 +3415,11 @@ static int initialize_superblock(struct dm_integrity_c *ic, unsigned journal_sec
        if (!journal_sections)
                journal_sections = 1;
 
+       if (ic->fix_hmac && (ic->internal_hash_alg.alg_string || ic->journal_mac_alg.alg_string)) {
+               ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_HMAC);
+               get_random_bytes(ic->sb->salt, SALT_SIZE);
+       }
+
        if (!ic->meta_dev) {
                if (ic->fix_padding)
                        ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_PADDING);
@@ -3804,7 +3914,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
        unsigned extra_args;
        struct dm_arg_set as;
        static const struct dm_arg _args[] = {
-               {0, 16, "Invalid number of feature args"},
+               {0, 17, "Invalid number of feature args"},
        };
        unsigned journal_sectors, interleave_sectors, buffer_sectors, journal_watermark, sync_msec;
        bool should_write_sb;
@@ -3942,7 +4052,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
                        if (r)
                                goto bad;
                } else if (!strncmp(opt_string, "journal_mac:", strlen("journal_mac:"))) {
-                       r = get_alg_and_key(opt_string, &ic->journal_mac_alg,  &ti->error,
+                       r = get_alg_and_key(opt_string, &ic->journal_mac_alg, &ti->error,
                                            "Invalid journal_mac argument");
                        if (r)
                                goto bad;
@@ -3952,6 +4062,8 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
                        ic->discard = true;
                } else if (!strcmp(opt_string, "fix_padding")) {
                        ic->fix_padding = true;
+               } else if (!strcmp(opt_string, "fix_hmac")) {
+                       ic->fix_hmac = true;
                } else if (!strcmp(opt_string, "legacy_recalculate")) {
                        ic->legacy_recalculate = true;
                } else {
@@ -4110,7 +4222,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv)
                        should_write_sb = true;
        }
 
-       if (!ic->sb->version || ic->sb->version > SB_VERSION_4) {
+       if (!ic->sb->version || ic->sb->version > SB_VERSION_5) {
                r = -EINVAL;
                ti->error = "Unknown version";
                goto bad;
@@ -4442,7 +4554,7 @@ static void dm_integrity_dtr(struct dm_target *ti)
 
 static struct target_type integrity_target = {
        .name                   = "integrity",
-       .version                = {1, 6, 0},
+       .version                = {1, 7, 0},
        .module                 = THIS_MODULE,
        .features               = DM_TARGET_SINGLETON | DM_TARGET_INTEGRITY,
        .ctr                    = dm_integrity_ctr,