Re: [PATCH v7 00/14] LUKS: encryption slot management using amend interface

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2020-06-02 at 18:29 +0200, Max Reitz wrote:
> On 18.05.20 14:20, Maxim Levitsky wrote:
> > Hi!
> > Here is the updated series of my patches, incorporating all the feedback I received.
> 
> You asked me on IRC what to do to get this series to move forward;
> considering I don’t think there were objections from anyone but me on
> v6, there are no further (substantial) objections from anyone on v7, and
> I gave all feedback I had on v6, I don’t think there’s much you can do
> right now.  (Sorry for the delay, but, well, I was on PTO as you know.)
> 
> As usual, I’ll try to get around to see whether I can rebase your series
> myself (because Dan hinted at some conflicts), and whether I feel like
> my comments were appropriately addressed (which I have little doubt
> about).  That’s the plan.
Sorry in advance if I missed any feedback from you. That can happen,
I do make mistakes like anybody of us.


> 
> Note from a couple minutes later: From what I can see, the rebase
> conflicts don’t look too wild, but I don’t feel quite comfortable
> addressing all of them.  There’s a functional one I could address in
> qemu-img.c (patch 3), where we need to bump OPTION_FORCE from 269 to
> 276.  I could do that, but that’s not the only one, unfortunately.
Yes, that conflict is easy.

> 
> Patch 7 needs a bit more extensive modification: First, we need
> %s/bdrv_filter_default_perms/bdrv_default_perms/.  Second, this will
> still not compile, because the .bdrv_child_perm interface has changed.
> BdrvChildRole is now an integer, so we also need
> s/const BdrvChildRole \*/BdrvChildRole /.
> (That gives us the nice side effect of being able to align the second
> line of the bdrv_default_perms() parameters (called from
> block_crypto_child_perms()) on the opening parenthesis.)
I did this.

> 
> Third, it becomes really interesting.  With these changes, it would be
> wrong, because bdrv_default_perms() will then not use the permissions
> for a filter but for an image file with metadata – because that’s what
> the LUKS file child is.
> 
> But that’s actually a bug that’s already there (and that I introduced).
>  It makese sense to fix it in your series here, because to fix it we
> need a dedicated block_crypto_child_perms() function anyway.
> 
> So, well.  Do we want to cheat?  Just let block_crypto_child_perms()
> call bdrv_default_perms() with role=BDRV_CHILD_FILTERED?  That would be
> the previous behavior, but it would also be bad cheating.
> 
> The comment that exists (before patch 7) above
> bdrv_crypto_luks.bdrv_child_perm says that we just want to allow
> share-rw=on, and we could achieve that simply by force-sharing the WRITE
> permission after invoking bdrv_default_perms() with the actual role
> (which is BDRV_CHILD_IMAGE).
> 
> But what about the other stuff that sets
> bdrv_default_perms_for_storage() apart from bdrv_filter_default_perms()?
>  I.e., force-taking WRITE and RESIZE permissions if the file is
> writable; force-taking the CONSISTENT_READ permission because we need
> the metadata; and unsharing the RESIZE permission?
> 
> I think the best thing to do would be to call bdrv_default_perms() with
> the @role passed to block_crypto_child_perms() (i.e., BDRV_CHILD_IMAGE),
> and then relaxing the permissions, that is to share the WRITE and RESIZE
> persmissions, and to perhaps restore the WRITE and RESIZE permissions
> from what’s given to block_crypto_child_perms() (i.e., *nperm &= ~(WRITE
> > RESIZE); *nperm |= perm & (WRITE | RESIZE);), because LUKS doesn’t
> need them unless the image may actually be written.
> 
> (OTOH, force-taking CONSISTENT_READ seems entirely reasonable.)

Could we talk about this on IRC tomorrow? I don't fully understand you here,
and I sort of understood this stuff back when I learned it, but
that was long ago, and since we are talking about permissions here,
plus an necessary hack that luks now have to make, I would like
to understand exactly what I am doing.

Other than that, I rebased the series, and all iotests (with the permission
fix below) are passing.
I'll send the rebased version once we finish the permission thing.


Best regards,
	Maxim Levitsky


PS: This code which is roughtly based on your suggestions,
does pass my test write sharing test, but I don't have much confidence that it is correct:

For example, I don't think that resize permission is needed to be touched,
since resize of the 'luks' image shoudn't have any affect on encryption keys
(since luks image is basically the underlying file minus the header, decrypted,
and we don't really change the encryption key, but a encrypted keyslot which contains it).


    BlockCrypto *crypto = bs->opaque;

    bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared);
    /*
     * Ask for consistent read permission so that if
     * someone else tries to open this image with this permission
     * neither will be able to edit encryption keys, since
     * we will unshare that permission while trying to
     * update the encryption keys
     */
    if (!(bs->open_flags & BDRV_O_NO_IO)) {

        *nperm |= BLK_PERM_CONSISTENT_READ;

        *nshared |= (BLK_PERM_WRITE | BLK_PERM_RESIZE);
        *nperm &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
        *nperm |= perm & (BLK_PERM_WRITE | BLK_PERM_RESIZE);
    }
    /*
     * This driver doesn't modify LUKS metadata except
     * when updating the encryption slots.
     * Thus unlike a proper format driver we don't ask for
     * shared write/read permission. However we need it
     * when we are updating the keys, to ensure that only we
     * have access to the device.
     *
     * Encryption update will set the crypto->updating_keys
     * during that period and refresh permissions
     *
     */
    if (crypto->updating_keys) {
        /* need exclusive write access for header update */
        *nperm |= BLK_PERM_WRITE;
        /* unshare read and write permission */
        *nshared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE);
    }