Re: [PATCH v3] x86/nSVM: Check the L1 IOPM_BASE assigned physical address

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2026 17:58, Abdelkareem Abdelsaamad wrote:
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -18,6 +18,7 @@
>  
>  #define NSVM_ERROR_VVMCB        1
>  #define NSVM_ERROR_VMENTRY      2
> +#define IOPM_PAGES_COUNT        3

This new item is separate from the NSVM_ERROR_* values and hence wants 
separating
by a blank line. Especially with the three numbers being in sequence, not doing
so could end up being confusing.

Considering the constant is used exactly once - do we actually need a constant?
Can't we ...

> @@ -294,6 +295,15 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, bool 
> viopm)
>      enum hvm_translation_result ret;
>      unsigned long *ns_viomap;
>      bool ioport_80 = true, ioport_ed = true;
> +    gfn_t ns_iopm_end =
> +        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), IOPM_PAGES_COUNT - 1);

... use a suitable expression here, e.g. PFN_DOWN((0xffff + 3) / 8)?

> +    if ( !gfn_valid(v->domain, ns_iopm_end) )
> +    {
> +        gdprintk(XENLOG_ERR, "%s invalid _iopm_base_pa address 
> (%#"PRIx64")\n",
> +                 __func__, ns_vmcb->_iopm_base_pa);
> +        return NSVM_ERROR_VVMCB;
> +    }
>  
>      ns_msrpm_ptr = (unsigned long *)svm->ns_cached_msrpm;
>  
> @@ -302,13 +312,12 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, 
> bool viopm)
>      if ( ret != HVMTRANS_okay )
>      {
>          gdprintk(XENLOG_ERR, "hvm_copy_from_guest_phys msrpm %u\n", ret);
> -        return 1;
> +        return NSVM_ERROR_VVMCB;
>      }
>  
>      /* Check l1 guest io permission map and get a shadow one based on
>       * if l1 guest intercepts io ports 0x80 and/or 0xED.
>       */
> -    svm->ns_oiomap_pa = svm->ns_iomap_pa;
>      svm->ns_iomap_pa = ns_vmcb->_iopm_base_pa;
>  
>      ns_viomap = hvm_map_guest_frame_ro(svm->ns_iomap_pa >> PAGE_SHIFT, 0);

In the description you say "without any sanity checks", yet
hvm_map_guest_frame_ro() -> _hvm_map_guest_frame() ->
check_get_page_from_gfn() won't allow unsuitable GFNs to be mapped. Since
here only the first page is mapped, some extra checking may indeed be
warranted, but the description then wants updating.

As to that part of the description, "directly to valid host address" also
doesn't look to adequately describe what's going on.

Jan