From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 01/13] Export hypervisor symbols Date: Wed, 25 Sep 2013 10:03:23 -0400 Message-ID: <5242ED2B.8060803@oracle.com> References: <1379670132-1748-1-git-send-email-boris.ostrovsky@oracle.com> <1379670132-1748-2-git-send-email-boris.ostrovsky@oracle.com> <5242FE2902000078000F63F9@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VOpee-0001V7-Ex for xen-devel@lists.xenproject.org; Wed, 25 Sep 2013 14:01:24 +0000 In-Reply-To: <5242FE2902000078000F63F9@nat28.tlf.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: suravee.suthikulpanit@amd.com, George.Dunlap@eu.citrix.com, jacob.shin@amd.com, eddie.dong@intel.com, dietmar.hahn@ts.fujitsu.com, jun.nakajima@intel.com, xen-devel List-Id: xen-devel@lists.xenproject.org On 09/25/2013 09:15 AM, Jan Beulich wrote: >>>> On 20.09.13 at 11:42, Boris Ostrovsky wrote: >> + case XENPF_get_symbols: >> + { >> + ret = xensyms_read(&op->u.symdata); >> + if ( ret >= 0 && __copy_field_to_guest(u_xenpf_op, op, u.symdata) ) >> + ret = -EFAULT; >> + } >> + break; > This yields a positive return value if a symbol was found, 0 if none > was found, and negative on error. Can we avoid this non-standard > first aspect? We need to know on the caller side when EOF is reached. There is no good error code that I can see that would be appropriate here. ERANGE or ENFILE are the closest I can imagine but EOF is not really an error so I am not sure this would be the right thing. We could look at first byte of the string and see where it's a 0 but that's also somewhat non-standard. Encoding type or address as an invalid token also doesn't look nice. > >> + return 0; >> + >> + spin_lock(&symbols_mutex); >> + >> + if ( symdata->xen_symnum == 0 ) >> + next_offset = next_symbol = 0; >> + else if ( next_symbol != symdata->xen_symnum ) >> + /* Non-sequential access */ >> + next_offset = get_symbol_offset(symdata->xen_symnum); >> + >> + symdata->type = symbols_get_symbol_type(next_offset); >> + next_offset = symbols_expand_symbol(next_offset, symdata->name, >> + sizeof(symdata->name)); >> + symdata->address = symbols_offsets[symdata->xen_symnum] + SYMBOLS_ORIGIN; >> + >> + next_symbol = symdata->xen_symnum + 1; >> + >> + spin_unlock(&symbols_mutex); >> + >> + return strlen(symdata->name); > Altogether the changes you do appear to allow the nul terminator > to be written outside of the passed in array. Hence (a) you need > to avoid corrupting memory and (b) whether using strlen() here is > appropriate depends on how you deal with (a). Yes, I should have passed 'sizeof(symdata->name)-1' to symbols_expand_symbol() (I was thinking of strlen instead of sizeof) >> + >> +struct xenpf_symdata { >> + /* IN variables */ >> + uint64_t xen_symnum; >> + >> + /* OUT variables */ >> + uint64_t address; >> + uint64_t type; > "type" and "xen_symnum" could easily be less than 64 bits wide. I am trying to avoid 32-bit compatibility issues here. You will see sometimes unnecessary uint64_t in other structures well for the same reason. -boris