dm integrity: conditionally disable "recalculate" feature

Otherwise a malicious user could (ab)use the "recalculate" feature
that makes dm-integrity calculate the checksums in the background
while the device is already usable. When the system restarts before all
checksums have been calculated, the calculation continues where it was
interrupted even if the recalculate feature is not requested the next
time the dm device is set up.

Disable recalculating if we use internal_hash or journal_hash with a
key (e.g. HMAC) and we don't have the "legacy_recalculate" flag.

This may break activation of a volume, created by an older kernel,
that is not yet fully recalculated -- if this happens, the user should
add the "legacy_recalculate" flag to constructor parameters.

Cc: stable@vger.kernel.org
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Daniel Glockner <dg@emlix.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 4e6f504474ac2d48bab8e3df7260fc441e033ba4..2cc5488acbd97a1efda5152a4c6e80299246d2a2 100644 (file)
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -177,14 +177,20 @@ bitmap_flush_interval:number
        The bitmap flush interval in milliseconds. The metadata buffers
        are synchronized when this interval expires.
 
+allow_discards
+       Allow block discard requests (a.k.a. TRIM) for the integrity device.
+       Discards are only allowed to devices using internal hash.
+
 fix_padding
        Use a smaller padding of the tag area that is more
        space-efficient. If this option is not present, large padding is
        used - that is for compatibility with older kernels.
 
-allow_discards
-       Allow block discard requests (a.k.a. TRIM) for the integrity device.
-       Discards are only allowed to devices using internal hash.
+legacy_recalculate
+       Allow recalculating of volumes with HMAC keys. This is disabled by
+       default for security reasons - an attacker could modify the volume,
+       set recalc_sector to zero, and the kernel would not detect the
+       modification.
 
 The journal mode (D/J), buffer_sectors, journal_watermark, commit_time and
 allow_discards can be changed when reloading the target (load an inactive

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index cce203adcf777665902911848f24202fd712ffbf..b64fede032dc5314eda9ccf353738ef7617fb89a 100644 (file)
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -257,8 +257,9 @@ struct dm_integrity_c {
        bool journal_uptodate;
        bool just_formatted;
        bool recalculate_flag;
-       bool fix_padding;
        bool discard;
+       bool fix_padding;
+       bool legacy_recalculate;
 
        struct alg_spec internal_hash_alg;
        struct alg_spec journal_crypt_alg;
@@ -386,6 +387,14 @@ static int dm_integrity_failed(struct dm_integrity_c *ic)
        return READ_ONCE(ic->failed);
 }
 
+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)
+               return true;
+       return false;
+}
+
 static commit_id_t dm_integrity_commit_id(struct dm_integrity_c *ic, unsigned i,
                                          unsigned j, unsigned char seq)
 {
@@ -3140,6 +3149,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->legacy_recalculate;
                DMEMIT("%s %llu %u %c %u", ic->dev->name, ic->start,
                       ic->tag_size, ic->mode, arg_count);
                if (ic->meta_dev)
@@ -3163,6 +3173,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->legacy_recalculate)
+                       DMEMIT(" legacy_recalculate");
 
 #define EMIT_ALG(a, n)                                                 \
                do {                                                    \
@@ -3792,7 +3804,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, 15, "Invalid number of feature args"},
+               {0, 16, "Invalid number of feature args"},
        };
        unsigned journal_sectors, interleave_sectors, buffer_sectors, journal_watermark, sync_msec;
        bool should_write_sb;
@@ -3940,6 +3952,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, "legacy_recalculate")) {
+                       ic->legacy_recalculate = true;
                } else {
                        r = -EINVAL;
                        ti->error = "Invalid argument";
@@ -4243,6 +4257,14 @@ try_smaller_buffer:
                }
        }
 
+       if (ic->sb->flags & cpu_to_le32(SB_FLAG_RECALCULATING) &&
+           le64_to_cpu(ic->sb->recalc_sector) < ic->provided_data_sectors &&
+           dm_integrity_disable_recalculate(ic)) {
+               ti->error = "Recalculating with HMAC is disabled for security reasons - if you really need it, use the argument \"legacy_recalculate\"";
+               r = -EOPNOTSUPP;
+               goto bad;
+       }
+
        ic->bufio = dm_bufio_client_create(ic->meta_dev ? ic->meta_dev->bdev : ic->dev->bdev,
                        1U << (SECTOR_SHIFT + ic->log2_buffer_sectors), 1, 0, NULL, NULL);
        if (IS_ERR(ic->bufio)) {