| Message ID | 20231124165933.27580-12-sergei.shtepa@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blksnap - block devices snapshots module | expand |
On Fri, Nov 24, 2023 at 05:59:33PM +0100, Sergei Shtepa wrote: > There is an opinion that the use of the blksnap module may violate the > security of encrypted data. The difference storage file may be located > on an unreliable disk or even network storage. I think this misses the point slightly. The main problem is that blksnap writes data in plaintext that is supposed to be encrypted, as indicated by the bio having an encryption context. That's just what it does, at least based on the last patchset; it's not just "an opinion". See https://lore.kernel.org/linux-block/20a5802d-424d-588a-c497-1d1236c52880@veeam.com/ > +#ifdef CONFIG_BLK_INLINE_ENCRYPTION > + if (bio->bi_crypt_context) { > + pr_err_once("Hardware inline encryption is not supported\n"); > + diff_area_set_corrupted(tracker->diff_area, -EPERM); > + return false; > + } > +#endif The error message for ->bi_crypt_context being set should say "Inline encryption", not "Hardware inline encryption". The submitter of the bio may have intended to use blk-crypto-fallback. Anyway, this patch is better than ignoring the problem. It's worth noting, though, that this patch does not prevent blksnap from being set up on a block device on which blk-crypto-fallback is already being used (or will be used). When that happens, I/O will suddenly start failing. For usability reasons, ideally that would be prevented somehow. - Eric
On 11/27/23 23:47, Eric Biggers wrote: > On Fri, Nov 24, 2023 at 05:59:33PM +0100, Sergei Shtepa wrote: >> There is an opinion that the use of the blksnap module may violate the >> security of encrypted data. The difference storage file may be located >> on an unreliable disk or even network storage. > I think this misses the point slightly. The main problem is that blksnap writes > data in plaintext that is supposed to be encrypted, as indicated by the bio > having an encryption context. That's just what it does, at least based on the > last patchset; it's not just "an opinion". See > https://lore.kernel.org/linux-block/20a5802d-424d-588a-c497-1d1236c52880@veeam.com/ Thanks Eric. Perhaps I formulated the thought inaccurately. The point is that blksnap should not be compatible with blk-crypto. Changes in version 6 do not allow to take a snapshot with a device on which the encryption context is detected. Additionally, protection is implemented in the bio handling code. For bio with bi_crypt_context, the COW algorithm is not executed. > >> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION >> + if (bio->bi_crypt_context) { >> + pr_err_once("Hardware inline encryption is not supported\n"); >> + diff_area_set_corrupted(tracker->diff_area, -EPERM); >> + return false; >> + } >> +#endif > The error message for ->bi_crypt_context being set should say > "Inline encryption", not "Hardware inline encryption". The submitter of the bio > may have intended to use blk-crypto-fallback. I was looking at the blk-crypto-fallback code. I tested the work in this case. Encryption is performed before the bio gets to the block layer. So, the filter receives cloned bios with already encrypted data. Therefore, the text of the message is correct. But I haven't tested the code on a device where hardware inline encryption is available. I would be glad if anyone could help with this. > > Anyway, this patch is better than ignoring the problem. It's worth noting, > though, that this patch does not prevent blksnap from being set up on a block > device on which blk-crypto-fallback is already being used (or will be used). > When that happens, I/O will suddenly start failing. For usability reasons, > ideally that would be prevented somehow. I didn't observe any failures during testing. It's just that the snapshot image shows files with encrypted names and data. Backup in this case is useless. Unfortunately, there is no way to detect a blk-crypto-fallback on the block device filter level. Maybe my tests aren't enough. The next step I think would be great to add new tests to xfstests.
On Tue, Nov 28, 2023 at 12:00:17PM +0100, Sergei Shtepa wrote: > But I haven't tested the code on a device where hardware inline encryption is > available. I would be glad if anyone could help with this. > > > > Anyway, this patch is better than ignoring the problem. It's worth noting, > > though, that this patch does not prevent blksnap from being set up on a block > > device on which blk-crypto-fallback is already being used (or will be used). > > When that happens, I/O will suddenly start failing. For usability reasons, > > ideally that would be prevented somehow. > > I didn't observe any failures during testing. It's just that the snapshot > image shows files with encrypted names and data. Backup in this case is > useless. Unfortunately, there is no way to detect a blk-crypto-fallback on > the block device filter level. Huh, I thought that this patch is supposed to exclude blk-crypto-fallback too. __submit_bio() calls bio->bi_bdev->bd_filter->ops->submit_bio(bio) before blk_crypto_bio_prep(), so doesn't your check of ->bi_crypt_context exclude blk-crypto-fallback? I think you're right that it might actually be fine to use blksnap with blk-crypto-fallback, provided that the encryption is done first. I would like to see a proper explanation of that, though. And we still have this patch which claims that it doesn't work, which is confusing. - Eric
On 11/28/23 18:18, Eric Biggers wrote: > On Tue, Nov 28, 2023 at 12:00:17PM +0100, Sergei Shtepa wrote: >> But I haven't tested the code on a device where hardware inline encryption is >> available. I would be glad if anyone could help with this. >>> Anyway, this patch is better than ignoring the problem. It's worth noting, >>> though, that this patch does not prevent blksnap from being set up on a block >>> device on which blk-crypto-fallback is already being used (or will be used). >>> When that happens, I/O will suddenly start failing. For usability reasons, >>> ideally that would be prevented somehow. >> I didn't observe any failures during testing. It's just that the snapshot >> image shows files with encrypted names and data. Backup in this case is >> useless. Unfortunately, there is no way to detect a blk-crypto-fallback on >> the block device filter level. > Huh, I thought that this patch is supposed to exclude blk-crypto-fallback too. > __submit_bio() calls bio->bi_bdev->bd_filter->ops->submit_bio(bio) before > blk_crypto_bio_prep(), so doesn't your check of ->bi_crypt_context exclude > blk-crypto-fallback? Thank you, Eric. You're right. The filter handle unencrypted data when using blk-crypto-fallback. Indeed, the I/O unit has an encryption context. And yes, the word "Hardware" is not necessary. - pr_err_once("Hardware inline encryption is not supported\n"); + pr_err_once("Inline encryption is not supported\n"); > > I think you're right that it might actually be fine to use blksnap with > blk-crypto-fallback, provided that the encryption is done first. I would like > to see a proper explanation of that, though. And we still have this patch which > claims that it doesn't work, which is confusing. I found a bug in my test. I was let down by the cache. I redid the test and posted it. Link: https://github.com/veeam/blksnap/blob/stable-v2.0/tests/8000-inline-encryption.sh When the bi_crypt_context is detected in the write I/O unit, the snapshot image is marked as corrupted. The COW algorithm is not executed. The blksnap code does not allow data leakage. For a disk with hardware encryption, a block device cannot be added to the snapshot since the encryption context for the disk will be detected for it. Unfortunately, it is impossible to detect the presence of a blk-crypto-fallback when adding a block device to the snapshot. So, I think that the patch fully ensures the confidentiality of data when using inline encryption. However, it does not allow to perform a backup for this case. If we make a filter handling point in the __submit_bio() function after calling blk_crypto_bio_prep(), then this will not change the situation for the case of hardware encryption. But the filter will never know what the blk-crypto-fallback is being used. I have no opinion whether it will be better.
diff --git a/drivers/block/blksnap/snapshot.c b/drivers/block/blksnap/snapshot.c index 21d94f12b5fc..a7675fdcf359 100644 --- a/drivers/block/blksnap/snapshot.c +++ b/drivers/block/blksnap/snapshot.c @@ -149,6 +149,23 @@ int snapshot_add_device(const uuid_t *id, struct tracker *tracker) int ret = 0; struct snapshot *snapshot = NULL; +#ifdef CONFIG_BLK_DEV_INTEGRITY + if (tracker->orig_bdev->bd_disk->queue->integrity.profile) { + pr_err("Blksnap is not compatible with data integrity\n"); + ret = -EPERM; + goto out_up; + } else + pr_debug("Data integrity not found\n"); +#endif + +#ifdef CONFIG_BLK_INLINE_ENCRYPTION + if (tracker->orig_bdev->bd_disk->queue->crypto_profile) { + pr_err("Blksnap is not compatible with hardware inline encryption\n"); + ret = -EPERM; + goto out_up; + } else + pr_debug("Inline encryption not found\n"); +#endif snapshot = snapshot_get_by_id(id); if (!snapshot) return -ESRCH; diff --git a/drivers/block/blksnap/tracker.c b/drivers/block/blksnap/tracker.c index 2b8978a2f42e..b38ead9afa69 100644 --- a/drivers/block/blksnap/tracker.c +++ b/drivers/block/blksnap/tracker.c @@ -57,6 +57,20 @@ static bool tracker_submit_bio(struct bio *bio) if (diff_area_is_corrupted(tracker->diff_area)) return false; +#ifdef CONFIG_BLK_INLINE_ENCRYPTION + if (bio->bi_crypt_context) { + pr_err_once("Hardware inline encryption is not supported\n"); + diff_area_set_corrupted(tracker->diff_area, -EPERM); + return false; + } +#endif +#ifdef CONFIG_BLK_DEV_INTEGRITY + if (bio->bi_integrity) { + pr_err_once("Data integrity is not supported\n"); + diff_area_set_corrupted(tracker->diff_area, -EPERM); + return false; + } +#endif return diff_area_cow(bio, tracker->diff_area, ©_iter); }