On Mon, 07/02 23:07, Max Reitz wrote:
> VMDK performs a probing check in vmdk_co_create_opts() to prevent the
> user from assigning non-VMDK files as a backing file, because it only
> supports VMDK backing files. However, with the @backing runtime option,
> it is possible to assign arbitrary nodes as backing nodes, regardless of
> what the image header says. Therefore, VMDK may not just access backing
> nodes assuming they are VMDK nodes -- which it does, because it needs to
> compare the backing file's CID with the overlay's parentCID value, and
> naturally the backing file only has a CID when it's a VMDK file.
> Instead, it should report the CID of non-VMDK backing files not to match
> the overlay because clearly a non-present CID does not match.
>
> Without this change, vmdk_read_cid() reads from the backing file's
> bs->file, which may be NULL (in which case we get a segfault). Also, it
> interprets bs->opaque as a BDRVVmdkState and then reads from the
> .desc_offset field, which usually will just return some arbitrary value
> which then results in either garbage to be read, or bdrv_pread() to
> return an error, both of which result in a non-matching CID to be
> reported.
>
> (In a very unlikely case, we could read something that looks like a
> VMDK descriptor, and then get a CID which might actually match. But
> that is highly unlikely, and the only result would be that VMDK accepts
> the backing file which is not too bad (albeit unintentional).)
>
> ((And in theory, the seek to .desc_offset might leak data from another
> block driver's opaque object. But then again, the user should realize
> very quickly that a non-VMDK backing file does not work (because the
> read will very likely fail, due to the reasons given above), so this
> should not be exploitable.))
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> block/vmdk.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 84f8bbe480..a9d0084e36 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -333,6 +333,12 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
> if (!s->cid_checked && bs->backing) {
> BlockDriverState *p_bs = bs->backing->bs;
>
> + if (strcmp(p_bs->drv->format_name, "vmdk")) {
> + /* Backing file is not in vmdk format, so it does not have
> + * a CID, which makes the overlay's parent CID invalid */
> + return 0;
> + }
> +
Reviewed-by: Fam Zheng <f...@redhat.com>
> if (vmdk_read_cid(p_bs, 0, &cur_pcid) != 0) {
> /* read failure: report as not valid */
> return 0;
> --
> 2.17.1
>
