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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 04/03/2026 11:43, Jan Beulich wrote:
> 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?
AFAICT (I though of a few scenarios), the race can only reproduce the pre-patch
behavior - pages leaked until domain destruction. After this patch, it only
happens if two vCPUs race on the same GFN.

> 
>> +     * 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.
I'll address that in the v3. I also found that I cannot use guest_remove_page()
for foreign. Foreign pages have a cleanup in the insertion path through
p2m_set_entry.

~Michal