| Message ID | 20200910093655.255774-4-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 10/09/2020 11.36, Collin Walling wrote: > The header contained within the SCCB passed to the SCLP service call > contains the actual length of the SCCB. Instead of allocating a static > 4K size for the work sccb, let's allow for a variable size determined > by the value in the header. The proper checks are already in place to > ensure the SCCB length is sufficent to store a full response and that > the length does not cross any explicitly-set boundaries. > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > hw/s390x/event-facility.c | 2 +- > hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- > include/hw/s390x/sclp.h | 2 +- > 3 files changed, 35 insertions(+), 27 deletions(-) > > diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c > index 645b4080c5..ed92ce510d 100644 > --- a/hw/s390x/event-facility.c > +++ b/hw/s390x/event-facility.c > @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, > > event_buf = &red->ebh; > event_buf->length = 0; > - slen = sizeof(sccb->data); > + slen = sccb_data_len(sccb); > > rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 69a8724dc7..cb8e2e8ec3 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, > { > SCLPDevice *sclp = get_sclp_device(); > SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); > - SCCB work_sccb; > - hwaddr sccb_len = sizeof(SCCB); > + SCCBHeader header; > + SCCB *work_sccb; I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to worry about doing the g_free() later. > - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); > + > + work_sccb = g_malloc0(header.length); Please use be16_to_cpu(header.length) here as well. > + s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb, > + be16_to_cpu(header.length)); > > if (!sclp_command_code_valid(code)) { > - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > goto out_write; > } > > - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { > - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { > + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > goto out_write; > } > > - sclp_c->execute(sclp, &work_sccb, code); > + sclp_c->execute(sclp, work_sccb, code); > out_write: > - s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, > - be16_to_cpu(work_sccb.h.length)); > + s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb, > + be16_to_cpu(work_sccb->h.length)); > + g_free(work_sccb); > sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); > return 0; > } > @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) > { > SCLPDevice *sclp = get_sclp_device(); > SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); > - SCCB work_sccb; > - > - hwaddr sccb_len = sizeof(SCCB); > + SCCBHeader header; > + SCCB *work_sccb; Maybe g_autofree again? > /* first some basic checks on program checks */ > if (env->psw.mask & PSW_MASK_PSTATE) { > @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) > return -PGM_SPECIFICATION; > } > > + /* the header contains the actual length of the sccb */ > + cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader)); > + > + /* Valid sccb sizes */ > + if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) { > + return -PGM_SPECIFICATION; > + } > + > /* > * we want to work on a private copy of the sccb, to prevent guests > * from playing dirty tricks by modifying the memory content after > * the host has checked the values > */ > - cpu_physical_memory_read(sccb, &work_sccb, sccb_len); > - > - /* Valid sccb sizes */ > - if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { > - return -PGM_SPECIFICATION; > - } > + work_sccb = g_malloc0(header.length); Needs be16_to_cpu again. > + cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length)); > > if (!sclp_command_code_valid(code)) { > - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > goto out_write; > } > > - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { > - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { > + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); > goto out_write; > } > > - sclp_c->execute(sclp, &work_sccb, code); > + sclp_c->execute(sclp, work_sccb, code); > out_write: > - cpu_physical_memory_write(sccb, &work_sccb, > - be16_to_cpu(work_sccb.h.length)); > - > + cpu_physical_memory_write(sccb, work_sccb, > + be16_to_cpu(work_sccb->h.length)); > + g_free(work_sccb); > sclp_c->service_interrupt(sclp, sccb); > > return 0; Thomas
On 9/10/20 1:50 PM, Thomas Huth wrote: > On 10/09/2020 11.36, Collin Walling wrote: >> The header contained within the SCCB passed to the SCLP service call >> contains the actual length of the SCCB. Instead of allocating a static >> 4K size for the work sccb, let's allow for a variable size determined >> by the value in the header. The proper checks are already in place to >> ensure the SCCB length is sufficent to store a full response and that >> the length does not cross any explicitly-set boundaries. >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> hw/s390x/event-facility.c | 2 +- >> hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- >> include/hw/s390x/sclp.h | 2 +- >> 3 files changed, 35 insertions(+), 27 deletions(-) >> >> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c >> index 645b4080c5..ed92ce510d 100644 >> --- a/hw/s390x/event-facility.c >> +++ b/hw/s390x/event-facility.c >> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, >> >> event_buf = &red->ebh; >> event_buf->length = 0; >> - slen = sizeof(sccb->data); >> + slen = sccb_data_len(sccb); >> >> rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 69a8724dc7..cb8e2e8ec3 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >> { >> SCLPDevice *sclp = get_sclp_device(); >> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >> - SCCB work_sccb; >> - hwaddr sccb_len = sizeof(SCCB); >> + SCCBHeader header; >> + SCCB *work_sccb; > > I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to > worry about doing the g_free() later. Can do. > >> - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); >> + >> + work_sccb = g_malloc0(header.length); > > Please use be16_to_cpu(header.length) here as well. Good catch, thanks! > >> + s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb, >> + be16_to_cpu(header.length)); >> >> if (!sclp_command_code_valid(code)) { >> - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> goto out_write; >> } >> >> - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { >> - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { >> + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> goto out_write; >> } >> >> - sclp_c->execute(sclp, &work_sccb, code); >> + sclp_c->execute(sclp, work_sccb, code); >> out_write: >> - s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, >> - be16_to_cpu(work_sccb.h.length)); >> + s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb, >> + be16_to_cpu(work_sccb->h.length)); >> + g_free(work_sccb); >> sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); >> return 0; >> } >> @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) >> { >> SCLPDevice *sclp = get_sclp_device(); >> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >> - SCCB work_sccb; >> - >> - hwaddr sccb_len = sizeof(SCCB); >> + SCCBHeader header; >> + SCCB *work_sccb; > > Maybe g_autofree again? > >> /* first some basic checks on program checks */ >> if (env->psw.mask & PSW_MASK_PSTATE) { >> @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) >> return -PGM_SPECIFICATION; >> } >> >> + /* the header contains the actual length of the sccb */ >> + cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader)); >> + >> + /* Valid sccb sizes */ >> + if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) { >> + return -PGM_SPECIFICATION; >> + } >> + >> /* >> * we want to work on a private copy of the sccb, to prevent guests >> * from playing dirty tricks by modifying the memory content after >> * the host has checked the values >> */ >> - cpu_physical_memory_read(sccb, &work_sccb, sccb_len); >> - >> - /* Valid sccb sizes */ >> - if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { >> - return -PGM_SPECIFICATION; >> - } >> + work_sccb = g_malloc0(header.length); > > Needs be16_to_cpu again. > >> + cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length)); >> >> if (!sclp_command_code_valid(code)) { >> - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> goto out_write; >> } >> >> - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { >> - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { >> + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); >> goto out_write; >> } >> >> - sclp_c->execute(sclp, &work_sccb, code); >> + sclp_c->execute(sclp, work_sccb, code); >> out_write: >> - cpu_physical_memory_write(sccb, &work_sccb, >> - be16_to_cpu(work_sccb.h.length)); >> - >> + cpu_physical_memory_write(sccb, work_sccb, >> + be16_to_cpu(work_sccb->h.length)); >> + g_free(work_sccb); >> sclp_c->service_interrupt(sclp, sccb); >> >> return 0; > > Thomas > > Thanks, Thomas!
On 9/10/20 1:56 PM, Collin Walling wrote: > On 9/10/20 1:50 PM, Thomas Huth wrote: >> On 10/09/2020 11.36, Collin Walling wrote: >>> The header contained within the SCCB passed to the SCLP service call >>> contains the actual length of the SCCB. Instead of allocating a static >>> 4K size for the work sccb, let's allow for a variable size determined >>> by the value in the header. The proper checks are already in place to >>> ensure the SCCB length is sufficent to store a full response and that >>> the length does not cross any explicitly-set boundaries. >>> >>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>> --- >>> hw/s390x/event-facility.c | 2 +- >>> hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- >>> include/hw/s390x/sclp.h | 2 +- >>> 3 files changed, 35 insertions(+), 27 deletions(-) >>> >>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c >>> index 645b4080c5..ed92ce510d 100644 >>> --- a/hw/s390x/event-facility.c >>> +++ b/hw/s390x/event-facility.c >>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, >>> >>> event_buf = &red->ebh; >>> event_buf->length = 0; >>> - slen = sizeof(sccb->data); >>> + slen = sccb_data_len(sccb); >>> >>> rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; >>> >>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>> index 69a8724dc7..cb8e2e8ec3 100644 >>> --- a/hw/s390x/sclp.c >>> +++ b/hw/s390x/sclp.c >>> @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >>> { >>> SCLPDevice *sclp = get_sclp_device(); >>> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >>> - SCCB work_sccb; >>> - hwaddr sccb_len = sizeof(SCCB); >>> + SCCBHeader header; >>> + SCCB *work_sccb; >> >> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to >> worry about doing the g_free() later. > > Can do. > >> >>> - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); >>> + >>> + work_sccb = g_malloc0(header.length); >> >> Please use be16_to_cpu(header.length) here as well. > > Good catch, thanks! > Shouldn't the mallocs use cpu_to_be16 instead since the header length was read in from a cpu? [...]
On 11/09/2020 20.16, Collin Walling wrote: > On 9/10/20 1:56 PM, Collin Walling wrote: >> On 9/10/20 1:50 PM, Thomas Huth wrote: >>> On 10/09/2020 11.36, Collin Walling wrote: >>>> The header contained within the SCCB passed to the SCLP service call >>>> contains the actual length of the SCCB. Instead of allocating a static >>>> 4K size for the work sccb, let's allow for a variable size determined >>>> by the value in the header. The proper checks are already in place to >>>> ensure the SCCB length is sufficent to store a full response and that >>>> the length does not cross any explicitly-set boundaries. >>>> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>> --- >>>> hw/s390x/event-facility.c | 2 +- >>>> hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- >>>> include/hw/s390x/sclp.h | 2 +- >>>> 3 files changed, 35 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c >>>> index 645b4080c5..ed92ce510d 100644 >>>> --- a/hw/s390x/event-facility.c >>>> +++ b/hw/s390x/event-facility.c >>>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, >>>> >>>> event_buf = &red->ebh; >>>> event_buf->length = 0; >>>> - slen = sizeof(sccb->data); >>>> + slen = sccb_data_len(sccb); >>>> >>>> rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; >>>> >>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>>> index 69a8724dc7..cb8e2e8ec3 100644 >>>> --- a/hw/s390x/sclp.c >>>> +++ b/hw/s390x/sclp.c >>>> @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >>>> { >>>> SCLPDevice *sclp = get_sclp_device(); >>>> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >>>> - SCCB work_sccb; >>>> - hwaddr sccb_len = sizeof(SCCB); >>>> + SCCBHeader header; >>>> + SCCB *work_sccb; >>> >>> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to >>> worry about doing the g_free() later. >> >> Can do. >> >>> >>>> - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >>>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); >>>> + >>>> + work_sccb = g_malloc0(header.length); >>> >>> Please use be16_to_cpu(header.length) here as well. >> >> Good catch, thanks! >> > > Shouldn't the mallocs use cpu_to_be16 instead since the header length > was read in from a cpu? Now you confuse me ... s390x is big endian, so to get a usable value, we have to convert big-endian to the host byte order, not the other way round, don't we? Thomas
On 9/12/20 2:28 AM, Thomas Huth wrote: > On 11/09/2020 20.16, Collin Walling wrote: >> On 9/10/20 1:56 PM, Collin Walling wrote: >>> On 9/10/20 1:50 PM, Thomas Huth wrote: >>>> On 10/09/2020 11.36, Collin Walling wrote: >>>>> The header contained within the SCCB passed to the SCLP service call >>>>> contains the actual length of the SCCB. Instead of allocating a static >>>>> 4K size for the work sccb, let's allow for a variable size determined >>>>> by the value in the header. The proper checks are already in place to >>>>> ensure the SCCB length is sufficent to store a full response and that >>>>> the length does not cross any explicitly-set boundaries. >>>>> >>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>>> --- >>>>> hw/s390x/event-facility.c | 2 +- >>>>> hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- >>>>> include/hw/s390x/sclp.h | 2 +- >>>>> 3 files changed, 35 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c >>>>> index 645b4080c5..ed92ce510d 100644 >>>>> --- a/hw/s390x/event-facility.c >>>>> +++ b/hw/s390x/event-facility.c >>>>> @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, >>>>> >>>>> event_buf = &red->ebh; >>>>> event_buf->length = 0; >>>>> - slen = sizeof(sccb->data); >>>>> + slen = sccb_data_len(sccb); >>>>> >>>>> rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; >>>>> >>>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>>>> index 69a8724dc7..cb8e2e8ec3 100644 >>>>> --- a/hw/s390x/sclp.c >>>>> +++ b/hw/s390x/sclp.c >>>>> @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, >>>>> { >>>>> SCLPDevice *sclp = get_sclp_device(); >>>>> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >>>>> - SCCB work_sccb; >>>>> - hwaddr sccb_len = sizeof(SCCB); >>>>> + SCCBHeader header; >>>>> + SCCB *work_sccb; >>>> >>>> I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to >>>> worry about doing the g_free() later. >>> >>> Can do. >>> >>>> >>>>> - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >>>>> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); >>>>> + >>>>> + work_sccb = g_malloc0(header.length); >>>> >>>> Please use be16_to_cpu(header.length) here as well. >>> >>> Good catch, thanks! >>> >> >> Shouldn't the mallocs use cpu_to_be16 instead since the header length >> was read in from a cpu? > > Now you confuse me ... s390x is big endian, so to get a usable value, we > have to convert big-endian to the host byte order, not the other way > round, don't we? > > Thomas > > Err, yes you're right.
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c index 645b4080c5..ed92ce510d 100644 --- a/hw/s390x/event-facility.c +++ b/hw/s390x/event-facility.c @@ -213,7 +213,7 @@ static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb, event_buf = &red->ebh; event_buf->length = 0; - slen = sizeof(sccb->data); + slen = sccb_data_len(sccb); rc = SCLP_RC_NO_EVENT_BUFFERS_STORED; diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 69a8724dc7..cb8e2e8ec3 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -231,25 +231,30 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb, { SCLPDevice *sclp = get_sclp_device(); SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); - SCCB work_sccb; - hwaddr sccb_len = sizeof(SCCB); + SCCBHeader header; + SCCB *work_sccb; - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); + s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader)); + + work_sccb = g_malloc0(header.length); + s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb, + be16_to_cpu(header.length)); if (!sclp_command_code_valid(code)) { - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); goto out_write; } - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); goto out_write; } - sclp_c->execute(sclp, &work_sccb, code); + sclp_c->execute(sclp, work_sccb, code); out_write: - s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb, - be16_to_cpu(work_sccb.h.length)); + s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb, + be16_to_cpu(work_sccb->h.length)); + g_free(work_sccb); sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR); return 0; } @@ -258,9 +263,8 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) { SCLPDevice *sclp = get_sclp_device(); SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); - SCCB work_sccb; - - hwaddr sccb_len = sizeof(SCCB); + SCCBHeader header; + SCCB *work_sccb; /* first some basic checks on program checks */ if (env->psw.mask & PSW_MASK_PSTATE) { @@ -274,33 +278,37 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) return -PGM_SPECIFICATION; } + /* the header contains the actual length of the sccb */ + cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader)); + + /* Valid sccb sizes */ + if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) { + return -PGM_SPECIFICATION; + } + /* * we want to work on a private copy of the sccb, to prevent guests * from playing dirty tricks by modifying the memory content after * the host has checked the values */ - cpu_physical_memory_read(sccb, &work_sccb, sccb_len); - - /* Valid sccb sizes */ - if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { - return -PGM_SPECIFICATION; - } + work_sccb = g_malloc0(header.length); + cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length)); if (!sclp_command_code_valid(code)) { - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); goto out_write; } - if (!sccb_verify_boundary(sccb, work_sccb.h.length)) { - work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); + if (!sccb_verify_boundary(sccb, work_sccb->h.length)) { + work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION); goto out_write; } - sclp_c->execute(sclp, &work_sccb, code); + sclp_c->execute(sclp, work_sccb, code); out_write: - cpu_physical_memory_write(sccb, &work_sccb, - be16_to_cpu(work_sccb.h.length)); - + cpu_physical_memory_write(sccb, work_sccb, + be16_to_cpu(work_sccb->h.length)); + g_free(work_sccb); sclp_c->service_interrupt(sclp, sccb); return 0; diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index a87ed2a0ab..98c4727984 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -177,7 +177,7 @@ typedef struct IoaCfgSccb { typedef struct SCCB { SCCBHeader h; - char data[SCCB_DATA_LEN]; + char data[]; } QEMU_PACKED SCCB; #define TYPE_SCLP "sclp"
The header contained within the SCCB passed to the SCLP service call contains the actual length of the SCCB. Instead of allocating a static 4K size for the work sccb, let's allow for a variable size determined by the value in the header. The proper checks are already in place to ensure the SCCB length is sufficent to store a full response and that the length does not cross any explicitly-set boundaries. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- hw/s390x/event-facility.c | 2 +- hw/s390x/sclp.c | 58 ++++++++++++++++++++++----------------- include/hw/s390x/sclp.h | 2 +- 3 files changed, 35 insertions(+), 27 deletions(-)