Re: [Qemu-devel] [PATCH v2 05/17] crypto: add support for anti-forensic split algorithm

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Feb 04, 2016 at 04:26:42PM -0700, Eric Blake wrote:
> On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> > +++ b/crypto/afsplit.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * QEMU Crypto anti forensic information splitter
> > + *
> > + * Copyright (c) 2015-2016 Red Hat, Inc.
> > + *
> > + * Derived from cryptsetup package lib/lusk1/af.c
> > + *
> > + * Copyright (C) 2004, Clemens Fruhwirth <clemens@endorphin.org>
> > + * Copyright (C) 2009-2012, Red Hat, Inc. All rights reserved.
> > + *
> 
> > +
> > +static void qcrypto_afsplit_xor(size_t blocklen,
> > +                                const uint8_t *in1,
> > +                                const uint8_t *in2,
> > +                                uint8_t *out)
> > +{
> > +    size_t i;
> > +    for (i = 0; i < blocklen; i++) {
> > +        out[i] = in1[i] ^ in2[i];
> > +    }
> 
> Could be made faster using larger data types, if we know that things are
> properly aligned.  I'm not sure if the compiler can automatically
> optimize this.  I'm also not sure if this is ever on a hot path where it
> matters.

It is used when opening an encrypted volume, or when formatting
an encrypted volume. Both those codepaths are made intentionally
very slow by the pbkdf algorithm, so slowness in the afsplit code
is not going to be noticable overall.

> > +static int qcrypto_afsplit_hash(QCryptoHashAlgorithm hash,
> > +                                size_t blocklen,
> > +                                uint8_t *block,
> > +                                Error **errp)
> > +{
> > +    size_t digestlen = qcrypto_hash_digest_len(hash);

[snip]

> > +        if (qcrypto_hash_bytesv(hash,
> > +                                in,
> > +                                G_N_ELEMENTS(in),
> > +                                &out, &outlen,
> > +                                errp) < 0) {
> > +            return -1;
> > +        }
> > +
> > +        if (outlen != digestlen) {
> > +            error_setg(errp, "Hash output %zu not %zu",
> > +                       outlen, digestlen);
> > +            g_free(out);
> 
> Is this error even possible, or can it be an assert?

Yes it can assert, since it should be impossible.

> > +int qcrypto_afsplit_encode(QCryptoHashAlgorithm hash,
> > +                           size_t blocklen,
> > +                           uint32_t stripes,
> > +                           const uint8_t *in,
> > +                           uint8_t *out,
> > +                           Error **errp)
> > +{
> > +    uint8_t *block = g_new0(uint8_t, blocklen);
> > +    size_t i;
> 
> > 
> > +
> > +        qcrypto_afsplit_xor(blocklen,
> > +                            out + (i * blocklen),
> > +                            block,
> > +                            block);
> 
> Should we ensure no overflow in the multiplication here (or rather, that
> the input blocklen * stripes is reasonable on input)?

The use case for this code is splitting an encryption key. So we're
talking blocklen in the 100's and stripes i the 1000's. So we're
not going to be anywhere near possible overflow.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|