Addressing a few assorted aspects I've noticed during the investigations / reviews. 1: mod_l<N>_entry() have no need to use __copy_from_user() 2: rename and tidy create_pae_xen_mappings() 3: avoid IOMMU operations in more cases in _get_page_type() 4: drop redundant smp_wmb() from _put_final_page_type() Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
Fold page_info lock into type_info"), and the other three never had such
a need, at least going back as far as 3.2.0.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2124,13 +2124,10 @@ static int mod_l1_entry(l1_pgentry_t *pl
struct vcpu *pt_vcpu, struct domain *pg_dom)
{
bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD);
- l1_pgentry_t ol1e;
+ l1_pgentry_t ol1e = *pl1e;
struct domain *pt_dom = pt_vcpu->domain;
int rc = 0;
- if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) )
- return -EFAULT;
-
ASSERT(!paging_mode_refcounts(pt_dom));
if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
@@ -2248,8 +2245,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
return -EPERM;
}
- if ( unlikely(__copy_from_user(&ol2e, pl2e, sizeof(ol2e)) != 0) )
- return -EFAULT;
+ ol2e = *pl2e;
if ( l2e_get_flags(nl2e) & _PAGE_PRESENT )
{
@@ -2311,8 +2307,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
if ( is_pv_32bit_domain(d) && (pgentry_ptr_to_slot(pl3e) >= 3) )
return -EINVAL;
- if ( unlikely(__copy_from_user(&ol3e, pl3e, sizeof(ol3e)) != 0) )
- return -EFAULT;
+ ol3e = *pl3e;
if ( l3e_get_flags(nl3e) & _PAGE_PRESENT )
{
@@ -2378,8 +2373,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
return -EINVAL;
}
- if ( unlikely(__copy_from_user(&ol4e, pl4e, sizeof(ol4e)) != 0) )
- return -EFAULT;
+ ol4e = *pl4e;
if ( l4e_get_flags(nl4e) & _PAGE_PRESENT )
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 14:19, Jan Beulich wrote:
> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
> Fold page_info lock into type_info"), and the other three never had such
> a need, at least going back as far as 3.2.0.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
These presumably want ACCESS_ONCE() to avoid introducing repeated read
vulnerabilities? While the mappings might be safe, they still point to
live guest data.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.12.2019 15:42, Andrew Cooper wrote:
> On 20/12/2019 14:19, Jan Beulich wrote:
>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
>> Fold page_info lock into type_info"), and the other three never had such
>> a need, at least going back as far as 3.2.0.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> These presumably want ACCESS_ONCE() to avoid introducing repeated read
> vulnerabilities? While the mappings might be safe, they still point to
> live guest data.
Oh, yes - will do.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 12/20/19 2:42 PM, Andrew Cooper wrote:
> On 20/12/2019 14:19, Jan Beulich wrote:
>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
>> Fold page_info lock into type_info"), and the other three never had such
>> a need, at least going back as far as 3.2.0.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> These presumably want ACCESS_ONCE() to avoid introducing repeated read
> vulnerabilities? While the mappings might be safe, they still point to
> live guest data.
The L1 itself should be mapped read-only by the guest, and locked at
this point, no?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.12.2019 15:47, George Dunlap wrote:
> On 12/20/19 2:42 PM, Andrew Cooper wrote:
>> On 20/12/2019 14:19, Jan Beulich wrote:
>>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
>>> Fold page_info lock into type_info"), and the other three never had such
>>> a need, at least going back as far as 3.2.0.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> These presumably want ACCESS_ONCE() to avoid introducing repeated read
>> vulnerabilities? While the mappings might be safe, they still point to
>> live guest data.
>
> The L1 itself should be mapped read-only by the guest, and locked at
> this point, no?
True, but I think it won't hurt to use ACCESS_ONCE() nevertheless
- it makes the code more obviously safe independent of any locking
knowledge.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 12/20/19 2:52 PM, Jan Beulich wrote:
> On 20.12.2019 15:47, George Dunlap wrote:
>> On 12/20/19 2:42 PM, Andrew Cooper wrote:
>>> On 20/12/2019 14:19, Jan Beulich wrote:
>>>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
>>>> Fold page_info lock into type_info"), and the other three never had such
>>>> a need, at least going back as far as 3.2.0.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> These presumably want ACCESS_ONCE() to avoid introducing repeated read
>>> vulnerabilities? While the mappings might be safe, they still point to
>>> live guest data.
>>
>> The L1 itself should be mapped read-only by the guest, and locked at
>> this point, no?
>
> True, but I think it won't hurt to use ACCESS_ONCE() nevertheless
> - it makes the code more obviously safe independent of any locking
> knowledge.
But that might give someone the idea that that it *was* safe to do the
type adjustments without the page being locked, which it certainly isn't.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.12.2019 15:54, George Dunlap wrote:
> On 12/20/19 2:52 PM, Jan Beulich wrote:
>> On 20.12.2019 15:47, George Dunlap wrote:
>>> On 12/20/19 2:42 PM, Andrew Cooper wrote:
>>>> On 20/12/2019 14:19, Jan Beulich wrote:
>>>>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86:
>>>>> Fold page_info lock into type_info"), and the other three never had such
>>>>> a need, at least going back as far as 3.2.0.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> These presumably want ACCESS_ONCE() to avoid introducing repeated read
>>>> vulnerabilities? While the mappings might be safe, they still point to
>>>> live guest data.
>>>
>>> The L1 itself should be mapped read-only by the guest, and locked at
>>> this point, no?
>>
>> True, but I think it won't hurt to use ACCESS_ONCE() nevertheless
>> - it makes the code more obviously safe independent of any locking
>> knowledge.
>
> But that might give someone the idea that that it *was* safe to do the
> type adjustments without the page being locked, which it certainly isn't.
It isn't in the common case, but we couldn't, for example, put
in ASSERT()s in place of using ACCESS_ONCE(), as new_guest_cr3()'s
use of mod_l4_entry() is without lock, yet still safe (because of
acting on the otherwise immutable L4 table of a 32-bit guest).
FAOD this code path also doesn't need the ACCESS_ONCE() (for the
same reason).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
After dad74b0f9e ("i386: fix handling of Xen entries in final L2 page
table") and the removal of 32-bit support the function doesn't modify
state anymore, and hence its name has been misleading. Change its name,
constify parameters and a local variable, and make it return bool.
Also drop the call to it from mod_l3_entry(): The function explicitly
disallows 32-bit domains to modify slot 3. Furthermore, re-checking
of slot 3 when other slots change has needlessly disallowed doing this
together with making some L2 table recursively link back to an L2 used
in some L3's 3rd slot (because of the checking of the type ref count to
be 1). (Note that allowing dynamic changes of L3 entries in the way we
do is bogus anyway, as that's not how L3s behave in the native case:
They get re-evaluated only upon CR3 reloads.)
As a result of this we no longer need to play games to get at the start
of the L3 table. Leave an ASSERT() though, just in case.
Further move a BUG_ON() such that in the common case its condition
wouldn't need evaluating.
Finally, since we're at it, move init_xen_pae_l2_slots() next to the
renamed function, as they really belong together (in fact
init_xen_pae_l2_slots() was [indirectly] broken out of this function).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
We could go further here and delete the function altogether: There are
no linear mappings in a PGT_pae_xen_l2 table anymore (this was on 32-bit
only). The corresponding conditional in mod_l3_entry() could then go
away as well (or, more precisely, would need to be replaced by correct
handling of 3rd slot updates). This would mean that a 32-bit guest
functioning on new Xen may fail to work on older (possibly 32-bit) Xen.
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1414,22 +1414,23 @@ static int promote_l1_table(struct page_
return ret;
}
-static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
+static bool pae_xen_mappings_check(const struct domain *d,
+ const l3_pgentry_t *pl3e)
{
- struct page_info *page;
- l3_pgentry_t l3e3;
+ const struct page_info *page;
+ l3_pgentry_t l3e3;
if ( !is_pv_32bit_domain(d) )
- return 1;
+ return true;
- pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
+ ASSERT(!((unsigned long)pl3e & ~PAGE_MASK));
/* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
l3e3 = pl3e[3];
if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
{
gdprintk(XENLOG_WARNING, "PAE L3 3rd slot is empty\n");
- return 0;
+ return false;
}
/*
@@ -1443,15 +1444,23 @@ static int create_pae_xen_mappings(struc
*/
page = l3e_get_page(l3e3);
BUG_ON(page->u.inuse.type_info & PGT_pinned);
- BUG_ON((page->u.inuse.type_info & PGT_count_mask) == 0);
BUG_ON(!(page->u.inuse.type_info & PGT_pae_xen_l2));
if ( (page->u.inuse.type_info & PGT_count_mask) != 1 )
{
+ BUG_ON(!(page->u.inuse.type_info & PGT_count_mask));
gdprintk(XENLOG_WARNING, "PAE L3 3rd slot is shared\n");
- return 0;
+ return false;
}
- return 1;
+ return true;
+}
+
+void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d)
+{
+ memcpy(&l2t[COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d)],
+ &compat_idle_pg_table_l2[
+ l2_table_offset(HIRO_COMPAT_MPT_VIRT_START)],
+ COMPAT_L2_PAGETABLE_XEN_SLOTS(d) * sizeof(*l2t));
}
static int promote_l2_table(struct page_info *page, unsigned long type)
@@ -1621,7 +1630,7 @@ static int promote_l3_table(struct page_
pl3e[i] = adjust_guest_l3e(l3e, d);
}
- if ( !rc && !create_pae_xen_mappings(d, pl3e) )
+ if ( !rc && !pae_xen_mappings_check(d, pl3e) )
rc = -EINVAL;
if ( rc < 0 && rc != -ERESTART && rc != -EINTR )
{
@@ -1663,14 +1672,6 @@ static int promote_l3_table(struct page_
unmap_domain_page(pl3e);
return rc;
}
-
-void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d)
-{
- memcpy(&l2t[COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d)],
- &compat_idle_pg_table_l2[
- l2_table_offset(HIRO_COMPAT_MPT_VIRT_START)],
- COMPAT_L2_PAGETABLE_XEN_SLOTS(d) * sizeof(*l2t));
-}
#endif /* CONFIG_PV */
/*
@@ -2349,10 +2350,6 @@ static int mod_l3_entry(l3_pgentry_t *pl
return -EFAULT;
}
- if ( likely(rc == 0) )
- if ( !create_pae_xen_mappings(d, pl3e) )
- BUG();
-
put_page_from_l3e(ol3e, mfn, PTF_defer);
return rc;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 14:19, Jan Beulich wrote:
> After dad74b0f9e ("i386: fix handling of Xen entries in final L2 page
> table") and the removal of 32-bit support the function doesn't modify
> state anymore, and hence its name has been misleading. Change its name,
> constify parameters and a local variable, and make it return bool.
>
> Also drop the call to it from mod_l3_entry(): The function explicitly
> disallows 32-bit domains to modify slot 3. Furthermore, re-checking
> of slot 3 when other slots change has needlessly disallowed doing this
> together with making some L2 table recursively link back to an L2 used
> in some L3's 3rd slot (because of the checking of the type ref count to
> be 1). (Note that allowing dynamic changes of L3 entries in the way we
> do is bogus anyway, as that's not how L3s behave in the native case:
> They get re-evaluated only upon CR3 reloads.)
I'm having a hard time following this. I agree that dropping the call
from mod_l3_entry() is safe, but I cant parse the second sentence.
As for the final sentence, I'm not sure that is actually true. In
particular, AMD NPT says that the PDPTRs may actually behave like L3's
in 4-level paging, rather than how legacy PAE used to work.
> As a result of this we no longer need to play games to get at the start
> of the L3 table. Leave an ASSERT() though, just in case.
I'd drop the assert(). The sole caller is internal, and we would have
major problems if any of mm.c operated on misaligned pointers. (I did
double check all of the MM code following whichever XSA the misaligned
grant map by PTE was.)
>
> Further move a BUG_ON() such that in the common case its condition
> wouldn't need evaluating.
>
> Finally, since we're at it, move init_xen_pae_l2_slots() next to the
> renamed function, as they really belong together (in fact
> init_xen_pae_l2_slots() was [indirectly] broken out of this function).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> We could go further here and delete the function altogether: There are
> no linear mappings in a PGT_pae_xen_l2 table anymore (this was on 32-bit
> only). The corresponding conditional in mod_l3_entry() could then go
> away as well (or, more precisely, would need to be replaced by correct
> handling of 3rd slot updates). This would mean that a 32-bit guest
> functioning on new Xen may fail to work on older (possibly 32-bit) Xen.
Given that PV guests can't do anything differently or they'd stop
working on pre-4.14, I don't think removing this restriction is helpful.
It would be worth explicitly commenting that it is a legacy restriction
however. (I could have sworn I submitted a patch to do this, but I
can't locate it in my tree.)
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1414,22 +1414,23 @@ static int promote_l1_table(struct page_
> return ret;
> }
>
> -static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
> +static bool pae_xen_mappings_check(const struct domain *d,
> + const l3_pgentry_t *pl3e)
> {
> - struct page_info *page;
> - l3_pgentry_t l3e3;
> + const struct page_info *page;
> + l3_pgentry_t l3e3;
>
> if ( !is_pv_32bit_domain(d) )
> - return 1;
> + return true;
There is now a single caller. By repositioning the caller in
promote_l3_table(), this condition can be dropped, which removes one
eval_nospec() from the mix (seeing as these are expensive now.)
>
> - pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
> + ASSERT(!((unsigned long)pl3e & ~PAGE_MASK));
>
> /* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
> l3e3 = pl3e[3];
> if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
> {
> gdprintk(XENLOG_WARNING, "PAE L3 3rd slot is empty\n");
Also, by repositioning, it becomes clear that the _PAGE_PRESENT check is
duplicated, and this printk() is now dead code.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.12.2019 16:33, Andrew Cooper wrote:
> On 20/12/2019 14:19, Jan Beulich wrote:
>> After dad74b0f9e ("i386: fix handling of Xen entries in final L2 page
>> table") and the removal of 32-bit support the function doesn't modify
>> state anymore, and hence its name has been misleading. Change its name,
>> constify parameters and a local variable, and make it return bool.
>>
>> Also drop the call to it from mod_l3_entry(): The function explicitly
>> disallows 32-bit domains to modify slot 3. Furthermore, re-checking
>> of slot 3 when other slots change has needlessly disallowed doing this
>> together with making some L2 table recursively link back to an L2 used
>> in some L3's 3rd slot (because of the checking of the type ref count to
>> be 1). (Note that allowing dynamic changes of L3 entries in the way we
>> do is bogus anyway, as that's not how L3s behave in the native case:
>> They get re-evaluated only upon CR3 reloads.)
>
> I'm having a hard time following this. I agree that dropping the call
> from mod_l3_entry() is safe, but I cant parse the second sentence.
>
> As for the final sentence, I'm not sure that is actually true. In
> particular, AMD NPT says that the PDPTRs may actually behave like L3's
> in 4-level paging, rather than how legacy PAE used to work.
I've re-written this to
"Also drop the call to it from mod_l3_entry(): The function explicitly
disallows 32-bit domains to modify slot 3. This way we also won't
re-check slot 3 when a slot other than slot 3 changes. Doing so has
needlessly disallowed making some L2 table recursively link back to an
L2 used in some L3's 3rd slot, as we check for the type ref count to be
1. (Note that allowing dynamic changes of L3 entries in the way we do is
bogus anyway, as that's not how L3s behave in the native and EPT cases:
They get re-evaluated only upon CR3 reloads. NPT is different in this
regard.)"
If this isn't sufficiently better, then I guess I need some hint
towards possible further improvements.
>> As a result of this we no longer need to play games to get at the start
>> of the L3 table. Leave an ASSERT() though, just in case.
>
> I'd drop the assert(). The sole caller is internal, and we would have
> major problems if any of mm.c operated on misaligned pointers. (I did
> double check all of the MM code following whichever XSA the misaligned
> grant map by PTE was.)
Well, okay. As said in the description it was "just in case"
anyway.
>> Further move a BUG_ON() such that in the common case its condition
>> wouldn't need evaluating.
>>
>> Finally, since we're at it, move init_xen_pae_l2_slots() next to the
>> renamed function, as they really belong together (in fact
>> init_xen_pae_l2_slots() was [indirectly] broken out of this function).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> We could go further here and delete the function altogether: There are
>> no linear mappings in a PGT_pae_xen_l2 table anymore (this was on 32-bit
>> only). The corresponding conditional in mod_l3_entry() could then go
>> away as well (or, more precisely, would need to be replaced by correct
>> handling of 3rd slot updates). This would mean that a 32-bit guest
>> functioning on new Xen may fail to work on older (possibly 32-bit) Xen.
>
> Given that PV guests can't do anything differently or they'd stop
> working on pre-4.14, I don't think removing this restriction is helpful.
>
> It would be worth explicitly commenting that it is a legacy restriction
> however. (I could have sworn I submitted a patch to do this, but I
> can't locate it in my tree.)
I'm adding a comment to this effect.
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -1414,22 +1414,23 @@ static int promote_l1_table(struct page_
>> return ret;
>> }
>>
>> -static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
>> +static bool pae_xen_mappings_check(const struct domain *d,
>> + const l3_pgentry_t *pl3e)
>> {
>> - struct page_info *page;
>> - l3_pgentry_t l3e3;
>> + const struct page_info *page;
>> + l3_pgentry_t l3e3;
>>
>> if ( !is_pv_32bit_domain(d) )
>> - return 1;
>> + return true;
>
> There is now a single caller. By repositioning the caller in
> promote_l3_table(), this condition can be dropped, which removes one
> eval_nospec() from the mix (seeing as these are expensive now.)
I was about to say that's not possible, until I spotted the one
inside the loop that it can indeed be folded with.
>> - pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
>> + ASSERT(!((unsigned long)pl3e & ~PAGE_MASK));
>>
>> /* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
>> l3e3 = pl3e[3];
>> if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
>> {
>> gdprintk(XENLOG_WARNING, "PAE L3 3rd slot is empty\n");
>
> Also, by repositioning, it becomes clear that the _PAGE_PRESENT check is
> duplicated, and this printk() is now dead code.
Done as well, thanks for noticing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
All that really matters is whether writability of a page changes; in
paticular e.g. page table -> page table (but different levels)
transitions do not require unmapping the page from the IOMMU again.
Note that the XSA-288 fix did arrange for PGT_none pages not needing
special consideration here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2968,17 +2968,19 @@ static int _get_page_type(struct page_in
return -EINTR;
}
- if ( unlikely((x & PGT_type_mask) != type) )
+ if ( unlikely(((x & PGT_type_mask) == PGT_writable_page) !=
+ (type == PGT_writable_page)) )
{
/* Special pages should not be accessible from devices. */
struct domain *d = page_get_owner(page);
+
if ( d && is_pv_domain(d) && unlikely(need_iommu_pt_sync(d)) )
{
mfn_t mfn = page_to_mfn(page);
if ( (x & PGT_type_mask) == PGT_writable_page )
rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
- else if ( type == PGT_writable_page )
+ else
rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 14:19, Jan Beulich wrote: > All that really matters is whether writability of a page changes; in > paticular e.g. page table -> page table (but different levels) > transitions do not require unmapping the page from the IOMMU again. > > Note that the XSA-288 fix did arrange for PGT_none pages not needing > special consideration here. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
get_page_light()'s use of cmpxchg() is a full barrier already anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2746,7 +2746,7 @@ static int _put_final_page_type(struct p
else
{
BUG_ON(rc != -ERESTART);
- smp_wmb();
+ /* get_page_light() includes a full barrier. */
get_page_light(page);
page->u.inuse.type_info |= PGT_partial;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 14:20, Jan Beulich wrote: > get_page_light()'s use of cmpxchg() is a full barrier already anyway. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> While true, is this actually a clever change to make? The implementation of get_page_light() could plausibly change and no longer be a full barrier, introducing a vulnerability here. OTOH, smp_wmb() is free. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.12.2019 15:51, Andrew Cooper wrote: > On 20/12/2019 14:20, Jan Beulich wrote: >> get_page_light()'s use of cmpxchg() is a full barrier already anyway. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > While true, is this actually a clever change to make? > > The implementation of get_page_light() could plausibly change and no > longer be a full barrier, introducing a vulnerability here. OTOH, > smp_wmb() is free. It's free at the CPU level, but not at the compiler one (where it still is a barrier). I also don't think get_page_light() could change as drastically as losing its LOCK-ed operation, or if it did the author (and reviewer) would be well advised to at least briefly audit use sites (one of the reasons I'm leaving a comment). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 14:55, Jan Beulich wrote: > On 20.12.2019 15:51, Andrew Cooper wrote: >> On 20/12/2019 14:20, Jan Beulich wrote: >>> get_page_light()'s use of cmpxchg() is a full barrier already anyway. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> While true, is this actually a clever change to make? >> >> The implementation of get_page_light() could plausibly change and no >> longer be a full barrier, introducing a vulnerability here. OTOH, >> smp_wmb() is free. > It's free at the CPU level, but not at the compiler one (where > it still is a barrier). I also don't think get_page_light() > could change as drastically as losing its LOCK-ed operation, > or if it did the author (and reviewer) would be well advised > to at least briefly audit use sites (one of the reasons I'm > leaving a comment). The comment should be at get_page_light() then, saying "some callers depend on this function being a full memory barrier", which will be far more obvious for anyone who changes the behaviour. With an adjustment along those lines, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.