[v6,11/11] blksnap: prevents using devices with data integrity or inline encryption

  From: Sergei Shtepa <sergei.shtepa@veeam.com>

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. To implement secure
compatibility with hardware inline encrypted devices will require
discussion of algorithms and restrictions. For example, a restriction
on the location of the difference storage only in virtual memory might
help. Currently, there is no need for compatibility of the blksnap
module and hardware inline encryption.

I see no obstacles to ensuring the compatibility of the blksnap module
and block devices with data integrity. However, this functionality was
not planned or tested. Perhaps in the future this compatibility can be
implemented.

Theoretically possible that the block device was added to the snapshot
before crypto_profile and integrity.profile were initialized.
Checking the values of bi_crypt_context and bi_integrity ensures that
the blksnap will not perform any actions with I/O units with which it
is not compatible.

Reported-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
---
 drivers/block/blksnap/snapshot.c | 17 +++++++++++++++++
 drivers/block/blksnap/tracker.c  | 14 ++++++++++++++
 2 files changed, 31 insertions(+)
  

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.