Re: [Qemu-devel] [PATCH RESEND v2 2/3] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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 <gwshan@linux.vnet.ibm.com>
>>>>>>---
>>>>>>  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