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
