* [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers @ 2014-05-19 18:37 Jason Andryuk 2014-05-20 9:53 ` Andrew Cooper 0 siblings, 1 reply; 4+ messages in thread From: Jason Andryuk @ 2014-05-19 18:37 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, Jason Andryuk, Ian Jackson, Ian Campbell, Stefano Stabellini xc_domain_resume() expects the guest to be in state SHUTDOWN_suspend. However, nothing verifies the state before modify_returncode() modifies the domain's registers. This will crash guest processes or the kernel itself. This can be demonstrated with `LIBXL_SAVE_HELPER=/bin/false xl migrate`. Signed-off-by: Jason Andryuk <andryuk@aero.org> --- Changes since RFC: - Return -1 from modify_returncode - Set errno to EINVAL --- tools/libxc/xc_resume.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c index 18b4818..2163ad9 100644 --- a/tools/libxc/xc_resume.c +++ b/tools/libxc/xc_resume.c @@ -39,6 +39,13 @@ static int modify_returncode(xc_interface *xch, uint32_t domid) return -1; } + if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) ) + { + ERROR("Domain not in suspended state"); + errno = EINVAL; + return -1; + } + if ( info.hvm ) { /* HVM guests without PV drivers have no return code to modify. */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers 2014-05-19 18:37 [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers Jason Andryuk @ 2014-05-20 9:53 ` Andrew Cooper 2014-05-20 12:51 ` Jason Andryuk 0 siblings, 1 reply; 4+ messages in thread From: Andrew Cooper @ 2014-05-20 9:53 UTC (permalink / raw) To: Jason Andryuk; +Cc: Stefano Stabellini, Ian Jackson, Ian Campbell, xen-devel On 19/05/14 19:37, Jason Andryuk wrote: > xc_domain_resume() expects the guest to be in state SHUTDOWN_suspend. > However, nothing verifies the state before modify_returncode() modifies > the domain's registers. This will crash guest processes or the kernel > itself. > > This can be demonstrated with `LIBXL_SAVE_HELPER=/bin/false xl migrate`. > > Signed-off-by: Jason Andryuk <andryuk@aero.org> > --- > Changes since RFC: > - Return -1 from modify_returncode > - Set errno to EINVAL > --- > tools/libxc/xc_resume.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c > index 18b4818..2163ad9 100644 > --- a/tools/libxc/xc_resume.c > +++ b/tools/libxc/xc_resume.c > @@ -39,6 +39,13 @@ static int modify_returncode(xc_interface *xch, uint32_t domid) > return -1; > } Having looked at this more closely, there is also a bug in the hunk above. xc_domain_getinfo() being the 'special' function that it is doesn't always return the domain specified. If the given domid doesn't exist in the system, you will get back the first domain with a higher domid. The only safe way to use it is if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 || info.domid != domid ) { error... } > > + if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) ) > + { > + ERROR("Domain not in suspended state"); ERROR("Dom %d not suspended: (shutdown %d, reason %d)", domid, info.shutdown, info.shutdown_reason)); This way, someone unexpectedly finding this error message gets slightly more information than "something wasn't how I expected it to be". ~Andrew > + errno = EINVAL; > + return -1; > + } > + > if ( info.hvm ) > { > /* HVM guests without PV drivers have no return code to modify. */ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers 2014-05-20 9:53 ` Andrew Cooper @ 2014-05-20 12:51 ` Jason Andryuk 2014-05-20 13:16 ` Andrew Cooper 0 siblings, 1 reply; 4+ messages in thread From: Jason Andryuk @ 2014-05-20 12:51 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini On 5/20/2014 5:53 AM, Andrew Cooper wrote: > On 19/05/14 19:37, Jason Andryuk wrote: >> xc_domain_resume() expects the guest to be in state SHUTDOWN_suspend. >> However, nothing verifies the state before modify_returncode() modifies >> the domain's registers. This will crash guest processes or the kernel >> itself. >> >> This can be demonstrated with `LIBXL_SAVE_HELPER=/bin/false xl migrate`. >> >> Signed-off-by: Jason Andryuk <andryuk@aero.org> >> --- >> Changes since RFC: >> - Return -1 from modify_returncode >> - Set errno to EINVAL >> --- >> tools/libxc/xc_resume.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c >> index 18b4818..2163ad9 100644 >> --- a/tools/libxc/xc_resume.c >> +++ b/tools/libxc/xc_resume.c >> @@ -39,6 +39,13 @@ static int modify_returncode(xc_interface *xch, uint32_t domid) >> return -1; >> } > > Having looked at this more closely, there is also a bug in the hunk above. > > xc_domain_getinfo() being the 'special' function that it is doesn't > always return the domain specified. If the given domid doesn't exist in > the system, you will get back the first domain with a higher domid. > > The only safe way to use it is > > if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 || > info.domid != domid ) > { > error... > } I am ok making this change; I wasn't aware of the quirks of the function. This patch copy & pasted the check from xc_domain_save.c:suspend_and_state(). That and other locations throughout libxc fail to compare info.domid to the requested domid, so there are lots of places that should be fixed up. Maybe a wrapper xc_domain_getinfo_one(xch, domid, &info) that checks the domid would be useful? >> >> + if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) ) >> + { >> + ERROR("Domain not in suspended state"); > > ERROR("Dom %d not suspended: (shutdown %d, reason %d)", domid, > info.shutdown, info.shutdown_reason)); > > This way, someone unexpectedly finding this error message gets slightly > more information than "something wasn't how I expected it to be". Yes, this is more informative. suspend_and_state could also be updated to the same message. -Jason ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers 2014-05-20 12:51 ` Jason Andryuk @ 2014-05-20 13:16 ` Andrew Cooper 0 siblings, 0 replies; 4+ messages in thread From: Andrew Cooper @ 2014-05-20 13:16 UTC (permalink / raw) To: Jason Andryuk; +Cc: xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini On 20/05/14 13:51, Jason Andryuk wrote: > On 5/20/2014 5:53 AM, Andrew Cooper wrote: >> On 19/05/14 19:37, Jason Andryuk wrote: >>> xc_domain_resume() expects the guest to be in state SHUTDOWN_suspend. >>> However, nothing verifies the state before modify_returncode() modifies >>> the domain's registers. This will crash guest processes or the kernel >>> itself. >>> >>> This can be demonstrated with `LIBXL_SAVE_HELPER=/bin/false xl migrate`. >>> >>> Signed-off-by: Jason Andryuk <andryuk@aero.org> >>> --- >>> Changes since RFC: >>> - Return -1 from modify_returncode >>> - Set errno to EINVAL >>> --- >>> tools/libxc/xc_resume.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c >>> index 18b4818..2163ad9 100644 >>> --- a/tools/libxc/xc_resume.c >>> +++ b/tools/libxc/xc_resume.c >>> @@ -39,6 +39,13 @@ static int modify_returncode(xc_interface *xch, uint32_t domid) >>> return -1; >>> } >> Having looked at this more closely, there is also a bug in the hunk above. >> >> xc_domain_getinfo() being the 'special' function that it is doesn't >> always return the domain specified. If the given domid doesn't exist in >> the system, you will get back the first domain with a higher domid. >> >> The only safe way to use it is >> >> if ( xc_domain_getinfo(xch, domid, 1, &info) != 1 || >> info.domid != domid ) >> { >> error... >> } > I am ok making this change; I wasn't aware of the quirks of the function. > > This patch copy & pasted the check from xc_domain_save.c:suspend_and_state(). That and other locations throughout libxc fail to compare info.domid to the requested domid, so there are lots of places that should be fixed up. Maybe a wrapper xc_domain_getinfo_one(xch, domid, &info) that checks the domid would be useful? It certainly would be useful, although probably as a separate patch. > >>> >>> + if ( !info.shutdown || (info.shutdown_reason != SHUTDOWN_suspend) ) >>> + { >>> + ERROR("Domain not in suspended state"); >> ERROR("Dom %d not suspended: (shutdown %d, reason %d)", domid, >> info.shutdown, info.shutdown_reason)); >> >> This way, someone unexpectedly finding this error message gets slightly >> more information than "something wasn't how I expected it to be". > Yes, this is more informative. suspend_and_state could also be updated to the same message. > > -Jason Probably no need to worry about suspend_and_state(). It is about to disappear with the new migration protocol work. ~Andrew ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-20 13:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-19 18:37 [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers Jason Andryuk 2014-05-20 9:53 ` Andrew Cooper 2014-05-20 12:51 ` Jason Andryuk 2014-05-20 13:16 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.