From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v6 01/19] common/symbols: Export hypervisor symbols to privileged guest Date: Fri, 16 May 2014 10:58:37 -0400 Message-ID: <5376279D.8000503@oracle.com> References: <1399996413-1998-1-git-send-email-boris.ostrovsky@oracle.com> <1399996413-1998-2-git-send-email-boris.ostrovsky@oracle.com> <5375E2D30200007800012E89@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5375E2D30200007800012E89@mail.emea.novell.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: Jan Beulich Cc: Tim Deegan , kevin.tian@intel.com, keir@xen.org, jun.nakajima@intel.com, andrew.cooper3@citrix.com, Ian Jackson , donald.d.dugger@intel.com, xen-devel@lists.xen.org, Ian Campbell , dietmar.hahn@ts.fujitsu.com, suravee.suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org On 05/16/2014 04:05 AM, Jan Beulich wrote: > >> + case XENPF_get_symbol: >> + { >> + char name[XEN_KSYM_NAME_LEN + 1]; > This is a fairly large object you place on the stack. Considering that > there's a lock being held already, did you consider making this static? > >> + XEN_GUEST_HANDLE(char) nameh; >> + >> + guest_from_compat_handle(nameh, op->u.symdata.name); >> + >> + ret = xensyms_read(&op->u.symdata.symnum, &op->u.symdata.type, >> + &op->u.symdata.address, name); >> + >> + if ( !ret && copy_to_guest(nameh, name, strlen(name)) ) > I think I commented on this earlier on already - you end up calling > strlen() on uninitialized data if xensyms_read() takes its second > early exit path. Additionally there's no way for the caller to > distinguish that success case from other success cases. " name[0] = '\0' " in xensyms_read() should address both of these concerns. > Considering that xensyms_read() is there only to implement this sub- > hypercall, I wonder whether you wouldn't be better of passing it the > handle, and have it do the copying. It's holding a lock itself, so the > static buffer could be placed there, along with the other statics it > already uses (which btw should go into the only function that make > use of them). I didn't want to expose xensyms_read() to any handles to keep it a purely "internal" (for the lack of a better term) routine. But if 'name' is to become static I don't want to introduce another lock (or move existing one up from xensyms_read()) so I guess I would have to pass the handle. > > And finally (I think I commented on this too earlier on) you still don't > have the caller pass in the size of the buffer it wants the symbol > copied to, and instead bake into the hypercall interface an arbitrary > (derived from current Xen internals) fixed name length. That's non- > extensible. So do you suggest passing in (and out) buffer size? > >> --- a/xen/arch/x86/x86_64/platform_hypercall.c >> +++ b/xen/arch/x86/x86_64/platform_hypercall.c >> @@ -32,6 +32,8 @@ CHECK_pf_pcpu_version; >> CHECK_pf_enter_acpi_sleep; >> #undef xen_pf_enter_acpi_sleep >> >> +#define xenpf_symdata compat_pf_symdata > Did you check that you really need this? There's no explicit instance of > the structure, so I would think it's not needed. Isn't this required for auto-building compat interfaces? > >> --- a/xen/include/public/platform.h >> +++ b/xen/include/public/platform.h >> @@ -527,6 +527,21 @@ struct xenpf_core_parking { >> typedef struct xenpf_core_parking xenpf_core_parking_t; >> DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t); >> >> +#define XENPF_get_symbol 61 >> +#define XEN_KSYM_NAME_LEN 127 >> +struct xenpf_symdata { >> + /* IN variables */ >> + uint32_t symnum; >> + >> + /* OUT variables */ >> + uint32_t type; > Do you really need 32 bits here? Looks like this really is a char, i.e. > you could leave 3 bytes for future extensibility. Yes, this is a char. I can replace it with a padded union. -boris