On 3/12/20 9:33 AM, Christian Borntraeger wrote:
>
>
> On 11.03.20 14:21, Janosch Frank wrote:
> [...]
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index b81942e1e6f9002e..98df89e62c25f583 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -27,6 +27,7 @@
>> #include "hw/s390x/vfio-ccw.h"
>> #include "hw/s390x/css.h"
>> #include "hw/s390x/ebcdic.h"
>> +#include "hw/s390x/pv.h"
>> #include "ipl.h"
>> #include "qemu/error-report.h"
>> #include "qemu/config-file.h"
>> @@ -566,12 +567,31 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> - ipl->iplb = *iplb;
>> - ipl->iplb_valid = true;
>> + /*
>> + * The IPLB set and retrieved by subcodes 8/9 is completely
>> + * separate from the one managed via subcodes 5/6.
>> + */
>> + if (iplb->pbt == S390_IPL_TYPE_PV) {
>> + ipl->iplb_pv = *iplb;
>> + ipl->iplb_valid_pv = true;
>> + } else {
>> + ipl->iplb = *iplb;
>> + ipl->iplb_valid = true;
>> + }
>
> We call this for DIAG308_SET and DIAG308_PV_SET in diag.c (see below).
> Doesnt this allow to set S390_IPL_TYPE_PV via subcode 5 and an CCW type
> via subcode 8. It is certainly not an issue security-wise, but it seems to
> violate
> the architecture.
> Shouldnt we add a check in diag.c?
That would make sense, I'll add it
>
> [...]
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> [..]
>> @@ -93,6 +102,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1,
>> uint64_t r3, uintptr_t ra)
>> return;
>> }
>>
>> + if (subcode >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) {
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>> + return;
>> + }
>> +
>> switch (subcode) {
>> case DIAG308_RESET_MOD_CLR:
>> s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
>> @@ -105,6 +119,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1,
>> uint64_t r3, uintptr_t ra)
>> s390_ipl_reset_request(cs, S390_RESET_REIPL);
>> break;
>> case DIAG308_SET:
>> + case DIAG308_PV_SET:
>
> somewhere here after we have loaded the block.
>
>
>
> Other than that this looks good.
>
>
signature.asc
Description: OpenPGP digital signature
