Message ID | 1438557800-6947-3-git-send-email-gwshan@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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? > + 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. > + goto out; > + } > + > + rtas_st(rets, 0, spapr->errinjct_token); > + ret = RTAS_OUT_SUCCESS; > +out: > + rtas_st(rets, 1, ret); > +} > + > +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, > + sPAPRMachineState *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, uint32_t nret, > + target_ulong rets) > +{ > + uint32_t open_token; > + int32_t ret; > + > + /* Sanity check on number of arguments */ > + if ((nargs != 1) || (nret != 1)) { > + ret = RTAS_OUT_PARAM_ERROR; > + goto out; > + } > + > + /* Check if we had opened token */ > + if (!spapr->errinjct_token) { > + ret = RTAS_OUT_CLOSE_ERROR; > + goto out; > + } > + > + /* Match with the passed token */ > + open_token = rtas_ld(args, 0); > + if (spapr->errinjct_token != open_token) { > + ret = RTAS_OUT_CLOSE_ERROR; > + goto out; > + } > + > + spapr->errinjct_token = 0; > + ret = RTAS_OUT_SUCCESS; > +out: > + rtas_st(rets, 0, ret); > +} > + > static struct rtas_call { > const char *name; > spapr_rtas_fn fn; > @@ -754,6 +821,10 @@ static void core_rtas_register_types(void) > rtas_get_sensor_state); > spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector", > rtas_ibm_configure_connector); > + spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct", > + rtas_ibm_open_errinjct); > + spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct", > + rtas_ibm_close_errinjct); > } > > type_init(core_rtas_register_types) > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index b6cb0d0..30d9854 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -73,6 +73,9 @@ struct sPAPRMachineState { > int htab_fd; > bool htab_fd_stale; > > + /* Error injection token */ > + uint32_t errinjct_token; > + > /* RTAS state */ > QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; > > @@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); > #define RTAS_OUT_BUSY -2 > #define RTAS_OUT_PARAM_ERROR -3 > #define RTAS_OUT_NOT_SUPPORTED -3 > +#define RTAS_OUT_TOKEN_OPENED -4 > +#define RTAS_OUT_CLOSE_ERROR -4 > #define RTAS_OUT_NOT_AUTHORIZED -9002 > > /* RTAS tokens */ > @@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); > #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23) > #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24) > #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25) > +#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26) > +#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27) > > -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26) > +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28) > > /* RTAS ibm,get-system-parameter token values */ > #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
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; } >> + goto out; >> + } >> + >> + rtas_st(rets, 0, spapr->errinjct_token); >> + ret = RTAS_OUT_SUCCESS; >> +out: >> + rtas_st(rets, 1, ret); >> +} >> + >> +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, >> + sPAPRMachineState *spapr, >> + uint32_t token, uint32_t nargs, >> + target_ulong args, uint32_t nret, >> + target_ulong rets) >> +{ >> + uint32_t open_token; >> + int32_t ret; >> + >> + /* Sanity check on number of arguments */ >> + if ((nargs != 1) || (nret != 1)) { >> + ret = RTAS_OUT_PARAM_ERROR; >> + goto out; >> + } >> + >> + /* Check if we had opened token */ >> + if (!spapr->errinjct_token) { >> + ret = RTAS_OUT_CLOSE_ERROR; >> + goto out; >> + } >> + >> + /* Match with the passed token */ >> + open_token = rtas_ld(args, 0); >> + if (spapr->errinjct_token != open_token) { >> + ret = RTAS_OUT_CLOSE_ERROR; >> + goto out; >> + } >> + >> + spapr->errinjct_token = 0; >> + ret = RTAS_OUT_SUCCESS; >> +out: >> + rtas_st(rets, 0, ret); >> +} >> + >> static struct rtas_call { >> const char *name; >> spapr_rtas_fn fn; >> @@ -754,6 +821,10 @@ static void core_rtas_register_types(void) >> rtas_get_sensor_state); >> spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector", >> rtas_ibm_configure_connector); >> + spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct", >> + rtas_ibm_open_errinjct); >> + spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct", >> + rtas_ibm_close_errinjct); >> } >> >> type_init(core_rtas_register_types) >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index b6cb0d0..30d9854 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -73,6 +73,9 @@ struct sPAPRMachineState { >> int htab_fd; >> bool htab_fd_stale; >> >> + /* Error injection token */ >> + uint32_t errinjct_token; >> + >> /* RTAS state */ >> QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; >> >> @@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >> #define RTAS_OUT_BUSY -2 >> #define RTAS_OUT_PARAM_ERROR -3 >> #define RTAS_OUT_NOT_SUPPORTED -3 >> +#define RTAS_OUT_TOKEN_OPENED -4 >> +#define RTAS_OUT_CLOSE_ERROR -4 >> #define RTAS_OUT_NOT_AUTHORIZED -9002 >> >> /* RTAS tokens */ >> @@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >> #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23) >> #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24) >> #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25) >> +#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26) >> +#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27) >> >> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26) >> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28) >> >> /* RTAS ibm,get-system-parameter token values */ >> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 Thanks, Gavin
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 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(). > >>> + goto out; >>> + } >>> + >>> + rtas_st(rets, 0, spapr->errinjct_token); >>> + ret = RTAS_OUT_SUCCESS; >>> +out: >>> + rtas_st(rets, 1, ret); >>> +} >>> + >>> +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, >>> + sPAPRMachineState *spapr, >>> + uint32_t token, uint32_t nargs, >>> + target_ulong args, uint32_t nret, >>> + target_ulong rets) >>> +{ >>> + uint32_t open_token; >>> + int32_t ret; >>> + >>> + /* Sanity check on number of arguments */ >>> + if ((nargs != 1) || (nret != 1)) { >>> + ret = RTAS_OUT_PARAM_ERROR; >>> + goto out; >>> + } >>> + >>> + /* Check if we had opened token */ >>> + if (!spapr->errinjct_token) { >>> + ret = RTAS_OUT_CLOSE_ERROR; >>> + goto out; >>> + } >>> + >>> + /* Match with the passed token */ >>> + open_token = rtas_ld(args, 0); >>> + if (spapr->errinjct_token != open_token) { >>> + ret = RTAS_OUT_CLOSE_ERROR; >>> + goto out; >>> + } >>> + >>> + spapr->errinjct_token = 0; >>> + ret = RTAS_OUT_SUCCESS; >>> +out: >>> + rtas_st(rets, 0, ret); >>> +} >>> + >>> static struct rtas_call { >>> const char *name; >>> spapr_rtas_fn fn; >>> @@ -754,6 +821,10 @@ static void core_rtas_register_types(void) >>> rtas_get_sensor_state); >>> spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector", >>> rtas_ibm_configure_connector); >>> + spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct", >>> + rtas_ibm_open_errinjct); >>> + spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct", >>> + rtas_ibm_close_errinjct); >>> } >>> >>> type_init(core_rtas_register_types) >>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>> index b6cb0d0..30d9854 100644 >>> --- a/include/hw/ppc/spapr.h >>> +++ b/include/hw/ppc/spapr.h >>> @@ -73,6 +73,9 @@ struct sPAPRMachineState { >>> int htab_fd; >>> bool htab_fd_stale; >>> >>> + /* Error injection token */ >>> + uint32_t errinjct_token; >>> + >>> /* RTAS state */ >>> QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; >>> >>> @@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >>> #define RTAS_OUT_BUSY -2 >>> #define RTAS_OUT_PARAM_ERROR -3 >>> #define RTAS_OUT_NOT_SUPPORTED -3 >>> +#define RTAS_OUT_TOKEN_OPENED -4 >>> +#define RTAS_OUT_CLOSE_ERROR -4 >>> #define RTAS_OUT_NOT_AUTHORIZED -9002 >>> >>> /* RTAS tokens */ >>> @@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >>> #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23) >>> #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24) >>> #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25) >>> +#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26) >>> +#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27) >>> >>> -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26) >>> +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28) >>> >>> /* RTAS ibm,get-system-parameter token values */ >>> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 > > Thanks, > Gavin >
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(). > > >> >>>>+ goto out; >>>>+ } >>>>+ >>>>+ rtas_st(rets, 0, spapr->errinjct_token); >>>>+ ret = RTAS_OUT_SUCCESS; >>>>+out: >>>>+ rtas_st(rets, 1, ret); >>>>+} >>>>+ >>>>+static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, >>>>+ sPAPRMachineState *spapr, >>>>+ uint32_t token, uint32_t nargs, >>>>+ target_ulong args, uint32_t nret, >>>>+ target_ulong rets) >>>>+{ >>>>+ uint32_t open_token; >>>>+ int32_t ret; >>>>+ >>>>+ /* Sanity check on number of arguments */ >>>>+ if ((nargs != 1) || (nret != 1)) { >>>>+ ret = RTAS_OUT_PARAM_ERROR; >>>>+ goto out; >>>>+ } >>>>+ >>>>+ /* Check if we had opened token */ >>>>+ if (!spapr->errinjct_token) { >>>>+ ret = RTAS_OUT_CLOSE_ERROR; >>>>+ goto out; >>>>+ } >>>>+ >>>>+ /* Match with the passed token */ >>>>+ open_token = rtas_ld(args, 0); >>>>+ if (spapr->errinjct_token != open_token) { >>>>+ ret = RTAS_OUT_CLOSE_ERROR; >>>>+ goto out; >>>>+ } >>>>+ >>>>+ spapr->errinjct_token = 0; >>>>+ ret = RTAS_OUT_SUCCESS; >>>>+out: >>>>+ rtas_st(rets, 0, ret); >>>>+} >>>>+ >>>> static struct rtas_call { >>>> const char *name; >>>> spapr_rtas_fn fn; >>>>@@ -754,6 +821,10 @@ static void core_rtas_register_types(void) >>>> rtas_get_sensor_state); >>>> spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector", >>>> rtas_ibm_configure_connector); >>>>+ spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct", >>>>+ rtas_ibm_open_errinjct); >>>>+ spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct", >>>>+ rtas_ibm_close_errinjct); >>>> } >>>> >>>> type_init(core_rtas_register_types) >>>>diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>>>index b6cb0d0..30d9854 100644 >>>>--- a/include/hw/ppc/spapr.h >>>>+++ b/include/hw/ppc/spapr.h >>>>@@ -73,6 +73,9 @@ struct sPAPRMachineState { >>>> int htab_fd; >>>> bool htab_fd_stale; >>>> >>>>+ /* Error injection token */ >>>>+ uint32_t errinjct_token; >>>>+ >>>> /* RTAS state */ >>>> QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; >>>> >>>>@@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >>>> #define RTAS_OUT_BUSY -2 >>>> #define RTAS_OUT_PARAM_ERROR -3 >>>> #define RTAS_OUT_NOT_SUPPORTED -3 >>>>+#define RTAS_OUT_TOKEN_OPENED -4 >>>>+#define RTAS_OUT_CLOSE_ERROR -4 >>>> #define RTAS_OUT_NOT_AUTHORIZED -9002 >>>> >>>> /* RTAS tokens */ >>>>@@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); >>>> #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23) >>>> #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24) >>>> #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25) >>>>+#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26) >>>>+#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27) >>>> >>>>-#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26) >>>>+#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28) >>>> >>>> /* RTAS ibm,get-system-parameter token values */ >>>> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 >> >>Thanks, >>Gavin >> > > >-- >Alexey >
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.
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
On Tue, Aug 04, 2015 at 08:55:29PM +1000, Gavin Shan wrote: > 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? I understand that, but using a token from random() just doesn't make sense. 1) If this is just to prevent accidental multiple users of the device, then an incrementing counter is simpler, easier to understand and just as good. 2) If this is supposed to securely prevent other users from controlling device then a) there's no point, it requires privilege in the guest anyway, and b) you'd have to use a secure random number source, not a pseudo-rng like random(). Oh, also, you haven't added the token to the migration stream, which means a migration would silently close the token.
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(); + if (spapr->errinjct_token == 0) { + ret = RTAS_OUT_BUSY; + goto out; + } + + rtas_st(rets, 0, spapr->errinjct_token); + ret = RTAS_OUT_SUCCESS; +out: + rtas_st(rets, 1, ret); +} + +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu, + sPAPRMachineState *spapr, + uint32_t token, uint32_t nargs, + target_ulong args, uint32_t nret, + target_ulong rets) +{ + uint32_t open_token; + int32_t ret; + + /* Sanity check on number of arguments */ + if ((nargs != 1) || (nret != 1)) { + ret = RTAS_OUT_PARAM_ERROR; + goto out; + } + + /* Check if we had opened token */ + if (!spapr->errinjct_token) { + ret = RTAS_OUT_CLOSE_ERROR; + goto out; + } + + /* Match with the passed token */ + open_token = rtas_ld(args, 0); + if (spapr->errinjct_token != open_token) { + ret = RTAS_OUT_CLOSE_ERROR; + goto out; + } + + spapr->errinjct_token = 0; + ret = RTAS_OUT_SUCCESS; +out: + rtas_st(rets, 0, ret); +} + static struct rtas_call { const char *name; spapr_rtas_fn fn; @@ -754,6 +821,10 @@ static void core_rtas_register_types(void) rtas_get_sensor_state); spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector", rtas_ibm_configure_connector); + spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct", + rtas_ibm_open_errinjct); + spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct", + rtas_ibm_close_errinjct); } type_init(core_rtas_register_types) diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index b6cb0d0..30d9854 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -73,6 +73,9 @@ struct sPAPRMachineState { int htab_fd; bool htab_fd_stale; + /* Error injection token */ + uint32_t errinjct_token; + /* RTAS state */ QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list; @@ -412,6 +415,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); #define RTAS_OUT_BUSY -2 #define RTAS_OUT_PARAM_ERROR -3 #define RTAS_OUT_NOT_SUPPORTED -3 +#define RTAS_OUT_TOKEN_OPENED -4 +#define RTAS_OUT_CLOSE_ERROR -4 #define RTAS_OUT_NOT_AUTHORIZED -9002 /* RTAS tokens */ @@ -455,8 +460,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi); #define RTAS_IBM_SET_SLOT_RESET (RTAS_TOKEN_BASE + 0x23) #define RTAS_IBM_CONFIGURE_PE (RTAS_TOKEN_BASE + 0x24) #define RTAS_IBM_SLOT_ERROR_DETAIL (RTAS_TOKEN_BASE + 0x25) +#define RTAS_IBM_OPEN_ERRINJCT (RTAS_TOKEN_BASE + 0x26) +#define RTAS_IBM_CLOSE_ERRINJCT (RTAS_TOKEN_BASE + 0x27) -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x26) +#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x28) /* RTAS ibm,get-system-parameter token values */ #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20
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(-)