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 <gws...@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
