From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Andryuk Subject: Re: [PATCH] libxc: Protect xc_domain_resume from clobbering domain registers Date: Tue, 20 May 2014 08:51:40 -0400 Message-ID: <537B4FDC.6060200@aero.org> References: <1400524672-6141-1-git-send-email-andryuk@aero.org> <537B262E.20600@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <537B262E.20600@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: xen-devel@lists.xen.org, Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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