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