handle_x86_pv_info() has a subtle bug. It uses an 'else if' chain with a
clause in the middle which doesn't exit unconditionally. In practice, this
means that when restoring a 32bit PV guest, later sanity checks are skipped.
Rework the logic a little to be simpler. There are exactly two valid
combinations of fields in X86_PV_INFO, so factor this out and check them all
in one go, before making adjustments to the current domain.
Once adjustments have been completed successfully, sanity check the result
against the X86_PV_INFO settings in one go, rather than piecewise.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
tools/libxc/xc_sr_restore_x86_pv.c | 69 ++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 28 deletions(-)
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index a2dbf85157..9e9ff32d47 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -582,6 +582,21 @@ static int update_guest_p2m(struct xc_sr_context *ctx)
}
/*
+ * The valid width/pt_levels values in X86_PV_INFO are inextricably linked.
+ * Cross-check the legitimate combinations.
+ */
+static bool valid_x86_pv_info_combination(
+ const struct xc_sr_rec_x86_pv_info *info)
+{
+ switch ( info->guest_width )
+ {
+ case 4: return info->pt_levels == 3;
+ case 8: return info->pt_levels == 4;
+ default: return false;
+ }
+}
+
+/*
* Process an X86_PV_INFO record.
*/
static int handle_x86_pv_info(struct xc_sr_context *ctx,
@@ -602,29 +617,31 @@ static int handle_x86_pv_info(struct xc_sr_context *ctx,
rec->length, sizeof(*info));
return -1;
}
- else if ( info->guest_width != 4 &&
- info->guest_width != 8 )
+
+ if ( !valid_x86_pv_info_combination(info) )
{
- ERROR("Unexpected guest width %u, Expected 4 or 8",
- info->guest_width);
+ ERROR("Invalid X86_PV_INFO combination: width %u, pt_levels %u",
+ info->guest_width, info->pt_levels);
return -1;
}
- else if ( info->guest_width != ctx->x86_pv.width )
+
+ /*
+ * PV domains default to native width. For an incomming compat domain, we
+ * will typically be the first entity to inform Xen.
+ */
+ if ( info->guest_width != ctx->x86_pv.width )
{
- int rc;
- struct xen_domctl domctl;
-
- /* Try to set address size, domain is always created 64 bit. */
- memset(&domctl, 0, sizeof(domctl));
- domctl.domain = ctx->domid;
- domctl.cmd = XEN_DOMCTL_set_address_size;
- domctl.u.address_size.size = info->guest_width * 8;
- rc = do_domctl(xch, &domctl);
+ struct xen_domctl domctl = {
+ .domain = ctx->domid,
+ .cmd = XEN_DOMCTL_set_address_size,
+ .u.address_size.size = info->guest_width * 8,
+ };
+ int rc = do_domctl(xch, &domctl);
+
if ( rc != 0 )
{
- ERROR("Width of guest in stream (%u"
- " bits) differs with existing domain (%u bits)",
- info->guest_width * 8, ctx->x86_pv.width * 8);
+ ERROR("Failed to update d%d address size to %u",
+ ctx->domid, info->guest_width * 8);
return -1;
}
@@ -636,18 +653,14 @@ static int handle_x86_pv_info(struct xc_sr_context *ctx,
return -1;
}
}
- else if ( info->pt_levels != 3 &&
- info->pt_levels != 4 )
- {
- ERROR("Unexpected guest levels %u, Expected 3 or 4",
- info->pt_levels);
- return -1;
- }
- else if ( info->pt_levels != ctx->x86_pv.levels )
+
+ /* Sanity check (possibly new) domain settings. */
+ if ( (info->guest_width != ctx->x86_pv.width) ||
+ (info->pt_levels != ctx->x86_pv.levels) )
{
- ERROR("Levels of guest in stream (%u"
- ") differs with existing domain (%u)",
- info->pt_levels, ctx->x86_pv.levels);
+ ERROR("X86_PV_INFO width/pt_levels settings %u/%u mismatch with d%d %u/%u",
+ info->guest_width, info->pt_levels, ctx->domid,
+ ctx->x86_pv.width, ctx->x86_pv.levels);
return -1;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develAndrew Cooper writes ("[PATCH] libxc/restore: Fix data auditing in handle_x86_pv_info()"):
> handle_x86_pv_info() has a subtle bug. It uses an 'else if' chain with a
> clause in the middle which doesn't exit unconditionally. In practice, this
> means that when restoring a 32bit PV guest, later sanity checks are skipped.
>
> Rework the logic a little to be simpler. There are exactly two valid
> combinations of fields in X86_PV_INFO, so factor this out and check them all
> in one go, before making adjustments to the current domain.
>
> Once adjustments have been completed successfully, sanity check the result
> against the X86_PV_INFO settings in one go, rather than piecewise.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
None of these are buggy, but the resulting code is more robust.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
tools/libxc/xc_dom_core.c | 3 ++-
tools/libxc/xc_misc.c | 3 ++-
tools/libxc/xc_resource.c | 7 ++++---
tools/libxc/xc_sr_common.c | 3 ++-
tools/libxc/xc_sr_restore.c | 18 ++++++++++++------
tools/libxc/xc_sr_restore_x86_hvm.c | 4 +++-
tools/libxc/xc_sr_restore_x86_pv.c | 33 ++++++++++++++++++---------------
7 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 9bd04cb2d5..73fe09fe18 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -225,7 +225,8 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
"tried to map file which is too large");
goto err;
}
- else if ( !*size )
+
+ if ( !*size )
{
xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
"'%s': zero length file", filename);
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 8e60b6e9f0..b8eebd91e4 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -794,7 +794,8 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
xc_hypercall_bounce_post(xch, len);
continue;
}
- else if ( rc < 0 ) /* For all other errors we bail out. */
+
+ if ( rc < 0 ) /* For all other errors we bail out. */
break;
if ( !version )
diff --git a/tools/libxc/xc_resource.c b/tools/libxc/xc_resource.c
index 3abadbdcfc..3394cc1833 100644
--- a/tools/libxc/xc_resource.c
+++ b/tools/libxc/xc_resource.c
@@ -133,10 +133,11 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops)
{
if ( nr_ops == 1 )
return xc_resource_op_one(xch, ops);
- else if ( nr_ops > 1 )
+
+ if ( nr_ops > 1 )
return xc_resource_op_multi(xch, nr_ops, ops);
- else
- return -1;
+
+ return -1;
}
/*
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 79b9c3e940..6b887b3053 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -102,7 +102,8 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec)
PERROR("Failed to read Record Header from stream");
return -1;
}
- else if ( rhdr.length > REC_LENGTH_MAX )
+
+ if ( rhdr.length > REC_LENGTH_MAX )
{
ERROR("Record (0x%08x, %s) length %#x exceeds max (%#x)", rhdr.type,
rec_type_to_str(rhdr.type), rhdr.length, REC_LENGTH_MAX);
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 1ac404b97b..98038096c7 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -28,18 +28,21 @@ static int read_headers(struct xc_sr_context *ctx)
ERROR("Invalid marker: Got 0x%016"PRIx64, ihdr.marker);
return -1;
}
- else if ( ihdr.id != IHDR_ID )
+
+ if ( ihdr.id != IHDR_ID )
{
ERROR("Invalid ID: Expected 0x%08x, Got 0x%08x", IHDR_ID, ihdr.id);
return -1;
}
- else if ( ihdr.version != IHDR_VERSION )
+
+ if ( ihdr.version != IHDR_VERSION )
{
ERROR("Invalid Version: Expected %d, Got %d",
IHDR_VERSION, ihdr.version);
return -1;
}
- else if ( ihdr.options & IHDR_OPT_BIG_ENDIAN )
+
+ if ( ihdr.options & IHDR_OPT_BIG_ENDIAN )
{
ERROR("Unable to handle big endian streams");
return -1;
@@ -345,12 +348,14 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
rec->length, sizeof(*pages));
goto err;
}
- else if ( pages->count < 1 )
+
+ if ( pages->count < 1 )
{
ERROR("Expected at least 1 pfn in PAGE_DATA record");
goto err;
}
- else if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t)) )
+
+ if ( rec->length < sizeof(*pages) + (pages->count * sizeof(uint64_t)) )
{
ERROR("PAGE_DATA record (length %u) too short to contain %u"
" pfns worth of information", rec->length, pages->count);
@@ -383,7 +388,8 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
type, pfn, i);
goto err;
}
- else if ( type < XEN_DOMCTL_PFINFO_BROKEN )
+
+ if ( type < XEN_DOMCTL_PFINFO_BROKEN )
/* NOTAB and all L1 through L4 tables (including pinned) should
* have a page worth of data in the record. */
pages_of_data++;
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 4765a52f33..9763aaa8dc 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -142,12 +142,14 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
dhdr_type_to_str(ctx->restore.guest_type));
return -1;
}
- else if ( ctx->restore.guest_page_size != PAGE_SIZE )
+
+ if ( ctx->restore.guest_page_size != PAGE_SIZE )
{
ERROR("Invalid page size %u for x86_hvm domains",
ctx->restore.guest_page_size);
return -1;
}
+
#ifdef __i386__
/* Very large domains (> 1TB) will exhaust virtual address space. */
if ( ctx->restore.p2m_size > 0x0fffffff )
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 9e9ff32d47..2367f2a9ed 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -145,7 +145,8 @@ static int process_start_info(struct xc_sr_context *ctx,
ERROR("Start Info pfn %#lx out of range", pfn);
goto err;
}
- else if ( ctx->x86_pv.restore.pfn_types[pfn] != XEN_DOMCTL_PFINFO_NOTAB )
+
+ if ( ctx->x86_pv.restore.pfn_types[pfn] != XEN_DOMCTL_PFINFO_NOTAB )
{
ERROR("Start Info pfn %#lx has bad type %u", pfn,
(ctx->x86_pv.restore.pfn_types[pfn] >>
@@ -275,8 +276,8 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
ERROR("GDT frame %u (pfn %#lx) out of range", i, pfn);
goto err;
}
- else if ( (ctx->x86_pv.restore.pfn_types[pfn] !=
- XEN_DOMCTL_PFINFO_NOTAB) )
+
+ if ( (ctx->x86_pv.restore.pfn_types[pfn] != XEN_DOMCTL_PFINFO_NOTAB) )
{
ERROR("GDT frame %u (pfn %#lx) has bad type %u", i, pfn,
(ctx->x86_pv.restore.pfn_types[pfn] >>
@@ -302,10 +303,10 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
ERROR("cr3 (pfn %#lx) out of range", pfn);
goto err;
}
- else if ( (ctx->x86_pv.restore.pfn_types[pfn] &
- XEN_DOMCTL_PFINFO_LTABTYPE_MASK) !=
- (((xen_pfn_t)ctx->x86_pv.levels) <<
- XEN_DOMCTL_PFINFO_LTAB_SHIFT) )
+
+ if ( (ctx->x86_pv.restore.pfn_types[pfn] &
+ XEN_DOMCTL_PFINFO_LTABTYPE_MASK) !=
+ (((xen_pfn_t)ctx->x86_pv.levels) << XEN_DOMCTL_PFINFO_LTAB_SHIFT) )
{
ERROR("cr3 (pfn %#lx) has bad type %u, expected %u", pfn,
(ctx->x86_pv.restore.pfn_types[pfn] >>
@@ -334,10 +335,10 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
ERROR("cr1 (pfn %#lx) out of range", pfn);
goto err;
}
- else if ( (ctx->x86_pv.restore.pfn_types[pfn] &
- XEN_DOMCTL_PFINFO_LTABTYPE_MASK) !=
- (((xen_pfn_t)ctx->x86_pv.levels) <<
- XEN_DOMCTL_PFINFO_LTAB_SHIFT) )
+
+ if ( (ctx->x86_pv.restore.pfn_types[pfn] &
+ XEN_DOMCTL_PFINFO_LTABTYPE_MASK) !=
+ (((xen_pfn_t)ctx->x86_pv.levels) << XEN_DOMCTL_PFINFO_LTAB_SHIFT) )
{
ERROR("cr1 (pfn %#lx) has bad type %u, expected %u", pfn,
(ctx->x86_pv.restore.pfn_types[pfn] >>
@@ -542,8 +543,8 @@ static int update_guest_p2m(struct xc_sr_context *ctx)
pfn, i);
goto err;
}
- else if ( (ctx->x86_pv.restore.pfn_types[pfn] !=
- XEN_DOMCTL_PFINFO_NOTAB) )
+
+ if ( (ctx->x86_pv.restore.pfn_types[pfn] != XEN_DOMCTL_PFINFO_NOTAB) )
{
ERROR("pfn (%#lx) for p2m_frame_list[%u] has bad type %u", pfn, i,
(ctx->x86_pv.restore.pfn_types[pfn] >>
@@ -692,7 +693,8 @@ static int handle_x86_pv_p2m_frames(struct xc_sr_context *ctx,
rec->length, sizeof(*data) + sizeof(uint64_t));
return -1;
}
- else if ( data->start_pfn > data->end_pfn )
+
+ if ( data->start_pfn > data->end_pfn )
{
ERROR("End pfn in stream (%#x) exceeds Start (%#x)",
data->end_pfn, data->start_pfn);
@@ -1039,7 +1041,8 @@ static int x86_pv_setup(struct xc_sr_context *ctx)
dhdr_type_to_str(ctx->restore.guest_type));
return -1;
}
- else if ( ctx->restore.guest_page_size != PAGE_SIZE )
+
+ if ( ctx->restore.guest_page_size != PAGE_SIZE )
{
ERROR("Invalid page size %d for x86_pv domains",
ctx->restore.guest_page_size);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develAndrew Cooper writes ("[PATCH 2/1] libxc: Drop other examples of the 'goto x; } else if' antipattern"):
> None of these are buggy, but the resulting code is more robust.
>
> No functional change.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.