On Fri, Dec 20, 2019 at 02:49:48PM +0100, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit
> (in the native case, other than in the compat one).
Wouldn't this result in arch_set_info_guest failing if gdt_ents was
smaller than the maximum? Or all callers always pass gdt_ents set to
the maximum?
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -840,6 +840,7 @@ int arch_set_info_guest(
> #ifdef CONFIG_PV
> mfn_t cr3_mfn;
> struct page_info *cr3_page = NULL;
> + unsigned int nr_gdt_frames;
> int rc = 0;
> #endif
>
> @@ -951,6 +952,8 @@ int arch_set_info_guest(
> /* Ensure real hardware interrupts are enabled. */
> v->arch.user_regs.eflags |= X86_EFLAGS_IF;
>
> + nr_gdt_frames = DIV_ROUND_UP(c(gdt_ents), 512);
> +
> if ( !v->is_initialised )
> {
> if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] )
> @@ -982,9 +985,9 @@ int arch_set_info_guest(
> fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
> }
>
> - for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
> - fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
> fail |= v->arch.pv.gdt_ents != c(gdt_ents);
> + for ( i = 0; !fail && i < nr_gdt_frames; ++i )
> + fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
fail doesn't need to be OR'ed anymore here, since you check for it in
the loop condition.
>
> fail |= v->arch.pv.ldt_base != c(ldt_base);
> fail |= v->arch.pv.ldt_ents != c(ldt_ents);
> @@ -1089,12 +1092,11 @@ int arch_set_info_guest(
> else
> {
> unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
> - unsigned int nr_frames = DIV_ROUND_UP(c.cmp->gdt_ents, 512);
>
> - if ( nr_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
> + if ( nr_gdt_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
> return -EINVAL;
Shouldn't this check be performed when nr_gdt_frames is initialized
instead of here? (as nr_gdt_frames is already used as a limit to
iterate over gdt_frames).
Thanks, Roger.
