Re: [PATCH v2] arm/mm: Fix resource handling in xenmem_add_to_physmap_one

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2026 10:39, Michal Orzel wrote:
> @@ -237,13 +239,73 @@ int xenmem_add_to_physmap_one(
>          break;
>      }
>      case XENMAPSPACE_dev_mmio:
> -        rc = map_dev_mmio_page(d, gfn, _mfn(idx));
> -        return rc;
> +        if ( !iomem_access_permitted(d, idx, idx) )
> +            return 0;
> +
> +        mfn = _mfn(idx);
> +        t = p2m_mmio_direct_c;
> +        break;
>  
>      default:
>          return -ENOSYS;
>      }
>  
> +    /*
> +     * Release the old page reference if it was present.
> +     *
> +     * TODO: There are race conditions in this code due to multiple 
> lock/unlock
> +     * cycles:
> +     *
> +     * Race #1: Between checking the old mapping and removing it, another CPU
> +     * could replace the mapping. We would then remove the wrong mapping.
> +     *
> +     * Race #2: Between removing the old mapping and inserting the new one,
> +     * another CPU could insert a different mapping. We would then silently
> +     * overwrite it.

Can't such races be abused in a security relevant way, e.g. causing leaks of
some sort?

> +     * For now, we accept these races as they require concurrent
> +     * xenmem_add_to_physmap_one operations on the same GFN, which is not a
> +     * normal usage pattern.
> +     */
> +    p2m_read_lock(p2m);
> +    mfn_old = p2m_get_entry(p2m, gfn, &p2mt_old, NULL, NULL, NULL);
> +    p2m_read_unlock(p2m);
> +
> +    if ( mfn_valid(mfn_old) && !mfn_eq(mfn, mfn_old) )
> +    {
> +        if ( is_special_page(mfn_to_page(mfn_old)) )
> +        {
> +            /* Just unmap, don't free */
> +            p2m_write_lock(p2m);
> +            rc = p2m_set_entry(p2m, gfn, 1, INVALID_MFN,
> +                               p2m_invalid, p2m->default_access);
> +            p2m_write_unlock(p2m);
> +            if ( rc )
> +                goto out;
> +        }
> +        else if ( p2m_is_mmio(p2mt_old) || p2m_is_grant(p2mt_old) )
> +        {
> +            /* Reject MMIO and grant replacements */
> +            rc = -EPERM;
> +            goto out;
> +        }
> +        else
> +        {
> +            /* Allow RAM and foreign - both have proper cleanup */
> +            rc = guest_remove_page(d, gfn_x(gfn));
> +            if ( rc )
> +                goto out;
> +        }
> +    }
> +    else if ( mfn_valid(mfn_old) )
> +    {
> +        /* Mapping already exists. Drop the references taken above */
> +        if ( page != NULL )
> +            put_page(page);
> +
> +        return 0;

Is this correct regardless of p2mt_old?

> +    }

!mfn_valid(mfn_old) doesn't imply there was no valid mapping. It could be an
MMIO one with an MFN which simply has no associated struct page_info.

Jan