| Message ID | 20220330230549.26074-3-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce XSM ability for domain privilege escalation | expand |
On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote: > It is now possible to promote the idle domain to privileged during setup. It > is not desirable for the idle domain to still be privileged when moving into a > running state. If the idle domain was elevated and not properly demoted, it is > desirable to fail at this point. This commit adds an assert for both x86 and > Arm just before transitioning to a running state that ensures the idle is not > privileged. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 7968cee47d..3de394e946 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, > /* Hide UART from DOM0 if we're using it */ > serial_endboot(); > > + /* Ensure idle domain was not left privileged */ > + ASSERT(current->domain->is_privileged == false) ; > + > system_state = SYS_STATE_active; > > create_domUs(); Hm, I think you want to use the permission promotion of the idle domain in create_domUs() likely? At which point the check should be after create_domUs, and it would seem that logically SYS_STATE_active should be set after creating the domUs. Also, FWIW, I'm not seeing this create_domUs() call in my context, maybe you have other patches on your queue? > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 885919d5c3..b868463f83 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -589,6 +589,9 @@ static void noinline init_done(void) > void *va; > unsigned long start, end; > > + /* Ensure idle domain was not left privileged */ > + ASSERT(current->domain->is_privileged == false) ; ^ extra space. I think you could squash this patch with the previous one and also squash it with a patch that actually makes use of the introduced permission promotion functions (or at least in a patch series where further patches make use the introduced functions). Thanks, Roger.
Hi, On 31/03/2022 13:46, Roger Pau Monné wrote: > On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote: >> It is now possible to promote the idle domain to privileged during setup. It >> is not desirable for the idle domain to still be privileged when moving into a >> running state. If the idle domain was elevated and not properly demoted, it is >> desirable to fail at this point. This commit adds an assert for both x86 and >> Arm just before transitioning to a running state that ensures the idle is not >> privileged. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> xen/arch/arm/setup.c | 3 +++ >> xen/arch/x86/setup.c | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 7968cee47d..3de394e946 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, >> /* Hide UART from DOM0 if we're using it */ >> serial_endboot(); >> >> + /* Ensure idle domain was not left privileged */ >> + ASSERT(current->domain->is_privileged == false) ; >> + >> system_state = SYS_STATE_active; >> >> create_domUs(); > > Hm, I think you want to use the permission promotion of the idle > domain in create_domUs() likely? > > At which point the check should be after create_domUs, and it would > seem that logically SYS_STATE_active should be set after creating the > domUs. > > Also, FWIW, I'm not seeing this create_domUs() call in my context, > maybe you have other patches on your queue? I think the code is based on an old version of Xen (looks like 4.14). In newer version create_domUs() is called before just before discard_initial_modules() (see XSA-372 for the rationale). Daniel, can you please rebase this series to the latest staging? Cheer,
On Thu, 31 Mar 2022, Julien Grall wrote: > On 31/03/2022 13:46, Roger Pau Monné wrote: > > On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote: > > > It is now possible to promote the idle domain to privileged during setup. > > > It > > > is not desirable for the idle domain to still be privileged when moving > > > into a > > > running state. If the idle domain was elevated and not properly demoted, > > > it is > > > desirable to fail at this point. This commit adds an assert for both x86 > > > and > > > Arm just before transitioning to a running state that ensures the idle is > > > not > > > privileged. > > > > > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > > --- > > > xen/arch/arm/setup.c | 3 +++ > > > xen/arch/x86/setup.c | 3 +++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > > index 7968cee47d..3de394e946 100644 > > > --- a/xen/arch/arm/setup.c > > > +++ b/xen/arch/arm/setup.c > > > @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, > > > /* Hide UART from DOM0 if we're using it */ > > > serial_endboot(); > > > + /* Ensure idle domain was not left privileged */ > > > + ASSERT(current->domain->is_privileged == false) ; > > > + > > > system_state = SYS_STATE_active; > > > create_domUs(); > > > > Hm, I think you want to use the permission promotion of the idle > > domain in create_domUs() likely? > > > > At which point the check should be after create_domUs, and it would > > seem that logically SYS_STATE_active should be set after creating the > > domUs. > > > > Also, FWIW, I'm not seeing this create_domUs() call in my context, > > maybe you have other patches on your queue? > I think the code is based on an old version of Xen (looks like 4.14). In newer > version create_domUs() is called before just before discard_initial_modules() > (see XSA-372 for the rationale). > > Daniel, can you please rebase this series to the latest staging? Yeah they should be rebased. I have done it so that I could test this approach as well, see attached. I also added a patch that calls: xsm_elevate_priv(current->domain); at the beginning of create_domUs, then calls: xsm_demote_priv(current->domain); at the end of create_domUs. With all that in place, dom0less+PV drivers works fine. Note that I don't know if we want to do this within create_domUs of if there is a better place, I was just trying to make sure everything works as expected.
On 3/31/22 08:46, Roger Pau Monné wrote: > On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote: >> It is now possible to promote the idle domain to privileged during setup. It >> is not desirable for the idle domain to still be privileged when moving into a >> running state. If the idle domain was elevated and not properly demoted, it is >> desirable to fail at this point. This commit adds an assert for both x86 and >> Arm just before transitioning to a running state that ensures the idle is not >> privileged. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> xen/arch/arm/setup.c | 3 +++ >> xen/arch/x86/setup.c | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 7968cee47d..3de394e946 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, >> /* Hide UART from DOM0 if we're using it */ >> serial_endboot(); >> >> + /* Ensure idle domain was not left privileged */ >> + ASSERT(current->domain->is_privileged == false) ; >> + >> system_state = SYS_STATE_active; >> >> create_domUs(); > > Hm, I think you want to use the permission promotion of the idle > domain in create_domUs() likely? Apologies, I cherry-picked this onto a branch of staging of what I thought was an up to date remote, but as Julien pointed out I was not. > At which point the check should be after create_domUs, and it would > seem that logically SYS_STATE_active should be set after creating the > domUs. > > Also, FWIW, I'm not seeing this create_domUs() call in my context, > maybe you have other patches on your queue? > >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 885919d5c3..b868463f83 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -589,6 +589,9 @@ static void noinline init_done(void) >> void *va; >> unsigned long start, end; >> >> + /* Ensure idle domain was not left privileged */ >> + ASSERT(current->domain->is_privileged == false) ; > ^ extra space. > > I think you could squash this patch with the previous one and also > squash it with a patch that actually makes use of the introduced > permission promotion functions (or at least in a patch series where > further patches make use the introduced functions). Ack, I can squash them together. v/r, dps
On 31.03.2022 01:05, Daniel P. Smith wrote: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -589,6 +589,9 @@ static void noinline init_done(void) > void *va; > unsigned long start, end; > > + /* Ensure idle domain was not left privileged */ > + ASSERT(current->domain->is_privileged == false) ; I think this should be stronger than ASSERT(); I'd recommend calling panic(). Also please don't compare against "true" or "false" - use ordinary boolean operations instead (here it would be "!current->domain->is_privileged"). Jan
On 4/5/22 04:26, Jan Beulich wrote: > On 31.03.2022 01:05, Daniel P. Smith wrote: >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -589,6 +589,9 @@ static void noinline init_done(void) >> void *va; >> unsigned long start, end; >> >> + /* Ensure idle domain was not left privileged */ >> + ASSERT(current->domain->is_privileged == false) ; > > I think this should be stronger than ASSERT(); I'd recommend calling > panic(). Also please don't compare against "true" or "false" - use > ordinary boolean operations instead (here it would be > "!current->domain->is_privileged"). Ack. v/r, dps
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 7968cee47d..3de394e946 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset, /* Hide UART from DOM0 if we're using it */ serial_endboot(); + /* Ensure idle domain was not left privileged */ + ASSERT(current->domain->is_privileged == false) ; + system_state = SYS_STATE_active; create_domUs(); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 885919d5c3..b868463f83 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -589,6 +589,9 @@ static void noinline init_done(void) void *va; unsigned long start, end; + /* Ensure idle domain was not left privileged */ + ASSERT(current->domain->is_privileged == false) ; + system_state = SYS_STATE_active; domain_unpause_by_systemcontroller(dom0);
It is now possible to promote the idle domain to privileged during setup. It is not desirable for the idle domain to still be privileged when moving into a running state. If the idle domain was elevated and not properly demoted, it is desirable to fail at this point. This commit adds an assert for both x86 and Arm just before transitioning to a running state that ensures the idle is not privileged. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/arm/setup.c | 3 +++ xen/arch/x86/setup.c | 3 +++ 2 files changed, 6 insertions(+)