* [dm-devel] [PATCH v2 2/2] dm-integrity: introduce the "fix_hmac" argument
@ 2021-01-20 19:00 Mikulas Patocka
2021-01-21 12:40 ` [dm-devel] [PATCH v3 " Mikulas Patocka
0 siblings, 1 reply; 5+ messages in thread
From: Mikulas Patocka @ 2021-01-20 19:00 UTC (permalink / raw)
To: Mike Snitzer, Milan Broz, Ondrej Kozina
Cc: Laura Abbott, dm-devel, security, Alasdair Kergon, Daniel Glöckner
Introduce the "fix_hmac" arguent. It improves security of 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
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Daniel Glockner <dg@emlix.com>
---
Documentation/admin-guide/device-mapper/dm-integrity.rst | 12 +
drivers/md/dm-integrity.c | 104 +++++++++++++--
2 files changed, 105 insertions(+), 11 deletions(-)
Index: linux-2.6/drivers/md/dm-integrity.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-integrity.c
+++ linux-2.6/drivers/md/dm-integrity.c
@@ -57,6 +57,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
@@ -78,6 +79,7 @@ struct superblock {
#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 +261,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,7 +392,8 @@ static int dm_integrity_failed(struct dm
static bool dm_integrity_disable_recalculate(struct dm_integrity_c *ic)
{
- if ((ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
+ if (!ic->fix_hmac &&
+ (ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
!ic->legacy_recalculate)
return true;
return false;
@@ -477,7 +481,9 @@ static void wraparound_section(struct dm
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 +493,58 @@ static void sb_set_version(struct dm_int
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)) {
+ 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)) {
+ 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)) {
+ 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)) {
+ 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 +556,28 @@ static int sync_rw_sb(struct dm_integrit
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;
+ }
+ }
+
+ r = dm_io(&io_req, 1, &io_loc, NULL);
+ if (unlikely(r))
+ return r;
- return dm_io(&io_req, 1, &io_loc, NULL);
+ 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
@@ -727,6 +799,15 @@ static void section_mac(struct dm_integr
goto err;
}
+ if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+ uint64_t section_le = cpu_to_le64(section);
+ r = crypto_shash_update(desc, (__u8 *)§ion_le, sizeof section_le);
+ if (unlikely(r)) {
+ 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);
@@ -3149,6 +3230,7 @@ static void dm_integrity_status(struct d
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 +3255,8 @@ static void dm_integrity_status(struct d
}
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 +3394,9 @@ static int initialize_superblock(struct
if (!journal_sections)
journal_sections = 1;
+ if (ic->fix_hmac && ic->journal_mac_alg.alg_string)
+ ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_HMAC);
+
if (!ic->meta_dev) {
if (ic->fix_padding)
ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_PADDING);
@@ -3804,7 +3891,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4029,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4039,8 @@ static int dm_integrity_ctr(struct dm_ta
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 +4199,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4531,7 @@ static void dm_integrity_dtr(struct dm_t
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,
Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
===================================================================
--- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -186,6 +186,12 @@ 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 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
+
legacy_recalculate
Allow recalculating of volumes with HMAC keys. This is disabled by
default for security reasons - an attacker could modify the volume,
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dm-devel] [PATCH v3 2/2] dm-integrity: introduce the "fix_hmac" argument
2021-01-20 19:00 [dm-devel] [PATCH v2 2/2] dm-integrity: introduce the "fix_hmac" argument Mikulas Patocka
@ 2021-01-21 12:40 ` Mikulas Patocka
2021-01-21 15:09 ` [dm-devel] [PATCH v4 " Mikulas Patocka
0 siblings, 1 reply; 5+ messages in thread
From: Mikulas Patocka @ 2021-01-21 12:40 UTC (permalink / raw)
To: Mike Snitzer, Milan Broz, Ondrej Kozina
Cc: Laura Abbott, dm-devel, security, Alasdair Kergon, Daniel Glöckner
Hi
This is third version of the second patch. We change
dm_integrity_disable_recalculate to fix a security hole found by Milan.
Mikulas
From: Mikulas Patocka <mpatocka@redhat.com>
Introduce the "fix_hmac" arguent. It improves security of 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
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Daniel Glockner <dg@emlix.com>
---
Documentation/admin-guide/device-mapper/dm-integrity.rst | 12 +
drivers/md/dm-integrity.c | 104 +++++++++++++--
2 files changed, 105 insertions(+), 11 deletions(-)
Index: linux-2.6/drivers/md/dm-integrity.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-integrity.c
+++ linux-2.6/drivers/md/dm-integrity.c
@@ -57,6 +57,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
@@ -78,6 +79,7 @@ struct superblock {
#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 +261,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,7 +392,8 @@ static int dm_integrity_failed(struct dm
static bool dm_integrity_disable_recalculate(struct dm_integrity_c *ic)
{
- if ((ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
+ if (!(ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) &&
+ (ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
!ic->legacy_recalculate)
return true;
return false;
@@ -477,7 +481,9 @@ static void wraparound_section(struct dm
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 +493,58 @@ static void sb_set_version(struct dm_int
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)) {
+ 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)) {
+ 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)) {
+ 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)) {
+ 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 +556,28 @@ static int sync_rw_sb(struct dm_integrit
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;
+ }
+ }
+
+ r = dm_io(&io_req, 1, &io_loc, NULL);
+ if (unlikely(r))
+ return r;
- return dm_io(&io_req, 1, &io_loc, NULL);
+ 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
@@ -727,6 +799,15 @@ static void section_mac(struct dm_integr
goto err;
}
+ if (ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) {
+ uint64_t section_le = cpu_to_le64(section);
+ r = crypto_shash_update(desc, (__u8 *)§ion_le, sizeof section_le);
+ if (unlikely(r)) {
+ 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);
@@ -3149,6 +3230,7 @@ static void dm_integrity_status(struct d
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 +3255,8 @@ static void dm_integrity_status(struct d
}
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 +3394,9 @@ static int initialize_superblock(struct
if (!journal_sections)
journal_sections = 1;
+ if (ic->fix_hmac && ic->journal_mac_alg.alg_string)
+ ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_HMAC);
+
if (!ic->meta_dev) {
if (ic->fix_padding)
ic->sb->flags |= cpu_to_le32(SB_FLAG_FIXED_PADDING);
@@ -3804,7 +3891,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4029,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4039,8 @@ static int dm_integrity_ctr(struct dm_ta
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 +4199,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4531,7 @@ static void dm_integrity_dtr(struct dm_t
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,
Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
===================================================================
--- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -186,6 +186,12 @@ 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 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
+
legacy_recalculate
Allow recalculating of volumes with HMAC keys. This is disabled by
default for security reasons - an attacker could modify the volume,
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dm-devel] [PATCH v4 2/2] dm-integrity: introduce the "fix_hmac" argument
2021-01-21 12:40 ` [dm-devel] [PATCH v3 " Mikulas Patocka
@ 2021-01-21 15:09 ` Mikulas Patocka
2021-01-23 20:26 ` [dm-devel] [PATCH v5 " Mikulas Patocka
0 siblings, 1 reply; 5+ messages in thread
From: Mikulas Patocka @ 2021-01-21 15:09 UTC (permalink / raw)
To: Mike Snitzer, Milan Broz, Ondrej Kozina
Cc: Laura Abbott, dm-devel, security, Alasdair Kergon, Daniel Glöckner
Introduce the "fix_hmac" arguent. It improves security of 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
- mix a 16-byte salt to the mac, so that it can't be detected that two
volumes 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>
---
Documentation/admin-guide/device-mapper/dm-integrity.rst | 12 +
drivers/md/dm-integrity.c | 104 +++++++++++++--
2 files changed, 105 insertions(+), 11 deletions(-)
Index: linux-2.6/drivers/md/dm-integrity.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-integrity.c
+++ linux-2.6/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,7 +395,8 @@ static int dm_integrity_failed(struct dm
static bool dm_integrity_disable_recalculate(struct dm_integrity_c *ic)
{
- if ((ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
+ if (!(ic->sb->flags & cpu_to_le32(SB_FLAG_FIXED_HMAC)) &&
+ (ic->internal_hash_alg.key || ic->journal_mac_alg.key) &&
!ic->legacy_recalculate)
return true;
return false;
@@ -477,7 +484,9 @@ static void wraparound_section(struct dm
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 +496,58 @@ static void sb_set_version(struct dm_int
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 +559,28 @@ static int sync_rw_sb(struct dm_integrit
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 +797,32 @@ static void section_mac(struct dm_integr
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 *)§ion_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 +832,7 @@ static void section_mac(struct dm_integr
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 +845,7 @@ static void section_mac(struct dm_integr
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 +1648,12 @@ static void integrity_sector_checksum(st
goto failed;
}
+ 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 *)§or_le, sizeof sector_le);
if (unlikely(r < 0)) {
dm_integrity_io_error(ic, "crypto_shash_update", r);
@@ -3149,6 +3247,7 @@ static void dm_integrity_status(struct d
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 +3272,8 @@ static void dm_integrity_status(struct d
}
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 +3411,11 @@ static int initialize_superblock(struct
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 +3910,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4048,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4058,8 @@ static int dm_integrity_ctr(struct dm_ta
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 +4218,7 @@ static int dm_integrity_ctr(struct dm_ta
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 +4550,7 @@ static void dm_integrity_dtr(struct dm_t
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,
Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
===================================================================
--- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -186,6 +186,16 @@ 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 attaker 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,
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dm-devel] [PATCH v5 2/2] dm-integrity: introduce the "fix_hmac" argument
2021-01-21 15:09 ` [dm-devel] [PATCH v4 " Mikulas Patocka
@ 2021-01-23 20:26 ` Mikulas Patocka
2021-01-27 11:00 ` Milan Broz
0 siblings, 1 reply; 5+ messages in thread
From: Mikulas Patocka @ 2021-01-23 20:26 UTC (permalink / raw)
To: Mike Snitzer, Milan Broz, Ondrej Kozina
Cc: Laura Abbott, dm-devel, security, Alasdair Kergon, Daniel Glöckner
Introduce the "fix_hmac" arguent. It improves security of 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
- mix a 16-byte salt to the mac, so that it can't be detected that two
volumes 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>
---
Documentation/admin-guide/device-mapper/dm-integrity.rst | 12 +
drivers/md/dm-integrity.c | 104 +++++++++++++--
2 files changed, 105 insertions(+), 11 deletions(-)
Index: linux-2.6/drivers/md/dm-integrity.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-integrity.c
+++ linux-2.6/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
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
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_int
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_integrit
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_integr
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 *)§ion_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_integr
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_integr
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(st
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 *)§or_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 d
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 d
}
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
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_ta
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_ta
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_ta
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_ta
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_t
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,
Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
===================================================================
--- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -186,6 +186,16 @@ 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 attaker 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,
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dm-devel] [PATCH v5 2/2] dm-integrity: introduce the "fix_hmac" argument
2021-01-23 20:26 ` [dm-devel] [PATCH v5 " Mikulas Patocka
@ 2021-01-27 11:00 ` Milan Broz
0 siblings, 0 replies; 5+ messages in thread
From: Milan Broz @ 2021-01-27 11:00 UTC (permalink / raw)
To: Mike Snitzer
Cc: security, Daniel Glöckner, dm-devel, Mikulas Patocka,
Ondrej Kozina, Laura Abbott, Alasdair Kergon
On 23/01/2021 21:26, Mikulas Patocka wrote:
> Introduce the "fix_hmac" arguent. It improves security of 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
> - mix a 16-byte salt to the mac, so that it can't be detected that two
> volumes 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>
Hi Mike,
not that I like these fixes (I never expected standalone dm-integrity to be used with HMAC;
we have stacked AEAD modes in dmcrypt for cryptographically strong protection) but while it
is there, let's make it at least usable...
I addded support for these flags to integritysetup userspace, currently in merge request
https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/128
(stable minor version will be released with these changes soon).
Tested with Linus' mainline kernel + this v5 patch (most of changes below were based
on our discussion with Mikulas anyway).
If you want, add
Tested-by: Milan Broz <gmazyland@gmail.com>
Thanks,
m.
>
> ---
> Documentation/admin-guide/device-mapper/dm-integrity.rst | 12 +
> drivers/md/dm-integrity.c | 104 +++++++++++++--
> 2 files changed, 105 insertions(+), 11 deletions(-)
>
> Index: linux-2.6/drivers/md/dm-integrity.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-integrity.c
> +++ linux-2.6/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
>
> 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
>
> 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_int
> 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_integrit
> 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_integr
> 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 *)§ion_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_integr
>
> 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_integr
> 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(st
> 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 *)§or_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 d
> 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 d
> }
> 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
> 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_ta
> 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_ta
> 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_ta
> 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_ta
> 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_t
>
> 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,
> Index: linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
> ===================================================================
> --- linux-2.6.orig/Documentation/admin-guide/device-mapper/dm-integrity.rst
> +++ linux-2.6/Documentation/admin-guide/device-mapper/dm-integrity.rst
> @@ -186,6 +186,16 @@ 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 attaker 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,
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-27 11:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 19:00 [dm-devel] [PATCH v2 2/2] dm-integrity: introduce the "fix_hmac" argument Mikulas Patocka
2021-01-21 12:40 ` [dm-devel] [PATCH v3 " Mikulas Patocka
2021-01-21 15:09 ` [dm-devel] [PATCH v4 " Mikulas Patocka
2021-01-23 20:26 ` [dm-devel] [PATCH v5 " Mikulas Patocka
2021-01-27 11:00 ` Milan Broz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).