On Mon, Jan 16, 2017 at 09:25:55PM +0100, Max Reitz wrote:
> On 03.01.2017 19:27, Daniel P. Berrange wrote:
> > Instead of requiring separate input/output buffers for
> > encrypting data, change encrypt_sectors() to assume
> > use of a single buffer, encrypting in place. One current
> > caller all uses the same buffer for input/output already
>
> -all?
>
> > and the other two callers are easily converted todo so.
>
> s/todo/to do/
>
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> > ---
> > block/qcow.c | 36 +++++++++++-------------------------
> > 1 file changed, 11 insertions(+), 25 deletions(-)
> >
> > diff --git a/block/qcow.c b/block/qcow.c
> > index 8133fda..bc9fa2f 100644
> > --- a/block/qcow.c
> > +++ b/block/qcow.c
> > @@ -310,11 +310,10 @@ static int qcow_set_key(BlockDriverState *bs, const
> > char *key)
> > }
> >
> > /* The crypt function is compatible with the linux cryptoloop
> > - algorithm for < 4 GB images. NOTE: out_buf == in_buf is
> > - supported */
> > + algorithm for < 4 GB images. */
> > static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> > - uint8_t *out_buf, const uint8_t *in_buf,
> > - int nb_sectors, bool enc, Error **errp)
> > + uint8_t *buf, int nb_sectors, bool enc,
> > + Error **errp)
> > {
> > union {
> > uint64_t ll[2];
> > @@ -333,14 +332,12 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t
> > sector_num,
> > }
> > if (enc) {
> > ret = qcrypto_cipher_encrypt(s->cipher,
> > - in_buf,
> > - out_buf,
> > + buf, buf,
> > 512,
> > errp);
> > } else {
> > ret = qcrypto_cipher_decrypt(s->cipher,
> > - in_buf,
> > - out_buf,
> > + buf, buf,
> > 512,
> > errp);
> > }
> > @@ -348,8 +345,7 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t
> > sector_num,
> > return -1;
> > }
> > sector_num++;
> > - in_buf += 512;
> > - out_buf += 512;
> > + buf += 512;
> > }
> > return 0;
> > }
> > @@ -469,13 +465,12 @@ static uint64_t get_cluster_offset(BlockDriverState
> > *bs,
> > uint64_t start_sect;
> > assert(s->cipher);
> > start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> > - memset(s->cluster_data + 512, 0x00, 512);
> > + memset(s->cluster_data, 0x00, 512);
> > for(i = 0; i < s->cluster_sectors; i++) {
> > if (i < n_start || i >= n_end) {
> > Error *err = NULL;
> > if (encrypt_sectors(s, start_sect + i,
> > - s->cluster_data,
> > - s->cluster_data + 512, 1,
> > + s->cluster_data, 1,
> > true, &err) < 0) {
> > error_free(err);
> > errno = EIO;
>
> After the first iteration of the surrounding for () loop,
> s->cluster_data is unlikely to still be filled with zeros -- but I
> suspect the code intended to always write encrypted zeros.
Yep, we must push the memset down a level.
> > @@ -653,7 +648,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState
> > *bs, int64_t sector_num,
> > }
> > if (bs->encrypted) {
> > assert(s->cipher);
> > - if (encrypt_sectors(s, sector_num, buf, buf,
> > + if (encrypt_sectors(s, sector_num, buf,
> > n, false, &err) < 0) {
> > goto fail;
> > }
> > @@ -688,9 +683,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState
> > *bs, int64_t sector_num,
> > BDRVQcowState *s = bs->opaque;
> > int index_in_cluster;
> > uint64_t cluster_offset;
> > - const uint8_t *src_buf;
> > int ret = 0, n;
> > - uint8_t *cluster_data = NULL;
> > struct iovec hd_iov;
> > QEMUIOVector hd_qiov;
> > uint8_t *buf;
> > @@ -728,21 +721,15 @@ static coroutine_fn int
> > qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
> > if (bs->encrypted) {
> > Error *err = NULL;
> > assert(s->cipher);
> > - if (!cluster_data) {
> > - cluster_data = g_malloc0(s->cluster_size);
> > - }
> > - if (encrypt_sectors(s, sector_num, cluster_data, buf,
> > + if (encrypt_sectors(s, sector_num, buf,
>
> If qiov->niov == 1, buf is not copied from the I/O vector but just the
> I/O vector base itself. Then, this will modify the data pointed to by
> that vector. I don't think that is a good idea.
Opps, yes, that is a bad idea. I'll force copying when bs->encrypted == true,
regardless of qiov->niov value.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
