From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMZss-00054i-Jo for qemu-devel@nongnu.org; Tue, 04 Aug 2015 06:55:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMZsn-0005ux-Gi for qemu-devel@nongnu.org; Tue, 04 Aug 2015 06:55:50 -0400 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:57504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMZsm-0005s6-Tz for qemu-devel@nongnu.org; Tue, 04 Aug 2015 06:55:45 -0400 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 4 Aug 2015 16:25:41 +0530 Date: Tue, 4 Aug 2015 20:55:29 +1000 From: Gavin Shan Message-ID: <20150804105529.GA9980@gwshan> References: <1438557800-6947-1-git-send-email-gwshan@linux.vnet.ibm.com> <1438557800-6947-3-git-send-email-gwshan@linux.vnet.ibm.com> <20150803025109.GE15727@voom.fritz.box> <20150803033253.GA18268@gwshan> <55C0444A.2070206@ozlabs.ru> <20150804071644.GA16383@gwshan> <55C06872.8060309@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55C06872.8060309@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH RESEND v2 2/3] sPAPR: Support RTAS call ibm, {open, close}-errinjct Reply-To: Gavin Shan List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Gavin Shan , David Gibson On Tue, Aug 04, 2015 at 05:23:30PM +1000, Alexey Kardashevskiy wrote: >On 08/04/2015 05:16 PM, Gavin Shan wrote: >>On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote: >>>On 08/03/2015 01:32 PM, Gavin Shan wrote: >>>>On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote: >>>>>On Mon, Aug 03, 2015 at 09:23:19AM +1000, Gavin Shan wrote: >>>>>>The patch supports RTAS calls "ibm,{open,close}-errinjct" to >>>>>>manupliate the token, which is passed to RTAS call "ibm,errinjct" >>>>>>to indicate the valid context for error injection. Each VM is >>>>>>permitted to have only one token at once and we simply have one >>>>>>random number for that. >>>>>> >>>>>>Signed-off-by: Gavin Shan >>>>>>--- >>>>>> hw/ppc/spapr_rtas.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/hw/ppc/spapr.h | 9 ++++++- >>>>>> 2 files changed, 79 insertions(+), 1 deletion(-) >>>>>> >>>>>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >>>>>>index e99e25f..0a9c904 100644 >>>>>>--- a/hw/ppc/spapr_rtas.c >>>>>>+++ b/hw/ppc/spapr_rtas.c >>>>>>@@ -604,6 +604,73 @@ out: >>>>>> rtas_st(rets, 0, rc); >>>>>> } >>>>>> >>>>>>+static void rtas_ibm_open_errinjct(PowerPCCPU *cpu, >>>>>>+ sPAPRMachineState *spapr, >>>>>>+ uint32_t token, uint32_t nargs, >>>>>>+ target_ulong args, uint32_t nret, >>>>>>+ target_ulong rets) >>>>>>+{ >>>>>>+ int32_t ret; >>>>>>+ >>>>>>+ /* Sanity check on number of arguments */ >>>>>>+ if ((nargs != 0) || (nret != 2)) { >>>>>>+ ret = RTAS_OUT_PARAM_ERROR; >>>>>>+ goto out; >>>>>>+ } >>>>>>+ >>>>>>+ /* Check if we already had token */ >>>>>>+ if (spapr->errinjct_token) { >>>>>>+ ret = RTAS_OUT_TOKEN_OPENED; >>>>>>+ goto out; >>>>>>+ } >>>>>>+ >>>>>>+ /* Grab random number as token */ >>>>>>+ spapr->errinjct_token = random(); >>>>> >>>>>I don't quite understand the function of this token. Using random() >>>>>seems a very, very odd way of doing things. Is it supposed to be a >>>>>security thing? >>>>> >>>> >>>>Yes, the token is allocated by "ibm,open-errinjct". The token will be >>>>passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this >>>>perspecitve, the token owner is allowed to do error injection and it's >>>>for security. Apart from having random number as the token, is there >>>>better (fast) way to produce it? >>>> >>>>>>+ if (spapr->errinjct_token == 0) { >>>>>>+ ret = RTAS_OUT_BUSY; >>>>> >>>>>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY >>>>>for no particular reason. >>>>> >>>> >>>>Yes, "0" represents invalid token (not opened). Maybe here we can retry >>>>for a bit more like below. 0 returned from 10 successive random() would >>>>be rare. >>>> >>>> uint32_t retries; >>>> >>>> while (!spapr->errinjct_token && retries++ < 10) >>>> spapr->errinjct_token = random(); >>>> if (!spapr->errinjct_token) { >>>> ret = RTAS_OUT_BUSY; >>>> goto out; >>>> } >>> >>> >>>No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX >>>which is 0x7fffffff, you could do something simple like this: >>> >>>spapr->errinjct_token = (rand % 32767) + 1 >>> >> >>Good idea. I'll have it in next revision. >> >>Thanks, >>Gavin >> >>> >>>But for debugging purposes it makes more sense just to initialize it to 1 and >>>then increment it in every call of rtas_ibm_open_errinjct(). > > >Why rand() and not this? You do not protect against a guest attack by >limiting a number of the rtas calls so the token just needs to be unique and >that's it, and later in gdb is is going to be easier to trace these tokens if >need for this ever arises. > When calling rtas_ibm_close_errinjct(), the token (spapr->errinjct_token) will be zero'ed to indicate: the token has been closed. Alternatively, one statistics can be added if it's not expensive. However, I don't understand why we need trace the number of error injections that was ever raised. Could you please share the purpose about that? Thanks, Gavin