| Message ID | 20200106155423.9508-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/boot: Remove mappings at 0 | expand |
On 06.01.2020 16:54, Andrew Cooper wrote: > c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, > removed the final writes which occurred between enabling paging and switching > to the high mappings. There don't plausibly need to be any memory writes in > few instructions is takes to perform this transition. > > As a consequence, we can remove the RWX mapping of the trampoline. It is RX > via its identity mapping below 1M, and RW via the directmap. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. This is just cleanup, largely cosmetic in nature. It could be argued that once the directmap has disappeared this can serve as additional proof that the trampoline range has no (intended) writable mappings anymore, but prior to that point I don't see much further benefit. Could you expand on the reasons why you see both as backporting candidates? Jan
On 07/01/2020 15:21, Jan Beulich wrote: > On 06.01.2020 16:54, Andrew Cooper wrote: >> c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, >> removed the final writes which occurred between enabling paging and switching >> to the high mappings. There don't plausibly need to be any memory writes in >> few instructions is takes to perform this transition. >> >> As a consequence, we can remove the RWX mapping of the trampoline. It is RX >> via its identity mapping below 1M, and RW via the directmap. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > >> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. > This is just cleanup, largely cosmetic in nature. It could be argued > that once the directmap has disappeared this can serve as additional > proof that the trampoline range has no (intended) writable mappings > anymore, but prior to that point I don't see much further benefit. > Could you expand on the reasons why you see both as backporting > candidates? Defence in depth. An RWX mapping is very attractive for an attacker who's broken into Xen and is looking to expand the damage they can do. ~Andrew
On 07.01.2020 16:51, Andrew Cooper wrote: > On 07/01/2020 15:21, Jan Beulich wrote: >> On 06.01.2020 16:54, Andrew Cooper wrote: >>> c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, >>> removed the final writes which occurred between enabling paging and switching >>> to the high mappings. There don't plausibly need to be any memory writes in >>> few instructions is takes to perform this transition. >>> >>> As a consequence, we can remove the RWX mapping of the trampoline. It is RX >>> via its identity mapping below 1M, and RW via the directmap. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >>> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. >> This is just cleanup, largely cosmetic in nature. It could be argued >> that once the directmap has disappeared this can serve as additional >> proof that the trampoline range has no (intended) writable mappings >> anymore, but prior to that point I don't see much further benefit. >> Could you expand on the reasons why you see both as backporting >> candidates? > > Defence in depth. > > An RWX mapping is very attractive for an attacker who's broken into Xen > and is looking to expand the damage they can do. Such an attacker is typically in the position though to make themselves RWX mappings. Having as little as possible is only complicating their job, not making it impossible, I would say. Jan
On 07/01/2020 16:19, Jan Beulich wrote: > On 07.01.2020 16:51, Andrew Cooper wrote: >> On 07/01/2020 15:21, Jan Beulich wrote: >>> On 06.01.2020 16:54, Andrew Cooper wrote: >>>> c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, >>>> removed the final writes which occurred between enabling paging and switching >>>> to the high mappings. There don't plausibly need to be any memory writes in >>>> few instructions is takes to perform this transition. >>>> >>>> As a consequence, we can remove the RWX mapping of the trampoline. It is RX >>>> via its identity mapping below 1M, and RW via the directmap. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>>> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. >>> This is just cleanup, largely cosmetic in nature. It could be argued >>> that once the directmap has disappeared this can serve as additional >>> proof that the trampoline range has no (intended) writable mappings >>> anymore, but prior to that point I don't see much further benefit. >>> Could you expand on the reasons why you see both as backporting >>> candidates? >> Defence in depth. >> >> An RWX mapping is very attractive for an attacker who's broken into Xen >> and is looking to expand the damage they can do. > Such an attacker is typically in the position though to make > themselves RWX mappings. This is one example of a possibility. I wouldn't put it in the "likely" category, and it definitely isn't a guarantee. > Having as little as possible is only > complicating their job, not making it impossible, I would say. Yes, and? This is the entire point of defence in depth. Make an attackers job harder. Enforcing W^X is universally considered a good thing from a security perspective, because it removes a load of trivial cases cases where a stack over-write can easily be turned into arbitrary code execution. Sure - this isn't going to stop an attacker who has arbitrary write exploit, but it very well might stop an attacker who only has restricted write exploit. ~Andrew
On 07.01.2020 20:04, Andrew Cooper wrote: > On 07/01/2020 16:19, Jan Beulich wrote: >> On 07.01.2020 16:51, Andrew Cooper wrote: >>> On 07/01/2020 15:21, Jan Beulich wrote: >>>> On 06.01.2020 16:54, Andrew Cooper wrote: >>>>> c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, >>>>> removed the final writes which occurred between enabling paging and switching >>>>> to the high mappings. There don't plausibly need to be any memory writes in >>>>> few instructions is takes to perform this transition. >>>>> >>>>> As a consequence, we can remove the RWX mapping of the trampoline. It is RX >>>>> via its identity mapping below 1M, and RW via the directmap. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> >>>>> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. >>>> This is just cleanup, largely cosmetic in nature. It could be argued >>>> that once the directmap has disappeared this can serve as additional >>>> proof that the trampoline range has no (intended) writable mappings >>>> anymore, but prior to that point I don't see much further benefit. >>>> Could you expand on the reasons why you see both as backporting >>>> candidates? >>> Defence in depth. >>> >>> An RWX mapping is very attractive for an attacker who's broken into Xen >>> and is looking to expand the damage they can do. >> Such an attacker is typically in the position though to make >> themselves RWX mappings. > > This is one example of a possibility. I wouldn't put it in the "likely" > category, and it definitely isn't a guarantee. > >> Having as little as possible is only >> complicating their job, not making it impossible, I would say. > > Yes, and? > > This is the entire point of defence in depth. Make an attackers job harder. > > Enforcing W^X is universally considered a good thing from a security > perspective, because it removes a load of trivial cases cases where a > stack over-write can easily be turned into arbitrary code execution. Then let me ask the question differently: Did we backport any of the earlier RWX elimination changes? I don't recall us doing so. Please don't get me wrong - I'm happy to be convinced of the backport need, but as always I'd like to take such a decision in a consistent (and hence sufficiently predictable) manner, or alternatively with a good enough reason to ignore this general goal. Jan
On 08/01/2020 11:08, Jan Beulich wrote: > On 07.01.2020 20:04, Andrew Cooper wrote: >> On 07/01/2020 16:19, Jan Beulich wrote: >>> On 07.01.2020 16:51, Andrew Cooper wrote: >>>> On 07/01/2020 15:21, Jan Beulich wrote: >>>>> On 06.01.2020 16:54, Andrew Cooper wrote: >>>>>> c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, >>>>>> removed the final writes which occurred between enabling paging and switching >>>>>> to the high mappings. There don't plausibly need to be any memory writes in >>>>>> few instructions is takes to perform this transition. >>>>>> >>>>>> As a consequence, we can remove the RWX mapping of the trampoline. It is RX >>>>>> via its identity mapping below 1M, and RW via the directmap. >>>>>> >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>>> >>>>>> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. >>>>> This is just cleanup, largely cosmetic in nature. It could be argued >>>>> that once the directmap has disappeared this can serve as additional >>>>> proof that the trampoline range has no (intended) writable mappings >>>>> anymore, but prior to that point I don't see much further benefit. >>>>> Could you expand on the reasons why you see both as backporting >>>>> candidates? >>>> Defence in depth. >>>> >>>> An RWX mapping is very attractive for an attacker who's broken into Xen >>>> and is looking to expand the damage they can do. >>> Such an attacker is typically in the position though to make >>> themselves RWX mappings. >> This is one example of a possibility. I wouldn't put it in the "likely" >> category, and it definitely isn't a guarantee. >> >>> Having as little as possible is only >>> complicating their job, not making it impossible, I would say. >> Yes, and? >> >> This is the entire point of defence in depth. Make an attackers job harder. >> >> Enforcing W^X is universally considered a good thing from a security >> perspective, because it removes a load of trivial cases cases where a >> stack over-write can easily be turned into arbitrary code execution. > Then let me ask the question differently: Did we backport any of the > earlier RWX elimination changes? I don't recall us doing so. I don't know if we did or not. > Please > don't get me wrong - I'm happy to be convinced of the backport need, > but as always I'd like to take such a decision in a consistent (and > hence sufficiently predictable) manner, or alternatively with a good > enough reason to ignore this general goal. If we didn't, then we really ought to have done. There are real, concrete security nice-to-haves from it. ~Andrew
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index 8ea09ecc30..b7ce833ffc 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -699,7 +699,7 @@ void __init zap_low_mappings(void) /* Replace with mapping of the boot trampoline only. */ map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys), PFN_UP(trampoline_end - trampoline_start), - __PAGE_HYPERVISOR); + __PAGE_HYPERVISOR_RX); } int setup_compat_arg_xlat(struct vcpu *v)
c/s ec92fcd1d08, which caused the trampoline GDT Access bits to be set, removed the final writes which occurred between enabling paging and switching to the high mappings. There don't plausibly need to be any memory writes in few instructions is takes to perform this transition. As a consequence, we can remove the RWX mapping of the trampoline. It is RX via its identity mapping below 1M, and RW via the directmap. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> This probably wants backporting, alongside ec92fcd1d08 if it hasn't yet. --- xen/arch/x86/x86_64/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)