On Sun, Aug 11, 2024 at 17:34:48 +0300, Nikolai Barybin via Devel wrote:
> - parse data-file metadata and init src->dataFileStore struct
> - chown data-file to allow qemu to open it
> - add data-file path to VM's cgroup and namespace
> - add data-file option to QEMU process command line
>
> Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> ---
> src/qemu/qemu_block.c | 45 +++++++++++++++++++++++++++++++
> src/qemu/qemu_cgroup.c | 3 +++
> src/qemu/qemu_namespace.c | 6 +++++
> src/storage_file/storage_source.c | 38 ++++++++++++++++++++++++--
> 4 files changed, 90 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index d6cdf521c4..38f4717d56 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1271,6 +1271,48 @@ qemuBlockStorageSourceGetFormatQcowGenericProps(virStorageSource *src,
> }
>
>
> +static int
> +qemuBlockStorageGetQcow2DataFileProps(virStorageSource *src,
> + virJSONValue *props)
> +{
> + g_autoptr(virJSONValue) dataFileDef = NULL;
> + virStorageType actualType;
> + g_autofree char *driverName = NULL;
> +
> + if (!src->dataFileRaw)
> + return 0;
> +
> + actualType = virStorageSourceGetActualType(src->dataFileStore);
> + if (actualType != VIR_STORAGE_TYPE_BLOCK &&
> + actualType != VIR_STORAGE_TYPE_FILE) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%1$s storage type is not supported for qcow2 data-file"),
> + NULLSTR(virStorageTypeToString(actualType)));
> + return -1;
This form of validation doesn't really belong to the code that actually
formats the JSON, and should be done earlier. But based on my comment
below it won't even be needed.
> + }
> +
> + if (virStorageSourceIsBlockLocal(src->dataFileStore))
> + driverName = g_strdup("host_device");
> + else
> + driverName = g_strdup("file");
> +
> + if (virJSONValueObjectAdd(&dataFileDef,
> + "s:driver", driverName,
> + "b:read-only", src->readonly,
> + "s:filename", src->dataFileRaw,
> + NULL) < 0)
> + return -1;
> +
> + if (virJSONValueObjectAdd(&props,
> + "A:data-file", &dataFileDef,
> + NULL) < 0)
> + return -1;
So you're using the inline definition rather than an explicit new
'-blockdev'. As libvirt switched to the full blockdev topology this is
not acceptable.
Also with using a proper blockdev you won't need to re-invent this code
and simply use the existing one. the 'data-file' field will be filled by
a reference to the node name.
> +
> + return 0;
> +}
[...]
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 23b7e6b4e8..84c749b567 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -223,6 +223,9 @@ qemuSetupImageCgroupInternal(virDomainObj *vm,
> qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0)
> return -1;
>
> + if (src->dataFileRaw && qemuSetupImagePathCgroup(vm, src->dataFileRaw, readonly) < 0)
> + return -1;
The 'dataFileRaw' field _must not_ be accessed by anything besides the
code which then re-probes the metadata of the backing image. Anything
else must use the 'src' bits.
> +
> return qemuSetupImagePathCgroup(vm, path, readonly);
> }
>
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index bbe3d5a1f7..32653443ec 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -290,6 +290,12 @@ qemuDomainSetupDisk(virStorageSource *src,
>
> if (targetPaths)
> *paths = g_slist_concat(g_slist_reverse(targetPaths), *paths);
> +
> + if (next->dataFileRaw) {
> + g_autofree char *datafile = NULL;
> + datafile = g_strdup(next->dataFileRaw);
> + *paths = g_slist_prepend(*paths, g_steal_pointer(&datafile));
Same here.
> + }
> }
>
> *paths = g_slist_prepend(*paths, g_steal_pointer(&tmpPath));
> diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
> index 2cfe3bc325..0ea11228d4 100644
> --- a/src/storage_file/storage_source.c
> +++ b/src/storage_file/storage_source.c
> @@ -1378,8 +1378,16 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src,
> &buf, &headerLen) < 0)
> return -1;
>
> - if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0)
> - return -1;
> + if (!parent->dataFileRaw) {
> + if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0)
> + return -1;
> + } else {
> + if (virStorageSourceInitAs(src, uid, gid) < 0)
> + return -1;
> + if (virStorageSourceChown(src, uid, gid) < 0)
> + return -1;
> + virStorageSourceDeinit(src);
The probing of images must not chown anything. NACK to this hunk. Per
commit message you seem to suggest that this is to allow opening the
image, but as stated this is wrong. You need to implement this properly
in the security drivers.
> + }
>
> /* If we probed the format we MUST ensure that nothing else than the current
> * image is considered for security labelling and/or recursion. */
> @@ -1417,6 +1425,32 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src,
>
> backingStore->id = depth;
> src->backingStore = g_steal_pointer(&backingStore);
> +
> + } else if (src->dataFileRaw) {
You can't do this as an else-if branch here.
A QCOW2 image can have all 4 combinations of 'backing file' and 'data
file' properties.
By doning an else-if you prevent adding a terminator if the image has a
data file.
But what's worse, if you'd have a qcow2 overlay image with a data file
you'd not even parse the data file.
> + if ((rv = virStorageSourceNewFromChild(src, src->dataFileRaw, &backingStore)) < 0)
> + return -1;
Do not reuse 'backingStore' it makes the code confusing.
> +
> + if (rv == 1)
> + return 0;
> +
> + if ((rv = virStorageSourceGetMetadataRecurse(backingStore, parent,
> + uid, gid,
> + report_broken,
> + max_depth, depth + 1)) < 0) {
IIRC the 'data file' doesn't have a format thus can't even have a
backing image. Recursing here is pointless and also wrong (although IIUC
doesn't really break security).
Either way, since the top image is a qcow2 image with proper format,
qemu will allow writing arbitrary data including a qcow2 header into the
image which would be written verbatim into the data file.
Attempting to probe the contents of a data file, could then make us
parse a mailcious header.
Basically the data file must be treated as 'raw', at least when probed
from the image.
> + if (!report_broken)
> + return 0;
> +
> + if (rv == -2) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("format of data file image '%1$s' of image '%2$s' was not specified in the image metadata (See https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
> + src->dataFileRaw, NULLSTR(src->path));
> + }
The linked document definitely doesn't say anything about the data image
and also as per above this makes no sense as the data file can't have a
format which would allow backing chains.
> +
> + return -1;
> + }
> +
> + backingStore->id = depth;
> + src->dataFileStore = g_steal_pointer(&backingStore);
> } else {
> /* add terminator */
> src->backingStore = virStorageSourceNew();
> --
> 2.43.5
>
