On Tue, Jan 03, 2012 at 05:12:47PM +0100, Michal Privoznik wrote: > Currently, we support only filling a volume with zeroes on wiping. > However, it is not enough as data might still be readable by > experienced and equipped attacker. Many technical papers have been > written, therefore we should support other wiping algorithms. > --- > Okay, this is not as complete as I'd like it. Wiping could take ages, > therefore we *want* it to be asynchronous. But that would mean: > > 1) Create new API to answer: "Are we done yet?" > > 2) Create event emitting system - like we have for domains > > 3) Combination of previous two > > In fact, I've started writing events, but it turned out to be > huge and I am not even in a half yet. I need to find a way of > re-using event code we already have. But thats another day and > another patch set. > > For those interested, take a look at Section 4.1.11 of DSS_PCI > requirements at [1]. > > 1: https://www.pcisecuritystandards.org/documents/Virtualization_InfoSupp_v2.pdf > > configure.ac | 26 ++++++++++- > include/libvirt/libvirt.h.in | 24 ++++++++++ > src/libvirt.c | 6 ++- > src/storage/storage_driver.c | 103 ++++++++++++++++++++++++++++++++++------- > tools/virsh.c | 41 +++++++++++++++- > tools/virsh.pod | 25 ++++++++++- > 6 files changed, 201 insertions(+), 24 deletions(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index ad6fcce..caa9127 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -2118,6 +2118,30 @@ typedef enum { > VIR_STORAGE_VOL_DELETE_ZEROED = 1, /* Clear all data to zeros (slow) */ > } virStorageVolDeleteFlags; > > +typedef enum { > + /* Keep this place for async flag. Currently, > + * it is not implemented because of lack of > + * events support within storage driver. > + * VIR_STORAGE_VOL_WIPE_ASYNC = (1 << 0), > + */ Just remove this reservation entirely - if we ever need it, there's no reason why we can't put it in as (1 << 9), or whatever other flag is next available. > + VIR_STORAGE_VOL_WIPE_ALG_NNSA = (1 << 1), /* 4-pass NNSA Policy Letter > + NAP-14.1-C (XVI-8) */ > + VIR_STORAGE_VOL_WIPE_ALG_DOD = (1 << 2), /* 4-pass DoD 5220.22-M section > + 8-306 procedure */ > + VIR_STORAGE_VOL_WIPE_ALG_BSI = (1 << 3), /* 9-pass method recommended by the > + German Center of Security in > + Information Technologies */ > + VIR_STORAGE_VOL_WIPE_ALG_GUTMANN = (1 << 4), /* The canonical 35-pass sequence */ > + VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER = (1 << 5), /* 7-pass method described by > + Bruce Schneier in "Applied > + Cryptography" (1996) */ > + VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7 = (1 << 6), /* 7-pass random */ > + > + VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33 = (1 << 7), /* 33-pass random */ > + > + VIR_STORAGE_VOL_WIPE_ALG_RANDOM = (1 << 8), /* 1-pass random */ > +} virStorageVolWipeFlags; > + Hmm, I always feel slightly bad, when we use bit flags to encode options that are to be treated as all mutually exclusive. ie, this scheme allows us to pass in multiple algorithms at once, but we only actually want 1 algorithm. Should we instead create a new API for this int virStorageVolWipePattern(virStorageVolPtr vol, unsigned int algorithm, unsigned int flags); And define a normal enum typedef enum { VIR_STORAGE_VOL_WIPE_ALG_ZEROS = 1, VIR_STORAGE_VOL_WIPE_ALG_NNSA = 2, } virStorageVolWipeAlgorithm; > typedef struct _virStorageVolInfo virStorageVolInfo; > > struct _virStorageVolInfo { > diff --git a/src/libvirt.c b/src/libvirt.c > index feb3ca6..c3b3665 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -12557,7 +12557,11 @@ error: > * @vol: pointer to storage volume > * @flags: future flags, use 0 for now > * > - * Ensure data previously on a volume is not accessible to future reads > + * Ensure data previously on a volume is not accessible to future reads. > + * When specifying wipe algorithm only one algorithm flag is allowed. > + * Therefore if one wants to wipe a volume with say NNSA and DOD algorithms, > + * he/she needs two subsequent calls. First with _ALG_NNSA flag, second > + * with _ALG_DOD. eg, this comment would not be needed if we encoded the algorithm as a separate enum parameter. > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index 8c2d6e1..f3a084a 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -1801,14 +1801,16 @@ out: > > > static int > -storageVolumeWipeInternal(virStorageVolDefPtr def) > +storageVolumeWipeInternal(virStorageVolDefPtr def, > + unsigned int flags) > { > int ret = -1, fd = -1; > struct stat st; > char *writebuf = NULL; > size_t bytes_wiped = 0; > + virCommandPtr cmd = NULL; > > - VIR_DEBUG("Wiping volume with path '%s'", def->target.path); > + VIR_DEBUG("Wiping volume with path '%s' flags %x", def->target.path, flags); > > fd = open(def->target.path, O_RDWR); > if (fd == -1) { > @@ -1825,29 +1827,62 @@ storageVolumeWipeInternal(virStorageVolDefPtr def) > goto out; > } > > - if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { > - ret = storageVolumeZeroSparseFile(def, st.st_size, fd); > - } else { > - > - if (VIR_ALLOC_N(writebuf, st.st_blksize) != 0) { > - virReportOOMError(); > + if (flags) { > + cmd = virCommandNew(SCRUB); > + virCommandAddArg(cmd, "-f"); > + if (flags &VIR_STORAGE_VOL_WIPE_ALG_NNSA) { > + virCommandAddArgList(cmd, "-p", "nnsa", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_DOD) { > + virCommandAddArgList(cmd, "-p", "dod", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_BSI) { > + virCommandAddArgList(cmd, "-p", "bsi", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_GUTMANN) { > + virCommandAddArgList(cmd, "-p", "gutmann", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER) { > + virCommandAddArgList(cmd, "-p", "schneier", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7) { > + virCommandAddArgList(cmd, "-p", "pfitzner7", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33) { > + virCommandAddArgList(cmd, "-p", "pfitzner33", NULL); > + } else if (flags & VIR_STORAGE_VOL_WIPE_ALG_RANDOM) { > + virCommandAddArgList(cmd, "-p", "random", NULL); Then, we could just use a VIR_ENUM_DECL/IMPL lookup to avoid this big if/elseif' block. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list