Re: [Qemu-devel] [PATCH v8 00/20] Convert QCow[2] to QCryptoBlock & add LUKS support

[Max Reitz]

On 2017-06-01 19:27, Daniel P. Berrange wrote:
> Previously posted:
> 
>  v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00201.html
>  v2: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05147.html
>  v3: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05671.html
>  v4: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02293.html
>  v5: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04653.html
>  v6: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04561.html
>  v7: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05818.html
> 
> This series is a continuation of previous work to support LUKS in
> QEMU. The existing merged code supports LUKS as a standalone
> driver which can be layered over/under any other QEMU block device
> driver. This works well when using LUKS over protocol drivers (file,
> rbd, iscsi, etc, etc), but has some downsides when combined with
> format drivers like qcow2.
> 
> If you layer LUKS under qcow2 (eg qcow2 -> luks -> file) then you
> cannot get any information about the qcow2 file without first
> decrypting it, as both the header and payload are encrypted.
> 
> If you layer LUKS over qcow2 (eg luks -> qcow2 -> file) then you
> cannot distinguish between a qcow2 file where the guest has done
> LUKS encryption from a qcow2 file which qemu has done encryption.
> More seriously, when encrypting sectors the guest virtual sector
> is used as the input for deriving the initialization vectors.
> When internal snapshots are used, this means that multiple sectors
> in the qcow2 file may be encrypted with the same initialization
> vector. This is a security weakness when combined with certain
> cryptographic modes.
> 
> Integrating LUKS natively into qcow2 allows us to combine the
> best aspects of both layering strategies above. In particular
> the header remains unecrypted, but initialization vectors are
> generated using physical sector numbers preserving security
> when snapshots are used. This is a change from previous postings
> of this work, where the IVs were (incorrectly) generated based
> on the virtual disk sector.
> 
> In a previous posting of this work, Fam had suggested that we
> do integration by layering luks over qcow2, but having QEMU
> block layer automatically create the luks driver above qcow2
> based on the qcow2 header crypt_method field. This is not
> possible though, because such a scheme would suffer from the
> problem of IVs being generated from the virtual disk sector
> instead of physical disk sector. So having LUKS specific
> code in the qcow2 block driver is unavoidable. In comparison
> to the previous posting though, the amount of code in qcow2.c
> has been reduced by allowing re-use of code from block/crypto.c
> for handling QemuOpts -> QAPI conversion. So extra lines of
> code in qcow2 to support LUKS is < 200.
> 
> I have also split the changes to qcow2 up into 2 patches. The
> first patch simply introduces use of the QCryptoBlock framework
> to qcow2 for the existing (deprecated) AES-CBC encryption method.
> The second patch wires up the LUKS support for qcow2. This makes
> it clearer which parts of the changes are related to plain code
> refactoring, vs enabling the new features. Specifically we can
> now see that the LUKS enablement in qcow2 has this footprint:

I'm trusting Eric and Berto, so I haven't looked at all patches in
depth. But I did have a look at all, and found only some minor stuff to
complain about. I didn't send an R-b for patches I did not find
anything, because (as I said) I didn't look at them sufficiently that
I'd give an R-b (which I didn't because Eric's and Berto's were already
present).

I'm just writing this so you know I'm not planning to write any
complaints for the patches I did not reply to. :-)

Max

signature.asc
Description: OpenPGP digital signature